Skip to content
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

Prevent glean metadata overrides form overwriting default metadata #280

Merged
merged 1 commit into from
Aug 23, 2024

Conversation

BenWu
Copy link
Contributor

@BenWu BenWu commented Aug 21, 2024

#266 introduced a bug where the metadata for a ping set at the app-level might overwrite the app defaults in moz_pipeline_metadata_defaults. This is because GleanPing.apply_default_metadata is called twice and the first call puts the default_metadata dict in the ping and then the second call will update that dict in place so the default_metadata will then have new values.

For example, this can be seen in pine.deletion-request where the retention is set to 1130 but it's set to 10000 in probe-scraper.

I made a pr against the generated schemas to show the changes this would make on the schemas mozilla-services/mozilla-pipeline-schemas#827. I'll go through those and make sure those are correct and won't unexpectedly delete things. All the changes are decreases in retention period except for pine deletion-requests and pageload which means that this bug shouldn't have deleted anything unexpectedly.

Copy link
Contributor

@akkomar akkomar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, leaving final r+ to whd since we don't yet know if this won't delete anything (or re-request me pls).

@BenWu
Copy link
Contributor Author

BenWu commented Aug 21, 2024

I looked through mozilla-services/mozilla-pipeline-schemas#827 and the new settings look to match probe-scraper. The only issue is that the firefox ios sync pings were added exactly 400 days ago and the default retention for the app is 400 so partitions would start getting dropped from this change. We should manually set the retention for those in probe scraper before making this fix. I commented on each of the affected pings in the other pr

@whd
Copy link
Member

whd commented Aug 22, 2024

https://github.com/mozilla/probe-scraper/pull/795/files will land today after which I think it's safe to propagate this.

@BenWu
Copy link
Contributor Author

BenWu commented Aug 23, 2024

I'll merge this and rerun schema generator and make sure it's working before the end of the day

@BenWu BenWu merged commit 5af721c into main Aug 23, 2024
2 checks passed
@BenWu BenWu deleted the benwu/fix-glean-metadata-override branch August 23, 2024 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants