Add an equivalent to MetricType and metrics::MetricType::meta to the public Glean Metric API
Categories
(Data Platform and Tools :: Glean: SDK, enhancement, P1)
Tracking
(Not tracked)
People
(Reporter: aabh, Assigned: aabh)
References
Details
Attachments
(1 file)
In order to record comprehensive profiler markers whenever the FoG API is used within Firefox, we need a way to record the label of submetrics.
Luckily, Glean knows what the label of a given metric is! Unfortunately, however, said label is hidden away from us, as we (the profiler) aren't allowed to access it. This is because (taking the Boolean metric as an example), although the glean-core metric does implement MetricType (which would give us the CommonMetricDataInternal, with the (optional) label), the trait itself is private to glean-core, meaning that we would need to re-import the core module to get access to it. In other words, if we try and print the internal metadata name for a boolean metric we get:
0:14.73 error[E0599]: no method named `meta` found for reference `&Arc<glean::private::BooleanMetric>` in the current scope
0:14.73 --> toolkit/components/glean/api/src/private/boolean.rs:119:50
0:14.73 |
0:14.74 119 | println!("Inner data: {}", inner.meta().inner.name);
0:14.74 | ^^^^ private field, not a method
0:14.74 |
0:14.74 = help: items from traits can only be used if the trait is in scope
0:14.74 help: trait `MetricType` which provides `meta` is implemented but not in scope; perhaps you want to import it
0:14.74 |
0:14.74 5 + use glean_core::metrics::MetricType;
attempting to import MetricType give us:
0:08.86 error[E0433]: failed to resolve: use of undeclared crate or module `glean_core`
0:08.86 --> toolkit/components/glean/api/src/private/boolean.rs:119:21
0:08.86 |
0:08.86 119 | use glean_core::metrics::MetricType;
0:08.86 | ^^^^^^^^^^ use of undeclared crate or module `glean_core`
0:08.86 For more information about this error, try `rustc --explain E0433`.
0:08.86 warning: `firefox-on-glean` (lib) generated 2 warnings
0:08.86 error: could not compile `firefox-on-glean` (lib) due to 1 previous error; 2 warnings emitted
(this could obviously be fixed by adding glean-core as a direct dependency of firefox-on-glean, but as this has not been done yet, I suspect that there is a probably a good reason not to do it!)
Step one: Re-export glean_core::metrics::MetricType from the glean crate, so that downstream consumers can access the internal data of metrics.
This is, however, not quite enough for some kinds of metrics. For instance, the EventMetric does not implement the MetricType trait, as it is defined within the glean crate, not glean-core unlike most other metrics (the other exceptions being ObjectMetric). As the inner Event metric is public only within the glean crate, downstream consumers have no way to get access to the internal metadata, even if they were able to import the MetricType trait.
Step two: Implement MetricType for EventMetric<K> and ObjectMetric<K>.
|
Assignee | |
Comment 1•2 months ago
|
||
:chutten - is this an accurate summary of the work that we discussed last month r.e. how to get labeled metrics in parent processes?
|
||
Comment 2•2 months ago
|
||
(this could obviously be fixed by adding glean-core as a direct dependency of firefox-on-glean, but as this has not been done yet, I suspect that there is a probably a good reason not to do it!)
Architecture-wise, FOG doesn't depend directly on glean-core because we're not "supposed" to. There's a separation in principle between glean-core and "the RLB (Rust Language Binding)" (the glean crate`) where the former is supposed to be a multi-language multi-platform "core", and the latter is supposed to be the adapted to be ergonomic in Rust "language binding".
Now, FOG is special because Firefox Desktop demands it to be. So this separation in principle need not necessarily be a separation in fact. But since it's easy enough to maintain the separation in fact (by re-exporting train MetricType say here), let's keep with that.
This is, however, not quite enough for some kinds of metrics. For instance, the EventMetric does not implement the MetricType trait, as it is defined within the glean crate, not glean-core unlike most other metrics (the other exceptions being ObjectMetric). As the inner Event metric is public only within the glean crate, downstream consumers have no way to get access to the internal metadata, even if they were able to import the MetricType trait.
...but it's inner is glean_core::metrics::EventMetric, which does impl MetricType? All the inners need to impl MetricType so they can... actually, weirdly, we don't take advantage of MetricType that much, upon inspection? It's necessary to make labeled metrics work, but that's it, seemingly? Weird. Guess we use it to encode common behaviours that we just use everywhere without them being generic methods, like how we use meta() literally everywhere
:chutten - is this an accurate summary of the work that we discussed last month r.e. how to get labeled metrics in parent processes?
I think so, but my memory is patchy. Let me check the meeting notes again.
Yes. Yes, I think this is the route forward. The profiler needs string identifiers for metrics (including their labels) at streaming time. And we don't want to store those strings more than once, and the SDK has priority on owning those strings at least once for storage+payload assembly purposes (in the parent process). But we have ids that uniquely specify which FOG metric (including its label) we're on about. So we need a mechanism to, on the parent process, go from "metric with id X" to "that's a metric with category C, name N, and [optional] label L" which is information present inside every glean_core metric instance that the MetricType trait could (and might already) supply.
I don't think we necessarily want to run this off of CommonMetricDataInternal. I think we want to write a new pub fn on trait MetricType that supplies the identifier and takes care of supplying it to you. IOW, I think we want the way to use this to be <fog metric instance>.inner.identifier(), not <fog metric instance>.inner.meta().name + checking whether we also need dynamic_label.
If along the way we end up with trait firefox_on_glean::traits::Metric that they all inherit from that ensures that the inner MetricType is always available at inner(), then that might dovetail with some plans we have for reducing the size of our codegen in bug 1932617.
|
|
Assignee | |
Comment 3•2 months ago
|
||
(In reply to Chris H-C :chutten from comment #2)
...but it's
innerisglean_core::metrics::EventMetric, which doesimpl MetricType?
Yes, but if you hand me an EventMetric<K> from glean (not glean core), I cannot access the inner glean_core::metrics::EventMetric, as it is private, so I cannot call the ::meta() method provided by MetricType.
The profiler needs string identifiers for metrics (including their labels) at streaming time. And we don't want to store those strings more than once, and the SDK has priority on owning those strings at least once for storage+payload assembly purposes (in the parent process). But we have ids that uniquely specify which FOG metric (including its label) we're on about. So we need a mechanism to, on the parent process, go from "metric with id X" to "that's a metric with category C, name N, and [optional] label L" which is information present inside every glean_core metric instance that the MetricType trait could (and might already) supply.
Two questions:
- How do we avoid running into Bug 1932617 while looking up the metrics by ID.
- Will this work with dynamic metrics?
IOW, I think we want the way to use this to be
<fog metric instance>.inner.identifier(), not<fog metric instance>.inner.meta().name+ checking whether we also need dynamic_label.
Sounds good!
|
|
||
Comment 4•2 months ago
|
||
(In reply to Adam Brouwers-Harries [:aabh] [he/him] ⌚GMT from comment #3)
(In reply to Chris H-C :chutten from comment #2)
...but it's
innerisglean_core::metrics::EventMetric, which doesimpl MetricType?Yes, but if you hand me an
EventMetric<K>fromglean(not glean core), I cannot access the innerglean_core::metrics::EventMetric, as it is private, so I cannot call the::meta()method provided byMetricType.
I'd never properly recognized that the RLB event and object (and ping) weren't just re-exports of the glean-core versions. You are correct: they should definitely impl glean_core::MetricType (probably by trivially dispatching to inner).
Two questions:
- How do we avoid running into Bug 1932617 while looking up the metrics by ID.
- Will this work with dynamic metrics?
To support the C++ and JS APIs we already have all the structure necessary to map FOG metric ids (static or dynamic) to FOG metric instances. We won't worsen bug 1932617 by simply using what's already there.
The only questions left unanswered in my mind are just how clever we can be when implementing in FOG
trait Metric { fn identifier() -> str; };
IOW, I think we want the way to use this to be
<fog metric instance>.inner.identifier(), not<fog metric instance>.inner.meta().name+ checking whether we also need dynamic_label.Sounds good!
Actually, even better, let's <fog metric instance : Metric>.identifier().
|
||
Updated•28 days ago
|
|
||
Comment 5•18 days ago
|
||
|
|
Assignee | |
Comment 6•6 days ago
|
||
Description
•