1934947 - Hoist label names out of constructors in `metrics.rs` and add a static lookup function, so we can access them from child process

Status: NEW

(Toolkit :: Telemetry, task)

Description

[Adam Brouwers-Harries [:aabh] [he/him] ⌚GMT]

As we start to add profiler markers for labeled metrics, and dynamic metrics (see Bug 1934880), we will notice that our profiler markers will start containing names and labels that are inconsistently in either the JS or YAML conjugation.

• Names that come from the functionality merged in D221795 (as part of Bug 1918097) are in the JS conjugation, as we use the sMetricByNameLookupEntries array in GleanJSMetricsLookup.h to look up the names of metrics.
• Names that come from (or will come from) the Glean itself, in Bug 1934880, and labels that come from Glean in Bug 1927580 will be in YAML conjugation.

(Note that names that will come from Glean in Bug 1934880 will be limited to the parent process, as we do not have a Glean "instance" in child processes.)

If we want to unify this, and provide YAML conjugation names to the profiler in child processes I propose that we "mimic" some of the behaviour in D227874, so that we can avoid going through Glean to access the static name strings, and access them directly.

In other words (and as an example) if we consider the metric networking.captive_portal_banned_display_time defined in metrics.rs here, we can see that we have a static string for the name of the metric:

pub static captive_portal_banner_display_time: Lazy<LabeledMetric<LabeledCounterMetric, CaptivePortalBannerDisplayTimeLabel>> = Lazy::new(|| {
    let meta =
        LabeledMetricData::Common {
            cmd: CommonMetricData {
                name: "captive_portal_banner_display_time".into(),

however, this is only used within the context of a lazy constructor (only called when Glean is instantiated, again, only in the parent process).

We could, instead, hoist this string into a separate variable:

static captive_portal_banner_display_time_name : &'static str = "captive_portal_banner_display_time";
pub static captive_portal_banner_display_time: Lazy<LabeledMetric<LabeledCounterMetric, CaptivePortalBannerDisplayTimeLabel>> = Lazy::new(|| {
    let meta =
        LabeledMetricData::Common {
            cmd: CommonMetricData {
                name: captive_portal_banner_display_time_name.into(),
    ...

and implement a similar function to labeled_enum_to_str, where we could associate a metric id to a given (static) name string, e.g.

pub(crate) fn metric_id_to_name(metric_id: u32) -> &'static str {
    match metric_id {
        ...
        33 => super::networking::captive_portal_banned_display_time_name,
        ...

One major drawback to this would be falling into the trap of Bug 1932617, by adding another large method to the generated FoG code.

Comment 1

[Chris H-C :chutten]

Alternatively we could add this to the trait fog::traits::Metric we keep thinking about in bug 1932617 and bug 1934880. Make it every FOG metric instance's responsibility to return, when asked, its identifier. On the parent process where we hand the strings to the Glean SDK we can obtain it from the SDK by the method in bug 1934880. On other processes, we could supply the ref to the 'static str itself (plus or minus any string labels).

Should save us from another big jump table, and be easier to understand in the future by hanging metric behaviour on a metric instead of supplying it by magic fn from elsewhere in the crate.

Comment 2

[Adam Brouwers-Harries [:aabh] [he/him] ⌚GMT]

That is indeed another possible solution. I admit I'm a little lost as to how we'd obtain the 'static str in child processes if we don't have a Glean instance to instantiate the metrics. Would we try and move towards having some form of static definitions of metrics in metrics.rs that child processes would use, or would we pass the reference directly when constructing the FoG metric? The big downside of the latter (from a profiler perspective) is that we have to record the name immediately, as opposed to storing an index and streaming the name later.

Comment 3

[Chris H-C :chutten]

That's why we'll put the logic in the FOG trait. The FOG metric instance knows whether there's an SDK or not, and will be responsible for doing what it takes to supply the identifier in both cases (asking the SDK for it when present, keeping refs to the ctor-supplied params when not)