1879329 - Changes to metrics change FOG's GleanMetrics.h which is included all over the place which requires broad rebuilds

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement)

Description

[Chris H-C :chutten]

The C++ API for all Glean metrics is in GleanMetrics.h. This is convenient for users as there's little to remember, but it's 31k lines long and only growing, and it changes every time any metric is added, requiring the build to look far and wide for changes.

This is not dissimilar to the problem StaticPrefs faces: a widely-used component with definitions that frequently change. Its solution is to split its headers by pref branch during codegen (e.g. StaticPrefs_dom.h, StaticPrefs_toolkit.h)

We could consider a similar approach, or, really, anything we want. The entire C++ codegen is controlled by these three files (and the last one's only for pings, so it doesn't change nearly as often). No one consumes this but Gecko. None of this code has dependencies or users outside of m-c.

I'm open to suggestions for how to handle this. Build times are an important aspect of the ergonomics of using Glean in m-c.

Comment 1

[Chris H-C :chutten]

Some initial explorations:

We could mimic what StaticPrefs does. They split headers by pref branch, we could split headers by metric category. However, Glean metrics categories are more descriptive than pref branches. For example, categories browser.launched.to.handle and messaging.system.attribution and use.counter.deprecated.ops.page.

I'm not sure I like #include "mozilla/glean/GleanMetrics_usecounterdeprecatedopspage.h", and I'm also not sure I like how, not just specifically for use counters but also generally, this will require many files to include many headers if folks decide to categorize their metrics under multiple categories (which they should feel free to do. This solution would provide back pressure against descriptive or deep category names). Not an ergonomics win overall, even if it cuts build times nicely.

We could go with the first token in the category. I'm not sure #include "mozilla/glean/GleanMetrics_use.h" really communicates what's going on particularly well, though. But it would likely split things along interest areas fairly well and keep builds narrow while not discouraging descriptive or deep category naming. Not bad, but not fantastic.

Maybe there's something we could think of that leverages how the unit of organization of Glean metrics in Firefox Desktop isn't really the metrics category as much as it is the location of the metrics.yaml. I'm not sure how, though:

• #include "mozilla/glean/GleanMetrics_browser_components_privatebrowsing.h"?
• #include "mozilla/glean/browser/components/privatebrowsing/GleanMetrics.h"? (if this is even possible)

Maybe we can tell the compiler to ease off a bit? Though it can't tell that a chance to GleanMetrics.h has no bearing on, say, dom/**/*, we can (because the only metrics.yaml that changed was in security/manager/ssl). (( I have no idea if this is possible, and even if it is whether it'd be desirable ))

Maybe we provide a default organization (say, by first token) but permit overriding? Not the most discoverable, and a system that operates one way except for exceptions is less comprehensible, but it could give us tailored #include "mozilla/glean/GleanMetrics_use_counters.h".

Maybe we leverage the tagging system? All metrics in mozilla-central should be tagged with their BMO Product :: Component (sometimes we miss one, but this is something we can enforce (bug 1749591)). These can get a little wordy (Core :: Dom: Credential Management, Web Compatibility :: Tooling & Investigations, ...) so they have a similar naming problem as using the metric category, but they echo the likely organization of the metrics in use in a given part of the code so shouldn't require many includes per file or different sets of includes between files in the same folder. They're also more stable over time and more likely to be known to developers working in a given component. And, heck, by Conway's Law, the code likely builds along bug component lines already anyway.

I don't think I have a solid idea of a "best" solution here (though I think there's something to the tags approach), but I think I've established that the core things we hope to optimize are:

• Ergonomics: it should be obvious or at least straightforward for developers to figure out how to interact with the solution we choose
• Build time: a change to metrics.yaml in one part of the tree should minimize or remove the need for compilations in other parts of the tree.
• Maintainability: the solution we choose should be as comprehensible as possible to ease long-term maintenance (don't get clever if it means getting obscure)

Comment 2

[Florian Quèze [:florian]]

I think having a one-to-one mapping between each metrics.yaml file and individual header files would make sense. It's something that would be easier to understand for developers.

If I write metric definitions in dom/blah/metrics.yaml, #include <mozilla/dom/blahGleanMetrics.h> seems reasonable.

For an easy migration from the current system, we could keep generating a GleanMetrics.h file that would #include all the other smaller header files.

One challenge I see with this approach is that currently the metric numeric ids are sequential, so adding/remove one new metric in a metrics.yaml file means all the other headers for metrics.yaml files processed after the modified one would be modified too, and we would still rebuild most of the tree.

I think a way around this would be to use the first few bits of the metric id to identify the metric header file, and the last few to identify the metric id within that file. This means we would still rebuild most of the tree when adding a new metrics.yaml file, but we would not when adding content to an existing metrics.yaml file. So that's not a perfect solution, but strictly a win over the current state of things.

We might want to consider something smarter, like a few bits for the top level folder, then a few bits for the rest of the path, so that adding a metrics.yaml file within dom/ at most rebuilds all of dom/. But I'm not sure that extra complexity is worth the effort.

Comment 3

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] from comment #2)

