Merge pull request #27 from mdboom/reserved

1522160: Enforce the `glean` category as a reserved word.

glean_parser/__main__.py

@@ -52,7 +52,15 @@ from . import validate_ping
     multiple=True,
     required=False,
 )
-def translate(input, format, output, option):
+@click.option(
+    '--allow-reserved',
+    is_flag=True,
+    help=(
+        "If provided, allow the use of reserved fields. "
+        "Should only be set when building the Glean library itself."
+    )
+)
+def translate(input, format, output, option, allow_reserved):
     """
     Translate metrics.yaml files to other formats.
     """
@@ -66,7 +74,8 @@ def translate(input, format, output, option):
             [Path(x) for x in input],
             format,
             Path(output),
-            option_dict
+            option_dict,
+            {'allow_reserved': allow_reserved}
         )
     )
 

glean_parser/parser.py

@@ -113,7 +113,7 @@ def _load_metrics_file(filepath):
         return metrics_content
 
 
-def _merge_and_instantiate_metrics(filepaths):
+def _merge_and_instantiate_metrics(filepaths, config):
     """
     Load a list of metrics.yaml files, convert the JSON information into Metric
     objects, and merge them into a single tree.
@@ -128,6 +128,14 @@ def _merge_and_instantiate_metrics(filepaths):
         for category_key, category_val in metrics_content.items():
             if category_key.startswith('$'):
                 continue
+            if (not config.get('allow_reserved') and
+                    category_key.split('.')[0] == 'glean'):
+                yield (
+                    f"{filepath}: For category '{category_key}': "
+                    f"Categories beginning with 'glean' are reserved for "
+                    f"glean internal use."
+                )
+                continue
             output_metrics.setdefault(category_key, {})
             for metric_key, metric_val in category_val.items():
                 try:
@@ -154,7 +162,7 @@ def _merge_and_instantiate_metrics(filepaths):
 
 
 @util.keep_value
-def parse_metrics(filepaths):
+def parse_metrics(filepaths, config={}):
     """
     Parse one or more metrics.yaml files, returning a tree of `metrics.Metric`
     instances.
@@ -170,7 +178,13 @@ def parse_metrics(filepaths):
     The result value is a dictionary of category names to categories, where
     each category is a dictionary from metric name to `metrics.Metric`
     instances.
+
+    :param filepaths: list of Path objects to metrics.yaml files
+    :param config: A dictionary of options that change parsing behavior.
+        Supported keys are:
+            - `allow_reserved`: When True, allow values that are reserved for
+              internal Glean use.
     """
     filepaths = util.ensure_list(filepaths)
-    all_metrics = yield from _merge_and_instantiate_metrics(filepaths)
+    all_metrics = yield from _merge_and_instantiate_metrics(filepaths, config)
     return all_metrics

glean_parser/translate.py

@@ -22,7 +22,13 @@ OUTPUTTERS = {
 }
 
 
-def translate(input_filepaths, output_format, output_dir, options={}):
+def translate(
+        input_filepaths,
+        output_format,
+        output_dir,
+        options={},
+        parser_config={}
+):
     """
     Translate the files in `input_filepaths` to the given `output_format` and
     put the results in `output_dir`.
@@ -32,11 +38,13 @@ def translate(input_filepaths, output_format, output_dir, options={}):
     :param output_dir: the path to the output directory
     :param options: dictionary of options. The available options are backend
         format specific.
+    :param parser_config: A dictionary of options that change parsing behavior.
+        See `parser.parse_metrics` for more info.
     """
     if output_format not in OUTPUTTERS:
         raise ValueError(f"Unknown output format '{output_format}'")
 
-    all_metrics = parser.parse_metrics(input_filepaths)
+    all_metrics = parser.parse_metrics(input_filepaths, parser_config)
     found_error = False
     for error in all_metrics:
         found_error = True

tests/test_cli.py

@@ -37,7 +37,8 @@ def test_translate(tmpdir):
             '-f',
             'kotlin',
             '-s',
-            'namespace=Foo'
+            'namespace=Foo',
+            '--allow-reserved'
         ]
     )
     assert result.exit_code == 0

tests/test_kotlin.py

@@ -23,7 +23,8 @@ def test_parser(tmpdir):
         ROOT / "data" / "core.yaml",
         'kotlin',
         tmpdir,
-        {'namespace': 'Foo'}
+        {'namespace': 'Foo'},
+        {'allow_reserved': True}
     )
 
     assert (

tests/test_parser.py

@@ -190,3 +190,25 @@ def test_event_must_be_ping_lifetime():
     errors = list(all_metrics)
     assert len(errors) == 1
     assert "On instance['category']['metric']['lifetime']" in errors[0]
+
+
+def test_parser_reserved():
+    contents = [
+        {
+            'glean.baseline': {
+                'metric': {
+                    'type': 'string',
+                },
+            },
+        },
+    ]
+
+    contents = [util.add_required(x) for x in contents]
+    all_metrics = parser.parse_metrics(contents)
+    errors = list(all_metrics)
+    assert len(errors) == 1
+    assert "For category 'glean.baseline'" in errors[0]
+
+    all_metrics = parser.parse_metrics(contents, {'allow_reserved': True})
+    errors = list(all_metrics)
+    assert len(errors) == 0

tests/test_translate.py

@@ -23,7 +23,12 @@ def test_translate_unknown_format():
 def test_translate_missing_directory(tmpdir):
     output = Path(tmpdir) / 'foo'
 
-    translate.translate(ROOT / 'data' / 'core.yaml', 'kotlin', output)
+    translate.translate(
+        ROOT / 'data' / 'core.yaml',
+        'kotlin',
+        output,
+        parser_config={'allow_reserved': True}
+    )
 
     assert len(list(output.iterdir())) == 5
 
@@ -31,7 +36,12 @@ def test_translate_missing_directory(tmpdir):
 def test_translate_remove_obsolete_files(tmpdir):
     output = Path(tmpdir) / 'foo'
 
-    translate.translate(ROOT / 'data' / 'core.yaml', 'kotlin', output)
+    translate.translate(
+        ROOT / 'data' / 'core.yaml',
+        'kotlin',
+        output,
+        parser_config={'allow_reserved': True}
+    )
 
     assert len(list(output.iterdir())) == 5