Repository navigation
ads-client: make telemetry a singleton with the glean-sym API - #7653
Conversation
| inner: AdsClient<MozAdsTelemetryWrapper>, | ||
| inner: AdsClient, | ||
| // The telemetry callback this client installed, if any. | ||
| telemetry: Option<Installation>, |
There was a problem hiding this comment.
Nit: I would like to call this something other than Installation if it's being used outside of the telemetry module (eg: here), this line in particular is a bit obscure to me if I were reading it blindly.
| // Drop the telemetry callback this client installed, unless another client has replaced it. | ||
| if let Some(telemetry) = &self.telemetry { | ||
| telemetry.uninstall(); | ||
| } |
There was a problem hiding this comment.
Is there a reason we've moved the telemetry being held within the AdsClient to the FFI/uniffi layer? I'm not opposed to it, but my understanding (esp. given recent lock changes) are we want to remove as much functionality from FFI layer as possible.
(especially given that this encapsulates a Weak and can be copied everywhere)
| /// Receives the ads client's metrics, for the app to record in Glean. | ||
| #[uniffi::export(callback_interface)] | ||
| pub trait MozAdsTelemetry: Send + Sync { | ||
| // By design: the foreign callback signature; labels come from `metrics.yaml`. |
There was a problem hiding this comment.
Nit/question: if our labels are sourced from metrics.yaml (eg: external), might it be better to centralize them or use an enum or something so we can easily ratify it if things change?
(eg: seeing the impl AdsStoreBuilderError block elsewhere).
|
I generally still have mixed feelings about the singleton of this in the first place, but this is a good implementation IMO and the singleton is still the best step forward here given our discussion. |
Replace the `T: Telemetry` generic threaded through AdsClient, MARSClient, MARSTransport, AdsStoreBuilder and AdResponse::parse with free `telemetry::record_*` functions backed by a process-wide singleton. The recording API matches the one glean-sym will use, so moving to glean-sym later only swaps the backend. For now the backend forwards to a core `Telemetry` trait, which the FFI layer implements by wrapping the `MozAdsTelemetry` callback. `MozAdsClientBuilder::build()` installs it and `MozAdsClient::shutdown()` uninstalls it, unless another client has since replaced it.
- Replace the installation handle with plain `telemetry::install()` and `telemetry::uninstall()`; telemetry is set up once per process, and `AdsClient::shutdown()` drops the callback. - Keep `MozAdsTelemetryWrapper` private to the FFI telemetry module. - Define every metric label in `telemetry.rs`, one enum per metric mirroring `metrics.yaml`. - Move `NoopMozAdsTelemetry` to `test_utils`.
34caa99 to
2f729e2
Compare
2 participants
Telemetry becomes a process-wide singleton recorded with plain
telemetry::record_*calls, matching the API glean-sym will use. For now the backend still forwards to theMozAdsTelemetrycallback.T: Telemetrygeneric; nothing threads telemetry through the clients, transport or store any more.Telemetrytrait and the singleton; the FFI layer wrapsMozAdsTelemetryto implement it. Switching to glean-sym only swapstelemetry/backend.rs.MozAdsClientBuilder::build()installs the callback;MozAdsClient::shutdown()uninstalls it unless another client has replaced it since.Pull Request checklist