Conversation
There was a problem hiding this comment.
This PR adds a new remote_settings_derived dataset containing clients_uptake_events_v1, a daily incremental table that unions uptake_remotesettings Glean events from firefox_desktop, fenix, and firefox_ios events_unnested views, tagging each row with an implementation (gecko/rust) and platform, and flattening the event extras into typed extra_* columns. It is scheduled on bqetl_default, partitioned on submission_timestamp with a 365-day expiration, and clustered on channel/implementation/source/status.
I verified the mechanics that tend to break this kind of query: the client_info struct is normalized identically by the generated cross_channel_events_unnested template for all three apps, so the UNION ALL is type-compatible; all seven !include-field-description targets exist in bigquery_etl/schema/global.yaml; the 17 schema fields match the final SELECT in name, order, and type (DECIMAL is a NUMERIC alias and has precedent in monitoring.remote_settings_uptake_live); and bqetl_default exists in dags.yaml. The main thing I'd want resolved before this lands is the shredder registration for client_id — the auto-discovery path doesn't cover this dataset. The rest are convention and observability points.
Per the reviewer checklist, I checked for pre-existing coverage: monitoring.remote_settings_uptake_live reads the same events but only from firefox_desktop_live for real-time Grafana monitoring, so a historical multi-platform table is a genuine addition rather than a duplicate. Labels used below are issue / suggestion / nitpick; blocking is the human reviewer's call.
| mozfun.map.get_key(event_extra, 'value') AS extra_status, | ||
| mozfun.map.get_key(event_extra, 'trigger') AS extra_trigger, | ||
| mozfun.map.get_key(event_extra, 'source') AS extra_source, | ||
| mozfun.map.get_key(event_extra, 'errorName') AS extra_errorname, | ||
| mozfun.map.get_key(event_extra, 'timestamp') AS extra_timestamp, | ||
| SAFE_CAST(mozfun.map.get_key(event_extra, 'age') AS DECIMAL) AS extra_age, | ||
| SAFE_CAST(mozfun.map.get_key(event_extra, 'duration') AS INT64) AS extra_duration |
There was a problem hiding this comment.
suggestion: Every extra_* column depends on an extra key looked up by literal string (value, trigger, source, errorName, age, duration). If any client renames a key — or if the Rust implementation uses different names than Gecko's GIFFT-mirrored value — mozfun.map.get_key returns NULL and the column silently fills with NULLs for those rows, with no failure anywhere in the pipeline.
Adding a checks.sql (see docs/reference/data_checks.md) that asserts a non-null rate per implementation/platform for at least extra_status and extra_source would surface that instead of producing a quietly-empty column. This matters especially for the rust rows, whose keys aren't exercised by the existing monitoring.remote_settings_uptake_live view.
This comment has been minimized.
This comment has been minimized.
|
I have a few questions:
Thank you! |
Run bqetl format
…live Fix yaml indentation
0f70f63 to
d68265c
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Integration report
|
Description
See https://docs.google.com/document/d/1Ly4eLDAOST6vCbaFnhlgx3PZDjFHdslshgi4k14WEnc/edit?tab=t.wmm0pv1y9gk4
Related Tickets & Documents
Reviewer, please follow this checklist