1913438 - [meta] Add profiler markers wherever a piece of data is being collected

Status: RESOLVED FIXED

(Toolkit :: Telemetry, task)

Description

[Florian Quèze [:florian]]

I think we should add profiler markers showing:

• whenever data is collected using Glean
• whenever data is collected using Legacy Telemetry
• (if the existing IPC markers are not clear enough) when data is transferred from child processes to the parent)
• when data is sent. It would be good if we can find a way to visualize the duration of pings, so that it is obvious if some collected data was included, or will need to wait for the next ping to be sent.

Expected benefits:

• Compared to about:telemetry, this would make it easy to see when a piece of a data is collected, and not just what the collected data looks like.
• When a user files a bug and includes a profile to show engineers what happened, seeing which pieces of data were collected during the profiling session might enable using our data to identify if the bug happens for other users too.
• Profiler markers would be added for both legacy telemetry and Glean, enabling developers to see the full picture of what is being collected (instead of using both about:telemetry and about:glean).
• Seeing profiler markers might help detect when some data collection is causing overhead, giving us an opportunity to reduce of eliminate the overhead.

Needinfo on Chutten who offered to help identify where in the code markers could be added.

Comment 1

[Chris H-C :chutten]

The best place to position markers

• for Legacy Telemetry Histograms would be Histogram::Add
• for Legacy Telemetry keyed Histograms would be KeyedHistogram::Add
• for Legacy Telemetry Scalars it'd be in Scalar{Bool|String|Unsigned}::SetValue, and ScalarUnsigned::{AddValue|SetMaximum} (specifically the ones that don't take a variant) (there's a perfect bottleneck in BaseScalar::SetValueInStores() which is called by all of those methods, but if you were hoping to include the instrumented value in your marker, it's not included there)
• for Legacy Telemetry Events it'd be RecordEvent
• for the Legacy Telemetry TelemetryEnvironment would be to mark _onEnvironmentChange
• for Legacy Telemetry custom pings the juice might not be worth the squeeze. We'll get them in ping handling later, and the few that use multi-store scalars or histograms (like the "new-profile" ping) will be covered by the standard marks. But they're each and every one of them bespoke, and our attentions may be better spent waiting to be asked to add markers or improving documentation for self-service there.
• for Legacy Telemetry pings, we should mark TelemetrySendImpl.submitPing. This is the point at which the ping has already been assembled into a payload, and just before we decide whether we're going to try the network or save it to disk. If we also want to know when pings actually hit the network then we'll want to mark the uninspiringly-named TelemetrySendImpl._doPing (you have start and end, failure and success in there).

As for Glean, as you can see from Firefox on Glean's architecture, there's a perfect bottleneck in the Rust FOG API, which is in t/c/glean/api/src/private. (This includes non-builtin (ie, not "baseline", "deletion-request", "events", and "metrics") pings.) Having a rich and expressive API that will result in comprehensive and understandable profiles unfortunately means we have a lot of different places to mark. Each file but mod.rs will have at least one writing API we'll likely be interested in. If we want to capture when code supplies data (as opposed to only marking when data is supplied to the Glean SDK in the parent process), we'll want to mark both ::Parent and ::Child branches. IPC plays out the samples on the parent process using these same APIs, so that will contribute to interesting patterns in profiles. We'll want to update the adding a new metric type docs so any new metric types don't forget to add markers for their APIs.