One challenge I see with this approach is that currently the metric numeric ids are sequential, so adding/remove one new metric in a metrics.yaml file means all the other headers for metrics.yaml files processed after the modified one would be modified too, and we would still rebuild most of the tree.

Here's another idea to address this: keep a cache file containing a mapping between metric names and numeric ids. This would allow existing metrics to keep the same ids when rebuilding, and new metrics would be assigned a new metric id that isn't taken yet. This would mean the metric ids would be unchanged compared to what we do currently for clobber builds, and adding a new metric in an incremental build would only rebuild what's strictly necessary.

Comment 4

[[:sergesanspaille]]

(In reply to Florian Quèze [:florian] from comment #2)

One challenge I see with this approach is that currently the metric numeric ids are sequential, so adding/remove one new metric in a metrics.yaml file means all the other headers for metrics.yaml files processed after the modified one would be modified too, and we would still rebuild most of the tree.

What about using a hash for the numeric id instead of a sequential ordering?

Comment 5

[Florian Quèze [:florian]]

Attachment: Bug 1879329 - remove empty mobile/shared/actors/metrics.yaml file, r=chutten.

Depends on D234332

Comment 6

[Florian Quèze [:florian]]

Attachment: Bug 1879329 - generate one header file per metrics.yaml file, r=chutten.

Depends on D234588

Comment 7

[Florian Quèze [:florian]]

Attachment: Bug 1879329 - generate glean metric ids using a hashing algorithm, r=chutten.

Depends on D234589

Comment 8

[Florian Quèze [:florian]]

Attachment: Bug 1879329 - avoid duplicated includes in generated metric header files, r=chutten.

Depends on D234741

Comment 9

[Florian Quèze [:florian]]

Attachment: Bug 1879329 - replace by hands the include of GleanMetrics.h that are difficult to script, r=chutten.

Depends on D234742

Comment 10

[Florian Quèze [:florian]]

Attachment: Bug 1879329 - script generated replacement of the GleanMetrics.h includes, r=chutten.

Depends on D234743

Comment 11

[Florian Quèze [:florian]]

Attachment: Bug 1879329 - add the missing includes, r=chutten.

Depends on D234744

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/885f39a7edc2
remove empty mobile/shared/actors/metrics.yaml file, r=chutten.
https://hg.mozilla.org/integration/autoland/rev/f9a40e2cbadb
generate one header file per metrics.yaml file, r=chutten,sergesanspaille.
https://hg.mozilla.org/integration/autoland/rev/aadc936928a2
generate glean metric ids using a hashing algorithm, r=chutten.
https://hg.mozilla.org/integration/autoland/rev/42f60f21eb0a
avoid duplicated includes in generated metric header files, r=chutten.
https://hg.mozilla.org/integration/autoland/rev/a4f37569c74d
replace by hands the include of GleanMetrics.h that are difficult to script, r=chutten.
https://hg.mozilla.org/integration/autoland/rev/bdfb954837c5
script generated replacement of the GleanMetrics.h includes, r=chutten,valentin,media-playback-reviewers,cookie-reviewers,anti-tracking-reviewers,profiler-reviewers,win-reviewers,rkraesig,emz,aabh,padenot.
https://hg.mozilla.org/integration/autoland/rev/c7fcb496868d
add the missing includes, r=chutten,necko-reviewers,cookie-reviewers,valentin.

Comment 13

[amarc]

https://hg.mozilla.org/mozilla-central/rev/885f39a7edc2
https://hg.mozilla.org/mozilla-central/rev/f9a40e2cbadb
https://hg.mozilla.org/mozilla-central/rev/aadc936928a2
https://hg.mozilla.org/mozilla-central/rev/42f60f21eb0a
https://hg.mozilla.org/mozilla-central/rev/a4f37569c74d
https://hg.mozilla.org/mozilla-central/rev/bdfb954837c5
https://hg.mozilla.org/mozilla-central/rev/c7fcb496868d

Comment 14

[Florian Quèze [:florian]]

Attachment: Node script used to rewrite the includes

For future reference, this is the script I used to generate https://phabricator.services.mozilla.com/D234744

Comment 15

[BMO Github Automation]

Attachment: [mozilla/glean] bug 1879329 - Update Firefox Desktop C++ header includes in docs to use specific headers [doc only] (#3051)