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): Add inbound filter for ChunkLoadError (OLD) #57343

Closed
wants to merge 6 commits into from

Conversation

iambriccardo
Copy link
Member

@iambriccardo iambriccardo commented Oct 3, 2023

This PR implements a new inbound filter for ChunkLoadErrors in the form:

ChunkLoadError: Loading chunk 3662 failed. (error: https://xxx.com/_next/static/chunks/29107295-0151559bd23117ba.js)

Such inbound filter idea emerged in vercel/next.js#38507.

Closes #57131

@vercel
Copy link

vercel bot commented Oct 3, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
sentry ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 3, 2023 11:45am

@codecov
Copy link

codecov bot commented Oct 3, 2023

Codecov Report

Merging #57343 (87a940d) into master (2500a36) will decrease coverage by 0.01%.
Report is 2 commits behind head on master.
The diff coverage is 100.00%.

❗ Current head 87a940d differs from pull request most recent head 7f35284. Consider uploading reports for the commit 7f35284 to get more accurate results

@@            Coverage Diff             @@
##           master   #57343      +/-   ##
==========================================
- Coverage   78.92%   78.92%   -0.01%     
==========================================
  Files        5116     5116              
  Lines      220751   220761      +10     
  Branches    37377    37380       +3     
==========================================
+ Hits       174225   174228       +3     
- Misses      40895    40905      +10     
+ Partials     5631     5628       -3     
Files Coverage Δ
src/sentry/api/endpoints/project_filter_details.py 71.42% <ø> (ø)
src/sentry/ingest/inbound_filters.py 77.65% <100.00%> (+0.48%) ⬆️
src/sentry/projectoptions/defaults.py 100.00% <100.00%> (ø)
src/sentry/relay/config/__init__.py 92.64% <100.00%> (+0.13%) ⬆️
src/sentry/snuba/referrer.py 100.00% <100.00%> (ø)
src/sentry/tsdb/base.py 90.90% <100.00%> (+0.05%) ⬆️

... and 7 files with indirect coverage changes

@iambriccardo iambriccardo requested a review from RaduW October 3, 2023 09:41
@iambriccardo iambriccardo marked this pull request as ready for review October 3, 2023 09:45
@iambriccardo iambriccardo requested a review from a team October 3, 2023 09:45
Copy link
Contributor

@RaduW RaduW left a comment

Choose a reason for hiding this comment

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

Every thing looks good except for my question about error_messages

Let's figure this out and then it is good to go.

Comment on lines 128 to +134
if settings is not None and settings.get("isEnabled", True):
filter_settings[filter_id] = settings
# In order to support the new declarative mechanism for inbound filters also for error messages filters
# we have to handle differently the settings, since each filter will still bind to the same key.
if (messages := settings.get("errorMessages")) is not None:
error_messages += messages
else:
filter_settings[filter_id] = settings
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's discuss about this.
Would be nice if we could attribute which error message comes from which filter ( on Relay side)

Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly, my implementation is not the best since it's hacky, I would prefer to design from first principles an integration of inbound filters that map to error messages or to top-level filters.

Comment on lines +60 to +63
# option was changed by the request
assert project.get_option("filters:chunk-load-error") == "1"

project.update_option("filters:chunk-load-error", "1")
Copy link
Contributor

Choose a reason for hiding this comment

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

If the option is set to a value, do we need to update it to the same value?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was just to clearly show that we set it to 1 and not leverage the state left by the previous execution but yeah, that could be removed.

@iambriccardo iambriccardo changed the title feat(inbound-filters): Add inbound filter for ChunkLoadError feat(inbound-filters): Add inbound filter for ChunkLoadError (OLD) Oct 3, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Oct 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inbound Filter: Chunk Load Error
3 participants