-
Notifications
You must be signed in to change notification settings - Fork 101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prepare active_users_aggregates for a backfill with shredder mitigation. #6349
base: main
Are you sure you want to change the base?
Prepare active_users_aggregates for a backfill with shredder mitigation. #6349
Conversation
…on. Rename columns [first_seen_date, os_version, segment] to cascade upstream changes during the backfill for stable numbers.
…ith_shredder_mitigation
…n is backfilled. Remove the view generation from the new version which conflicts with previous version.
This comment has been minimized.
This comment has been minimized.
…ith_shredder_mitigation
This comment has been minimized.
This comment has been minimized.
…ith_shredder_mitigation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some comments that I think are worth addressing, but it's possible that they're not very relevant. If we can address the comments this looks good to go.
scheduling: | ||
dag_name: bqetl_analytics_aggregations | ||
task_name: {{ app_name }}_active_users_aggregates | ||
task_name: {{ app_name }}_active_users_aggregates_v3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the CODEOWNERS file, all of the tables are referenced as active_users_aggregates_v4
. Should the task name match this? Should the directory also be updated from sql_generators/active_users_aggregates_v3/templates/metadata.yaml
to sql_generators/active_users_aggregates_v4/templates/metadata.yaml
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You;re right that the task must be named _v4, this happens in the folder for version 4.
In v3 folder the task remains as v3 and pending to be removed from the DAG once the v4 is in production.
focus_android_view_template = env.get_template("focus_android_view.sql") | ||
mobile_view_template = env.get_template("mobile_view.sql") | ||
view_template = env.get_template("view.sql") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm intentionally avoiding generating the view for v4 until the table is validated and in production. There should be a PR to generate it then.
TABLE_NAME = os.path.basename(os.path.normpath(THIS_PATH)) | ||
BASE_NAME = "_".join(TABLE_NAME.split("_")[:-1]) | ||
DATASET_FOR_UNIONED_VIEWS = "telemetry" | ||
CHECKS_TEMPLATE_CHANNELS = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't contain Fenix; is that expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is expected. Fenix checks are in a separate file.
elif browser.name == "focus_android": | ||
query_sql = reformat( | ||
focus_android_query_template.render( | ||
project_id=target_project, | ||
app_name=browser.name, | ||
) | ||
) | ||
schema_template = mobile_schema_template |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does focus_android have a query template that's distinct from the mobile query template?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was the only browser that required keeping legacy and Glean data, probably something to reevaluate soon.
type: STRING | ||
mode: NULLABLE | ||
- name: attributed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
attribution_medium, attribution_source, and attributed all seem to be removed from the schema. Is that intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not intentional, added back, thanks!
@@ -0,0 +1,93 @@ | |||
fields: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The desktop v4 schema contains attribution columns while the mobile schema doesn't; is that intentional?
Integration report for "Ensure attribution columns are present in mobile's schema."
|
Description
This PR prepares active_users_aggregates for a backfill with shredder mitigation, which includes renaming columns to cascade upstream changes.
Related Tickets & Documents
Reviewer, please follow this checklist
┆Issue is synchronized with this Jira Task