Bug 2053207 - Use glean-sym for places telemetry [ci full] - #7468
Conversation
|
This seems to be working and I even did a quick check that the FOG stuff works on desktop. However, I'm not sure about a couple things and I left comments for them. |
b7dcf9b to
6a9a5e6
Compare
|
The tests are failing because pip3 can't find the right glean_parser version. I'm not sure what's happening there, any ideas? |
Pretty sure that's because the image in use is way too old to have a recent Python Edit: #7505 |
Use glean-sym for real, rather than just for test metrics. Added support for Desktop/FOG. Removed the metrics meant to test glean-sym. Simplified `run_maintenance` now that we can record metrics in Rust. Updated chrono to `0.4.45` for compatibility with `glean`
6a9a5e6 to
cae24c9
Compare
|
Pushing a new version of this based on all the nice Glean changes in |
| [target.'cfg(any(target_os = "android", target_os = "ios"))'.dependencies] | ||
| glean-sym = { git = "https://github.com/mozilla/glean", tag = "v68.0.0", optional = true } | ||
| glean = { git = "https://github.com/mozilla/glean", branch = "main", optional = true } | ||
| glean-sym = { git = "https://github.com/mozilla/glean", branch = "main" } |
There was a problem hiding this comment.
With the introduction of the glean-sym/noop feature this gets much nicer:
- For Android, we build without any feature flags and use
glean-symfor the metrics - For Desktop, we build with the
glean-fogfeature flag and use that for the metrics. TODO: double check that the APIs are identical and everything compiles cleanly. - For testing, we build with the
glean-sym/noopfeature.
This means that can always just record metrics and never need #[cfg] attributes to conditionally enable them.
The main downside I see is that it's a bit weird for the "default" build, i.e. if I just run cargo build -p places. In that case, we use glean-sym and everything will fail unless it ends up linked to another library like libxul that provides the symbols. This is because we can't have a feature flag that disables glean-sym/noop, only one that enables it. I don't think this is that big of a deal, but I wonder if we could swap the way glean-sym works so that it's noop by default and there's a feature flag to enable the metrics recording. Alternatively, maybe glean-sym could always go into no-op mode if the library failed to load.
The other downside is that we're depending on glean-sym for the Desktop case, when we don't really need it. AFAIK, glean-sym is very lightweight though so I don't care about this at all.
There was a problem hiding this comment.
"Disable it when library couldn't be loaded" -- I initially tried writing the code that way but it was very unwieldy (and that was before we thought about a "real noop" mode). It either requires null-routing all the function pointers or constant checks if function pointers are non-null. Not impossible, but annoying to put together.
No-op by default however seems possible I guess. Cargo features are indeed not perfect for toggling between these modes, so I'm sure we can also find problems with that, so I need to think about it.
2 participants
Use glean-sym for real, rather than just for test metrics. Added support for Desktop/FOG. Removed the metrics meant to test glean-sym.
Simplified
run_maintenancenow that we can record metrics in Rust.Updated chrono to
0.4.45for compatibility withgleanPull Request checklist
[ci full]to the PR title.