feat(DENG-10365): add mobile to events_first_seen #8647
+3,852
−208
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment has been minimized.
This comment has been minimized.
kbammarito
changed the title
kbammarito
changed the title
remove null as special criteria name and simplify join condition encode criteria as dictionary update tests
This comment has been minimized.
This comment has been minimized.
… need to overwrite entire table) update tests and metadata
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Integration report for "Merge branch 'main' into DENG-10365-add-mobile-to-events-first-seen"
|
sean-rose
reviewed
Jan 6, 2026
Comment on lines
+633
to
+634
| - sql/moz-fx-data-shared-prod/org_mozilla_firefox_derived/events_first_seen_v1 | ||
| - sql/moz-fx-data-shared-prod/org_mozilla_ios_firefox_derived/events_first_seen_v1 |
Contributor
There was a problem hiding this comment.
issue (blocking): These two ETLs you're adding to the retention_exclusion_list config here are for the release channels of Firefox Android and Firefox iOS, but there are additional ETLs for their other channels which should presumably also be added here.
| sql: event_category = 'tabgroup' AND ((event_name = 'smart_tab_suggest' AND JSON_VALUE(event_extra.action) LIKE 'save%') OR (event_name = 'smart_tab_topic' AND JSON_VALUE(event_extra.action) = 'save')) | ||
| - name: "'linkpreview_ai_consent'" | ||
| sql: event_category = 'genai.linkpreview' AND event_name = 'card_ai_consent' AND JSON_VALUE(event_extra.option) = 'continue' | ||
| any_event: | |
Contributor
There was a problem hiding this comment.
note: Changing this criteria name value from null to any_event at this point means you'll need to manually update existing firefox_desktop_derived.events_first_seen_v1 records that currently have null criteria values when this is deployed to avoid getting extraneous any_event records for existing clients when that ETL runs with this new logic.
| depends_on_past: true | ||
| date_partition_parameter: null | ||
|
|
||
| bigquery: |
Contributor
There was a problem hiding this comment.
suggestion (non-blocking): I'd recommend adding criteria as the primary clustering field, removing normalized_country_code as a clustering field (since you can only have four clustering columns), and maybe moving event_category to be the last clustering field.
Comment on lines
+49
to
+53
| AND event_category NOT IN ( | ||
| 'media.playback', | ||
| 'nimbus_events', | ||
| 'uptake.remotecontent.result' | ||
| ) -- remove unnecessary high-volume categories to reduce cost |
Contributor
There was a problem hiding this comment.
quibble: Having the trailing comment was reasonable when this was all on one line, but when the expression is formatted as multiple lines IMO the trailing comment is less readable.
Suggested change
| AND event_category NOT IN ( | |
| 'media.playback', | |
| 'nimbus_events', | |
| 'uptake.remotecontent.result' | |
| ) -- remove unnecessary high-volume categories to reduce cost | |
| -- remove unnecessary high-volume categories to reduce cost | |
| AND event_category NOT IN ( | |
| 'media.playback', | |
| 'nimbus_events', | |
| 'uptake.remotecontent.result' | |
| ) |
| ) | ||
| ORDER BY | ||
| submission_timestamp, | ||
| COALESCE(event_timestamp, '9999-12-31 23:59:59') |
Contributor
There was a problem hiding this comment.
suggestion (non-blocking): I added a document_event_number column to events_stream_v1 tables in #8596, which could be used here to break ties if multiple events have the same timestamp.
Suggested change
| COALESCE(event_timestamp, '9999-12-31 23:59:59') | |
| COALESCE(event_timestamp, '9999-12-31 23:59:59'), | |
| document_event_number |
| 'uptake.remotecontent.result' | ||
| ) -- remove unnecessary high-volume categories to reduce cost | ||
| {% if app_id_dataset == 'firefox_desktop' -%} | ||
| AND profile_group_id IS NOT NULL -- only include non-null IDs so as not to create repeats |
Contributor
There was a problem hiding this comment.
question: Why would including null profile_group_id values create repeats? The query isn't grouping by profile_group_id, so it doesn't seem like including older event records with null profile_group_id values would cause repeats.
In fact, doesn't excluding older event records with null profile_group_id values mean this isn't returning entirely accurate "events first seen" data for Firefox Desktop? It's more like "events first seen since the profile group ID was set".
| , | ||
| _previous_cte AS ( | ||
| SELECT | ||
| first_submission_timestamp, |
Contributor
There was a problem hiding this comment.
suggestion (non-blocking): These _previous_cte queries don't need to select all the data fields anymore since they're only being used for comparison purposes. I think they only need to select client_id, event, and criteria.
| AND _current.event = _previous.event | ||
| AND (_current.criteria = _previous.criteria | ||
| OR (_current.criteria IS NULL AND _previous.criteria IS NULL)) | ||
| AND criteria IS NOT DISTINCT FROM {{ ("'" ~ criteria_name ~ "'") }} |
Contributor
There was a problem hiding this comment.
suggestion (non-blocking): Now that criteria won't have null values, this could be written more naturally using !=:
Suggested change
| AND criteria IS NOT DISTINCT FROM {{ ("'" ~ criteria_name ~ "'") }} | |
| AND criteria != {{ "'" ~ criteria_name ~ "'" }} |
However, I actually think it would instead be better overall to move the UNION ALL criteria loop into the events_stream_cte CTE and have a single not-criteria-specific _previous_cte CTE and final query that selects from events_stream_cte where there isn't a matching _previous_cte record (rather than repeating that code for each criteria).
| # - firefox_ios | ||
| # - fenix | ||
| - fenix | ||
| - firefox_ios |
Contributor
There was a problem hiding this comment.
question: How long do you estimate the init process for these new events_first_seen_v1 ETL tables will take?
Comment on lines
+35
to
+48
| WHERE | ||
| {% raw %} | ||
| {% if is_init() %} | ||
| {% endraw %} | ||
| DATE(submission_timestamp) >= '2023-01-01' -- initialize by looking over all of history | ||
| AND sample_id >= @sample_id | ||
| AND sample_id < @sample_id + @sampling_batch_size | ||
| {% raw %} | ||
| {% else %} | ||
| {% endraw %} | ||
| DATE(submission_timestamp) = @submission_date | ||
| {% raw %} | ||
| {% endif %} | ||
| {% endraw %} |
Contributor
There was a problem hiding this comment.
quibble: I think this would make it easier to see the structure of the Jinja code being generated:
Suggested change
| WHERE | |
| {% raw %} | |
| {% if is_init() %} | |
| {% endraw %} | |
| DATE(submission_timestamp) >= '2023-01-01' -- initialize by looking over all of history | |
| AND sample_id >= @sample_id | |
| AND sample_id < @sample_id + @sampling_batch_size | |
| {% raw %} | |
| {% else %} | |
| {% endraw %} | |
| DATE(submission_timestamp) = @submission_date | |
| {% raw %} | |
| {% endif %} | |
| {% endraw %} | |
| WHERE | |
| {% raw %}{% if is_init() %}{% endraw %} | |
| DATE(submission_timestamp) >= '2023-01-01' -- initialize by looking over all of history | |
| AND sample_id >= @sample_id | |
| AND sample_id < @sample_id + @sampling_batch_size | |
| {% raw %}{% else %}{% endraw %} | |
| DATE(submission_timestamp) = @submission_date | |
| {% raw %}{% endif %}{% endraw %} |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR adds mobile to
events_first_seen, optimizes the query and refactors codeRelated Tickets & Documents
Reviewer, please follow this checklist