From 69fed3a282635a4cc1bee34df788c66f927fe9a5 Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Wed, 3 Dec 2025 12:47:48 +0100 Subject: [PATCH] New lint: Warn against event metrics on the `metrics` or `baseline` ping --- CHANGELOG.md | 2 ++ glean_parser/lint.py | 16 +++++++++ tests/test_lint.py | 86 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 104 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b3c6f4d48..50eeae2c1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Unreleased +- New lint: Warn against event metrics on the `metrics` or `baseline` ping ([#823](https://github.com/mozilla/glean_parser/pull/823)) + ## 18.1.1 - Python server: Fix logging mechanism ([#820](https://github.com/mozilla/glean_parser/pull/820)) diff --git a/glean_parser/lint.py b/glean_parser/lint.py index 9e09b5a51..59632751b 100644 --- a/glean_parser/lint.py +++ b/glean_parser/lint.py @@ -300,6 +300,21 @@ def check_metric_on_events_lifetime( ) +def check_event_on_non_events_ping( + metric: metrics.Metric, parser_config: Dict[str, Any] +) -> LintGenerator: + """ + An event metric should usually go on the `events` ping or a custom ping, + not on a builtin ping. + """ + disallowed_pings = set(pings.RESERVED_PING_NAMES) - {"default", "events"} | {"health"} + if metric.type == "event" and any([ping in disallowed_pings for ping in metric.send_in_pings]): + yield ( + "An event metric should usually go on the `events` ping or a custom ping, " + + "not on a builtin ping." + ) + + def check_unexpected_unit( metric: metrics.Metric, parser_config: Dict[str, Any] ) -> LintGenerator: @@ -466,6 +481,7 @@ def check_name_too_similar( "EXPIRED": (check_expired_metric, CheckType.warning), "OLD_EVENT_API": (check_old_event_api, CheckType.warning), "METRIC_ON_EVENTS_LIFETIME": (check_metric_on_events_lifetime, CheckType.error), + "EVENT_ON_NON_EVENTS_PING": (check_event_on_non_events_ping, CheckType.warning), "UNEXPECTED_UNIT": (check_unexpected_unit, CheckType.warning), "EMPTY_DATAREVIEW": (check_empty_datareview, CheckType.warning), "HIGHER_DATA_SENSITIVITY_REQUIRED": ( diff --git a/tests/test_lint.py b/tests/test_lint.py index 3304a7c69..a1e515cda 100644 --- a/tests/test_lint.py +++ b/tests/test_lint.py @@ -875,3 +875,89 @@ def test_unknown_lint_warning(content, num_nits): assert set(["UNKNOWN_LINT"]) == set( v.check_name for v in nits ) + + +@pytest.mark.parametrize( + "content,num_nits", + [ + ( + { + "plain_event": { + "type": "event", + } + }, + 0, + ), + ( + { + "event_on_metrics": { + "type": "event", + "send_in_pings": ["metrics"], + } + }, + 1, + ), + ( + { + "event_on_metrics_and_events": { + "type": "event", + "send_in_pings": ["metrics", "events"], + } + }, + 1, + ), + ( + { + "event_on_baseline": { + "type": "event", + "send_in_pings": ["baseline"], + "no_lint": ["BASELINE_PING"], + } + }, + 1, + ), + ( + { + "event_on_baseline_and_metrics": { + "type": "event", + "send_in_pings": ["baseline", "metrics"], + "no_lint": ["BASELINE_PING"], + } + }, + 1, + ), + ( + { + "no_lint_events_on_metrics": { + "type": "event", + "send_in_pings": ["metrics", "events"], + "no_lint": ["EVENT_ON_NON_EVENTS_PING"], + } + }, + 0, + ), + ( + { + "default_ping": { + "type": "event", + "send_in_pings": ["default"], + } + }, + 0, + ), + ], +) +def test_events_on_metrics_ping(content, num_nits): + content = {"cat": content} + content = util.add_required(content) + all_metrics = parser.parse_objects(content) + + errs = list(all_metrics) + assert len(errs) == 0 + + nits = lint.lint_metrics(all_metrics.value) + assert len(nits) == num_nits + if num_nits > 0: + assert set(["EVENT_ON_NON_EVENTS_PING"]) == set( + v.check_name for v in nits + )