(( I wonder if there's something clever we could do to use, I dunno, #derive to decorate impl blocks whose functions we want to mark. I've never learned how to write Rust attributes, but maybe there's something fun in there to explore. ))

A question: Do we care solely for cases where data is successfully recorded, or are we concerned about cases where data tried to be recorded but was refused because of reasons including being the wrong process, the data definition being expired, user settings forbidding it, the data being malformed, etc etc? In Telemetry these checks are performed at a variety of times at a variety of levels. In Glean these checks are performed in two places, most commonly inside the SDK (I'll never not take the opportunity to tap the bug 1691073 sign). We might not actually care that much about the difference between these two cases or even being consistent with one or the other, and I personally don't have an opinion at this time, but I thought I'd ask.

To forestall a likely question: Yes, a GIFFT-mirrored metric will mark on both the FOG API for the Glean metric and the Legacy Telemetry API for the mirrored-to probe. And may mark more than once per system depending on whether the data is coming from a non-main process and how we decide to handle IPC.

Comment 2

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

Thanks for all your insights :chutten! I've been looking into this over the past couple of days, and I have a couple of observations/questions (from a profiler perspective):

• I've been looking at the various new and set methods for individual metrics. I assume that these are a good place to start adding markers?
• We can easily add plain/straightfoward markers here quite easily, but it would be even better if we could implement the ProfilerMarker trait for the underlying common metric type. That way, we can serialise it as a market payload straightforwardly. The big downside is that we'd need to make the glean-core crate aware of the profiler, which would mean making the profiler bindings buildable outside of mozilla-central.

Comment 3

[Jan-Erik Rediger [:janerik]]

re 2 (the trait thingy): We might be able to shim that with a minimal crate, which we replace in m-c with the ProfilerMarker trait from the profiler rust-api. And then we hide all of that behind a feature that gets activated only in m-c.

Comment 4

[Jan-Erik Rediger [:janerik]]

Ok, I took a closer look and see that it contains some additional m-c internal types, which makes this whole thing more complicated.

Comment 5

[Chris H-C :chutten]

(In reply to Adam Brouwers-Harries [:aabh] [he/him] ⌚GMT from comment #2)

• I've been looking at the various new and set methods for individual metrics. I assume that these are a good place to start adding markers?

Starting with construction:

• The C++ ctor isn't likely to be all that interesting since instances are singletons and each comprise a single uint32_t (and they're all static constexpr so their timing is boring).
• JS may be of more interest since they're created on use and it could help us learn if the assumptions that led us in bug 1848708 to treat these as non-refcounted plain objects should be revisited... but I bet there are already markers for objects created and destroyed for use in JS contexts (I mean, they're all public nsWrapperCache) so it might be duplicative. Not sure, I'm not a JS whiz.
• Rust construction is on first use only, so might be of interest. Not sure.
• We control codegen for all three of these so adding code is easy and it can be as awkward-looking as you'd like (it doesn't need to be ergonomic). We do have a lot of metrics (and that number grows each day) so beware of anything that inflates the binary size overmuch.

As for data setting methods:

• Definitely 100% we want to instrument these. We can even put in spans between start() and stop() calls as appropriate. I think this is the correct place to begin.

• We can easily add plain/straightfoward markers here quite easily, but it would be even better if we could implement the ProfilerMarker trait for the underlying common metric type. That way, we can serialise it as a market payload straightforwardly. The big downside is that we'd need to make the glean-core crate aware of the profiler, which would mean making the profiler bindings buildable outside of mozilla-central.

Ah, the orphan rule. Our old friend. What are your thoughts about how serde handles this sort of problem?

Comment 6

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

I should have been a little clearer: So far I've just been looking at the rust FOG Rust implementation. I think what you've said still applies though.

R.e. data setting (to make sure that I understand correctly): Will it be "enough" to instrument the various set methods in t/c/glean/api/src/private, or will we also want to build instrumentation into the code generation process, so that we set markers in all of the platforms that support Glean?

Ah, the orphan rule. Our old friend. What are your thoughts about how serde handles this sort of problem?

I /think/ that might be a bit of overkill for the profiler! We actually don't support deriving, so all implementations need to be written by hand. For the glean common metric type, it might be enough to just wrap it in a transparent struct, and them implement the trait for the wrapper.

Comment 7

[Chris H-C :chutten]

(In reply to Adam Brouwers-Harries [:aabh] [he/him] ⌚GMT from comment #6)

I should have been a little clearer: So far I've just been looking at the rust FOG Rust implementation. I think what you've said still applies though.

R.e. data setting (to make sure that I understand correctly): Will it be "enough" to instrument the various set methods in t/c/glean/api/src/private, or will we also want to build instrumentation into the code generation process, so that we set markers in all of the platforms that support Glean?

That will definitely be enough. I don't think we have the capability to easily mark deeply enough to catch all the platforms (e.g. t/c/glean/api/src/private won't catch the Kotlin parts of Fenix, but it'll get all the gecko pieces) so I'm happy if we want to focus on Desktop for now.

Ah, the orphan rule. Our old friend. What are your thoughts about how serde handles this sort of problem?

I /think/ that might be a bit of overkill for the profiler! We actually don't support deriving, so all implementations need to be written by hand. For the glean common metric type, it might be enough to just wrap it in a transparent struct, and them implement the trait for the wrapper.

Fair enough. Another thing to keep in mind is that FOG also needs to be able to compile without gecko (for the rusttests target, mostly). You'll see examples of how we handle that sprinkled around. No biggie, just something you'll probably run into as part of impl.

Comment 8

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

Attachment: Bug 1913438 - WIP: Add profiler markers to the Glean metric collection API r?chutten,florian

Comment 9

[Florian Quèze [:florian]]

(In reply to Chris H-C :chutten from comment #1)

A question: Do we care solely for cases where data is successfully recorded, or are we concerned about cases where data tried to be recorded but was refused because of reasons

Showing which data is collected is a good first step. But if we want to make the profiler a useful tool for debugging data collection, recording failures will be very valuable. So yes, we care :-).

Comment 10

[Florian Quèze [:florian]]

There are a few additional improvements we can make, but we have reached the initial goal of this meta bug, thanks to everybody who has been involved in making this happen!