Histogram Review Guidelines - Copy-Edit

Correct mis-rendered links and clean up the grammar slightly.

Change-Id: I5bbb8af18f40713957625805ae4757a657469d56
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1823800
Auto-Submit: Mark Pearson <mpearson@chromium.org>
Reviewed-by: Nik Bhagat <nikunjb@chromium.org>
Commit-Queue: Nik Bhagat <nikunjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#699888}

tools/metrics/histograms/review_guidelines.md

@@ -4,7 +4,7 @@
 
 This covers how to review metrics code in the Chromium codebase.
 
-## Reviewing UMA Histograms
+## UMA Histograms
 
 ### What is covered under review?
 
@@ -16,23 +16,23 @@ During code review ensure the following -
     (i.e if the histogram name is A.B.C, then A is the histogram namespace).
     If this is a new one, check if there a similar one that already exists?
 
-*   Histogram owners match the [histogram owners guidelines]
-    (https://chromium.googlesource.com/chromium/src/+/HEAD/tools/metrics/histograms/README.md#owners).
+*   Histogram owners match the
+    [histogram owners guidelines](https://chromium.googlesource.com/chromium/src/+/HEAD/tools/metrics/histograms/README.md#owners).
 
 *   If generating multiple histograms programmatically or defining common set of
-    histogram, guide them to use [histogram-suffixes]
-    (https://chromium.googlesource.com/chromium/src/tools/+/refs/heads/master/metrics/histograms/README.md#Histogram-Suffixes).
+    histogram, guide them to use
+    [histogram-suffixes](https://chromium.googlesource.com/chromium/src/tools/+/refs/heads/master/metrics/histograms/README.md#Histogram-Suffixes).
 
 *   Verify that expires_after is reasonable. CL author should be able to justify
-    it. See our guidance at on [histogram-expiry]
-    (https://chromium.googlesource.com/chromium/src/+/HEAD/tools/metrics/histograms/README.md#Histogram-Expiry).
+    it. See guidance at on
+    [histogram-expiry](https://chromium.googlesource.com/chromium/src/+/HEAD/tools/metrics/histograms/README.md#Histogram-Expiry).
 
 *   Summary section should describe what is measured, when is it recorded and
     when not. The summary should generally describe a single emission of sample
     and not the statistics in aggregate.
 
 *   If the histogram is recorded only for some platforms, then it should be
-    included in the summary (Unless part of name).
+    included in the summary (unless part of its name).
 
 *   Histogram and enum names don't include special characters besides dot,
     underscores or slashes.
@@ -48,27 +48,30 @@ During code review ensure the following -
 
 *   If modifying an existing histogram, request that the histogram be
     renamed if its meaning has changed significantly. Common practices are to
-    add suffix such as 2 to the name. When doing so the existing entry for the
+    add suffix such as 2 to the name. When doing so, the existing entry for the
     histogram should also be kept but with `<obsolete>` tag.
 
 *   Don't allow deleting histograms or enum buckets unless there is a *very*
     compelling reason to do so (e.g. never logged). Instead they should be
     marked as obsolete with `<obsolete>` tag.
 
-*   Re-naming enum bucket values is not allowed as these break backward
-    compatibility wrt the data stored. When modifying enums it is better to add
-    new values to the enum instead of re-purposing existing enum values.
+*   Re-numbering enum bucket values is not allowed as these break backward
+    compatibility with respect to the data stored.
 
-    *   Modifying labels / summary is safe and allowed without review. However,
-        if reviewing these changes make sure the semantic meaning of the bucket
-        remain unchanged. e.g re-labeling 'Has Error' to 'Has Warning' should
-        not be allowed, while re-labeling 'Has Error' to 'Has Error (e.g this
-        and that error)' is okay.
+*   When modifying enums it is better to add new values to the enum instead of
+    re-purposing existing enum values.
+
+*   Modifying enum labels / summary is safe and allowed without review. However,
+    if reviewing these changes make sure the semantic meaning of the bucket
+    remain unchanged. e.g re-labeling 'Has Error' to 'Has Warning' should not be
+    allowed, while re-labeling 'Has Error' to 'Has Error (e.g this and that
+    error)' is okay.
 
 *   Verify that histogram buckets are not a privacy risk. Some of the types
-    forbidden are if the buckets are encoding page contents, URL or is including
-    any other type of PII or sensitive information. If during review you are
-    unsure then please loop in Chrome Privacy Team in the review.
+    forbidden are if the buckets are encoding page contents, URL, domain name,
+    or is including any other type of personally identifying or sensitive
+    information. If during review you are unsure then do not hesitate to request
+    that Chrome Privacy Team review the change.
 
 *   Check that the histogram bucket space of all possible values for all clients
     will be limited to 50 generally. It should not exceed 100 unless the CL
@@ -90,13 +93,12 @@ During code review ensure the following -
     the max. This ensures that even outliers will not overflow the distribution.
 
 *   Verify that for an enum histogram, the enum described in enums.xml and the
-    enum defined in the client code matches. And the enum in the code should
-    have a comment mentioning to keep in-sync with enums.xml.
-
-    *   See the sample comment [here]
-        (https://cs.chromium.org/chromium/src/base/metrics/histogram_macros.h?rcl=2c99f35f64380ba63c928787834661fbc1fa4234&l=46)
-        . The comment should be similar.
+    enum defined in the client code matches. Furthermore, the enum in the code
+    should have a comment mentioning that the values must not be changes and aslo that additions to the enum should be synced to enums.xml.
 
+    *   See the sample comment
+        [here](https://cs.chromium.org/chromium/src/base/metrics/histogram_macros.h?rcl=2c99f35f64380ba63c928787834661fbc1fa4234&l=46).
+        The comment should be identical or nearly so.
 
 *   If the histogram is logged via a macro (rather than a call to the function),
     check that the names will be constant at runtime.
@@ -107,25 +109,26 @@ During code review ensure the following -
     recorded and are not reviewing all the corner cases associated with
     collecting the histogram.
 
-*   enums.xml changes don't need our review. But it is still useful to verify
-    that the changes match our guidelines mentioned above.
+*   enums.xml changes don't need a review. However, it is still useful to verify
+    that the changes match the guidelines mentioned above.
 
-### User actions specific
+## User Actions
 
 *   Verify that the user action logged is actually user triggered. If they
-    are not then advise cl author to convert them to a histogram. However if
-    ordering of actions is the interesting part of their analysis then maybe
-    see if each order combination can become a histogram bucket instead of
-    user action.
+    are not then advise cl author to convert them to a histogram.
+
+    *   However if ordering of actions is the interesting part of their analysis
+        then maybe see if each order combination can become a histogram bucket
+        instead of user action.
 
 *   Don't allow logging of noisy user actions (like scroll events). Typical
     allowed frequency is to be less frequent than PageLoad or MobilePageLoaded
     event.
 
-## UKM
+## UKMs
 
-*   UKM metrics are to be reviewed by UKM [data privacy owners]
-(https://cs.chromium.org/chromium/src/tools/metrics/ukm/PRIVACY_OWNERS).
+*   UKM metrics are to be reviewed by UKM
+    [data privacy owners](https://cs.chromium.org/chromium/src/tools/metrics/ukm/PRIVACY_OWNERS).
 
 *   The metrics must follow the
     [data collection guideline](/analysis/uma/g3doc/ukm/ukm.md#adding-ukms).
@@ -134,7 +137,7 @@ During code review ensure the following -
 
 ## Other specialized metrics
 
-ChromeUserMetricsExtension proto include variety of other metrics like -
-Omnibox, Profiler, Stability, etc. These are specialized reviews and should
-be routed to relevant owner. The guidelines here don't cover these cases
+The ChromeUserMetricsExtension proto includes a variety of other fields such as
+records for Omnibox, Profiler, Stability, etc. These are specialized reviews and
+should be routed to relevant owner. The guidelines here don't cover these cases
 and typically require a server-side review first to change the proto.