1578719 - glean_parser/probe_scraper disabled calculation is based on present, not the commit

Status: RESOLVED FIXED

(Data Platform and Tools :: Glean: SDK, defect, P3)

Description

[Michael Droettboom [:mdroettboom]]

glean_parser determines if a metric has expired by checking its expired parameter against the current date, and setting the disabled parameter to True if it has expired. This is helpful for the regular use of glean_parser for generating code, since the metric is completely disabled at build time without any additional overhead.

When glean_parser is used inside of probe_scraper, however, the same behavior occurs, even when probe_scraper is extracting a historical revision from before the metric expired. Therefore, these metrics will be marked as disabled in all history entries, even it is wasn't disabled/expired at that time.

This may or may not be a problem, but it's surprising enough that it's probably worth mentioning here and getting some discussion going.

A proper solution probably involves injecting extra history entries on the expiration dates and calculating "disabled" based on the historical date not the current date.

Comment 1

[Frank Bertsch [:frank]]

Thanks for the ping, Mike. This is surprising behavior from the probe-scraper perspective, but makes sense from the client-engineering side.

I think initially I would like a way to disable this behavior altogether, and we can decide down-the-line if we want to add in disabled metrics.

Comment 2

[Alessio Placitelli [:Dexter]]

(In reply to Frank Bertsch [:frank] from comment #1)

I think initially I would like a way to disable this behavior altogether, and we can decide down-the-line if we want to add in disabled metrics.

Hey Frank, what's the correct behaviour you'd like, here? Do you have a desired timeline for that?

Comment 3

[Frank Bertsch [:frank]]

We currently use parse_objects to get the metric definitions. What I'd like is a parameter to that function that indicates we don't want any special behavior for disabled metrics.

In the end what I'd like to see is disabled as another field within the metric, such that the history shows: [{..definition.., disabled: False}, {...definition..., disabled: True}]. There will be a tiny bit of work for the probe-scraper to compare that field over time, but easy to do.

Thoughts on making this available, Mike?

Comment 4

[Michael Droettboom [:mdroettboom]]

That should be easy enough to do.

Comment 5

[Michael Droettboom [:mdroettboom]]

Attachment: GitHub Pull Request

Comment 6

[Michael Droettboom [:mdroettboom]]

Attachment: GitHub Pull Request