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

feat(inbound-filters): Create new legacy browser filters #2950

Merged
merged 8 commits into from
Jan 23, 2024

Conversation

roggenkemper
Copy link
Member

@roggenkemper roggenkemper commented Jan 16, 2024

this pr adds in new legacy browser filters. These new filters will be more up to date than the existing ones, and would allow for easier updates to the filters if needed. right now, the filters are defined as both a browser and a version ("safari_pre_6"). now the filters will just be for a browser, and we will have easier control over what versions to filter- we no longer have to make a new filter, we can just update what values get filtered. this can both be a short term and longer term fix to the problem with legacy browsers. It will allow us to move over to new filters in the short term, while supporting updates in the long term if needed (since the generic filters project doesn't have a timeline right now).

closes getsentry/sentry#63053

@roggenkemper
Copy link
Member Author

@getsentry/ingest let me know your thoughts on this pr! was going off of previous PRs that added new filters since this isn't my area of expertise.

One question I have: Is it safe to merge this before any of the sentry changes that actually add in the ability to turn on these filters? or do we need to wait until the sentry changes to the settings/endpoint are merged in?

@roggenkemper roggenkemper marked this pull request as ready for review January 16, 2024 22:23
@roggenkemper roggenkemper requested a review from a team as a code owner January 16, 2024 22:23
Copy link
Member

@jjbayer jjbayer left a comment

Choose a reason for hiding this comment

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

This looks good to me, @roggenkemper would you mind adding some tests? There should be some you can copy-paste in legacy_browsers.rs.

@jjbayer
Copy link
Member

jjbayer commented Jan 19, 2024

Is it safe to merge this before any of the sentry changes that actually add in the ability to turn on these filters?

That should be safe AFAIK.

@roggenkemper roggenkemper requested a review from jjbayer January 23, 2024 04:20
CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Joris Bayer <[email protected]>
@roggenkemper roggenkemper merged commit b65956a into master Jan 23, 2024
20 checks passed
@roggenkemper roggenkemper deleted the roggenkemper/newbrowserfilter branch January 23, 2024 18:15
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.

Update Relay to include new filters
2 participants