bug 1955428 - Permit use of reserved metric category 'glean' only for 'glean.{attribution|distribution}.ext' object metrics

Author: chutten

CHANGELOG.md

@@ -2,6 +2,8 @@
 
 ## Unreleased
 
+- Permit object metrics named 'glean.attribution.ext' or 'glean.distribution.ext' even if allow_reserved is false ([bug 1955428](https://bugzilla.mozilla.org/show_bug.cgi?id=1955428))
+
 ## 17.0.1
 
 - BUGFIX: Fix missing `ping_arg` "`uploader_capabilities`" in util.py ([#786](https://github.com/mozilla/glean_parser/pull/786))

glean_parser/parser.py

@@ -188,19 +188,32 @@ def _instantiate_metrics(
         if category_key == "no_lint":
             continue
         if not config.get("allow_reserved") and category_key.split(".")[0] == "glean":
-            yield util.format_error(
-                filepath,
-                f"For category '{category_key}'",
-                "Categories beginning with 'glean' are reserved for "
-                "Glean internal use.",
-            )
-            continue
+            if category_key not in ("glean.attribution", "glean.distribution"):
+                yield util.format_error(
+                    filepath,
+                    f"For category '{category_key}'",
+                    "Categories beginning with 'glean' are reserved for "
+                    "Glean internal use.",
+                )
+                continue
         all_objects.setdefault(category_key, DictWrapper())
 
         if not isinstance(category_val, dict):
             raise TypeError(f"Invalid content for {category_key}")
 
         for metric_key, metric_val in sorted(category_val.items()):
+            if (
+                not config.get("allow_reserved")
+                and category_key in ("glean.attribution", "glean.distribution")
+                and metric_key != "ext"
+            ):
+                yield util.format_error(
+                    filepath,
+                    f"For {category_key}.{metric_key}",
+                    f"May only use semi-reserved category {category_key} with metric name 'ext'",
+                    metric_val.defined_in["line"],
+                )
+                continue
             try:
                 metric_obj = Metric.make_metric(
                     category_key, metric_key, metric_val, validated=True, config=config
@@ -214,18 +227,28 @@ def _instantiate_metrics(
                 )
                 metric_obj = None
             else:
-                if (
-                    not config.get("allow_reserved")
-                    and "all-pings" in metric_obj.send_in_pings
-                ):
-                    yield util.format_error(
-                        filepath,
-                        f"On instance {category_key}.{metric_key}",
-                        'Only internal metrics may specify "all-pings" '
-                        'in "send_in_pings"',
-                        metric_val.defined_in["line"],
-                    )
-                    metric_obj = None
+                if not config.get("allow_reserved"):
+                    if "all-pings" in metric_obj.send_in_pings:
+                        yield util.format_error(
+                            filepath,
+                            f"On instance {category_key}.{metric_key}",
+                            'Only internal metrics may specify "all-pings" '
+                            'in "send_in_pings"',
+                            metric_val.defined_in["line"],
+                        )
+                        metric_obj = None
+                    elif (
+                        metric_obj.identifier()
+                        in ("glean.attribution.ext", "glean.distribution.ext")
+                        and metric_obj.type != "object"
+                    ):
+                        yield util.format_error(
+                            filepath,
+                            f"On instance {category_key}.{metric_key}",
+                            "Extended attribution/distribution metrics must be of type 'object'",
+                            metric_val.defined_in["line"],
+                        )
+                        metric_obj = None
 
             if metric_obj is not None:
                 metric_obj.no_lint = sorted(set(metric_obj.no_lint + global_no_lint))
@@ -481,16 +504,16 @@ def parse_objects(
                 raise TypeError(f"Invalid content for {filepath}")
 
             for category_key, category_val in sorted(content.items()):
-              if category_key.startswith("$"):
-                  continue
+                if category_key.startswith("$"):
+                    continue
 
-              interesting_metrics_dict.setdefault(category_key, DictWrapper())
+                interesting_metrics_dict.setdefault(category_key, DictWrapper())
 
-              if not isinstance(category_val, dict):
-                  raise TypeError(f"Invalid category_val for {category_key}")
+                if not isinstance(category_val, dict):
+                    raise TypeError(f"Invalid category_val for {category_key}")
 
-              for metric_key, metric_val in sorted(category_val.items()):
-                  interesting_metrics_dict[category_key][metric_key] = metric_val
+                for metric_key, metric_val in sorted(category_val.items()):
+                    interesting_metrics_dict[category_key][metric_key] = metric_val
 
         for category_key, category_val in all_objects.items():
             if category_key == "tags":

tests/data/attribution.yaml

@@ -0,0 +1,93 @@
+# Any copyright is dedicated to the Public Domain.
+# https://creativecommons.org/publicdomain/zero/1.0/
+
+---
+$schema: moz://mozilla.org/schemas/glean/metrics/2-0-0
+
+glean.attribution:
+  ext:
+    type: object
+    lifetime: user
+    # Permit long description lines
+    # yamllint disable
+    description: |
+      Extended attribution information.
+      Mapped to client_info.attribution.ext in datasets.
+      * `experiment`: name/id of the enrolled funnel experiment
+      * `ua`: identifier derived from the user agent downloading the installer
+              e.g. chrome, Google Chrome 123
+      * `dltoken`: Unique token created at Firefox download time.
+                   e.g. c18f86a3-f228-4d98-91bb-f90135c0aa9c
+      * `msstoresignedin`: only present if the installation was done through the Microsoft Store,
+                           and was able to retrieve the "campaign ID" it was first installed with.
+                           This value is "true" if the user was signed into the Microsoft Store
+                           when they first installed, and false otherwise.
+      * `dlsource`: identifier that indicate where installations of Firefox originate
+    # yamllint enable
+    bugs:
+      - https://bugzilla.mozilla.org/1955428
+    data_reviews:
+      - http://example.com/reviews
+    notification_emails:
+      - CHANGE-ME@example.com
+    send_in_pings:
+      - metrics
+      - baseline
+      - events
+    expires: never
+    no_lint:
+      - BASELINE_PING
+    structure:
+      type: object
+      properties:
+        experiment:
+          type: string
+        ua:
+          type: string
+        dltoken:
+          type: string
+        msstoresignedin:
+          type: boolean
+        dlsource:
+          type: string
+
+glean.distribution:
+  ext:
+    type: object
+    lifetime: user
+    description: |
+      Extended distribution information.
+      Mapped to client_info.distribution.ext in datasets.
+      * `distributionVersion`: pref `distribution.version`, `null` on failure
+      * `partnerId`: pref `mozilla.partner.id`, `null` on failure
+      * `distributor`:  pref `app.distributor`, `null` on failure
+      * `distributorChannel`: pref `app.distributor.channel`, `null` on failure
+      * `partnerNames`: list from prefs `app.partner.<name>=<name>`
+    bugs:
+      - https://bugzilla.mozilla.org/1955428
+    data_reviews:
+      - http://example.com/reviews
+    notification_emails:
+      - CHANGE-ME@example.com
+    send_in_pings:
+      - metrics
+      - baseline
+      - events
+    expires: never
+    no_lint:
+      - BASELINE_PING
+    structure:
+      type: object
+      properties:
+        distributionVersion:
+          type: string
+        partnerId:
+          type: string
+        distributor:
+          type: string
+        distributorChannel:
+          type: string
+        partnerNames:
+          type: array
+          items:
+            type: string

tests/data/bad_attribution.yamlx

@@ -0,0 +1,36 @@
+# Any copyright is dedicated to the Public Domain.
+# https://creativecommons.org/publicdomain/zero/1.0/
+
+---
+$schema: moz://mozilla.org/schemas/glean/metrics/2-0-0
+
+glean.attribution:
+  not_ext:
+    type: object
+    description: |
+      Can only use glean.attribution if our name is ext.
+    bugs:
+      - https://bugzilla.mozilla.org/1955428
+    data_reviews:
+      - http://example.com/reviews
+    notification_emails:
+      - CHANGE-ME@example.com
+    expires: never
+    structure:
+      type: object
+      properties:
+        experiment:
+          type: string
+
+glean.distribution:
+  ext:
+    type: counter
+    description: |
+      Can only use glean.distribution.ext if our type is object.
+    bugs:
+      - https://bugzilla.mozilla.org/1955428
+    data_reviews:
+      - http://example.com/reviews
+    notification_emails:
+      - CHANGE-ME@example.com
+    expires: never

tests/data/reserved_categories.yamlx

@@ -0,0 +1,33 @@
+# Any copyright is dedicated to the Public Domain.
+# https://creativecommons.org/publicdomain/zero/1.0/
+
+---
+$schema: moz://mozilla.org/schemas/glean/metrics/2-0-0
+
+glean:
+  something:
+    type: counter
+    lifetime: ping
+    description: |
+      Just a test metric to fail.
+    bugs:
+      - https://bugzilla.mozilla.org/1955428
+    data_reviews:
+      - http://example.com/reviews
+    notification_emails:
+      - CHANGE-ME@example.com
+    expires: never
+
+glean.subcategory:
+  someotherthing:
+    type: counter
+    lifetime: ping
+    description: |
+      Just a test metric to fail.
+    bugs:
+      - https://bugzilla.mozilla.org/1955428
+    data_reviews:
+      - http://example.com/reviews
+    notification_emails:
+      - CHANGE-ME@example.com
+    expires: never

tests/test_parser.py

@@ -714,7 +714,7 @@ def test_interesting_disables_others():
     # But, only the metrics in metric-with-tags.yaml are marked as interesting
     # so every other metric will be marked as disabled.
     metrics_with_tags_yaml = ROOT / "data" / "metric-with-tags.yaml"
-    interesting = [Path(path) for path in [ metrics_with_tags_yaml ]]
+    interesting = [Path(path) for path in [metrics_with_tags_yaml]]
     config = {"interesting": interesting}
 
     all_metrics = parser.parse_objects([all_metrics, metrics_with_tags_yaml], config)
@@ -728,14 +728,14 @@ def test_interesting_disables_others():
     metrics = all_metrics.value
     for category_key, category_val in metrics.items():
         if category_key == "tags":
-          continue
+            continue
 
         for metric_key, metric_val in sorted(category_val.items()):
             cat = interesting_metrics.value.get(category_key)
             if cat and cat.get(metric_key):
-              disabled = cat.get(metric_key).disabled
+                disabled = cat.get(metric_key).disabled
             else:
-              disabled = True
+                disabled = True
             assert metrics[category_key][metric_key].disabled is disabled
 
 
@@ -1259,3 +1259,31 @@ def test_object_invalid():
     errors = list(all_metrics)
     assert len(errors) == 1
     assert "invalid `type` in object structure." in errors[0]
+
+
+def test_reserved_category():
+    all_metrics = parser.parse_objects(ROOT / "data" / "reserved_categories.yamlx")
+
+    errors = list(all_metrics)
+    assert len(errors) == 2
+    assert "Categories beginning with 'glean' are reserved" in errors[0]
+    assert "Categories beginning with 'glean' are reserved" in errors[1]
+
+
+def test_attribution_distribution():
+    all_metrics = parser.parse_objects(ROOT / "data" / "attribution.yaml")
+
+    errors = list(all_metrics)
+    assert len(errors) == 0
+
+
+def test_forbid_non_ext_non_object_attribution_distribution():
+    all_metrics = parser.parse_objects(ROOT / "data" / "bad_attribution.yamlx")
+
+    errors = list(all_metrics)
+    assert len(errors) == 2
+    assert "May only use semi-reserved category" in errors[0]
+    assert (
+        "Extended attribution/distribution metrics must be of type 'object'"
+        in errors[1]
+    )