feat: update filter for desktop_conversions#9469
Open
kik-kik wants to merge 1 commit into
Open
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
This PR expands the country-allowlist filter used by the firefoxdotcom.desktop_conversion_events view and the fbclid_desktop_conversion_events_export_v1 Python query, growing each list from a handful of countries to ~40, and also reformats a multi-line logging.info call.
Overall the change is mechanical and low-risk — both filter lists are kept alphabetically sorted, which is consistent with the surrounding style. A few things worth a second look before merging:
- The header comment in
view.sqlstill names only the five original countries and is now misleading. - The two country lists are not identical:
query.pyalso includes"China"and"Hong Kong". Worth confirming this is intentional and, if so, capturing the reason in a comment; if not, the two lists should be reconciled. - Maintaining two hand-edited copies of essentially the same list invites drift on the next update — a shared source (lookup table, mozfun constant, or generator) would remove that risk.
- The unrelated logging reformat could live in its own commit to keep this change scoped.
No correctness concerns with the country names themselves (alphabetized, double-quoted, including the correct "Türkiye" spelling). Inline notes on the specific lines.
Comment on lines
+228
to
+236
| logging.info( | ||
| "Event payload: event_name=%s, event_time=%s, event_id=%s, action_source=%s, fbc=%s" | ||
| % (event.event_name, event.event_time, event.event_id, event.action_source, event.user_data.fbc) | ||
| % ( | ||
| event.event_name, | ||
| event.event_time, | ||
| event.event_id, | ||
| event.action_source, | ||
| event.user_data.fbc, | ||
| ) |
Contributor
There was a problem hiding this comment.
nitpick: This logging reformat is unrelated to the country-filter change in the PR title/scope. Not a problem, just calling it out — keeping unrelated style tweaks in a separate commit makes the country-list change easier to audit/revert.
This comment has been minimized.
This comment has been minimized.
kik-kik
force-pushed
the
feat/DENG-10688/desktop_conversion-filter-update
branch
3 times, most recently
from
08de809 to
da99379
Compare
This comment has been minimized.
This comment has been minimized.
Contributor
Integration report for "feat: update filter for desktop_conversions"
|
kik-kik
force-pushed
the
feat/DENG-10688/desktop_conversion-filter-update
branch
from
da99379 to
54b55cc
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
feat: update filter for desktop_conversions