feat(inbound-filters): Add inbound filter for ChunkLoadError #57277

iambriccardo · 2023-10-02T11:44:09Z

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

iambriccardo · 2023-10-02T12:09:54Z

src/sentry/relay/config/__init__.py

+    enable_chunk_upload_error_filter = project.get_option("filters:chunk-load-error")
+    if enable_chunk_upload_error_filter:
+        # ChunkLoadError: Loading chunk 3662 failed. (error: https://xxx.com/_next/static/chunks/29107295-0151559bd23117ba.js)
+        error_messages += ["ChunkLoadError: Loading chunk * failed. (error: *)"]


This glob pattern assumes that type and value for an error always exist and that the type is always ChunkLoadError for all the errors of this type (e.g., there is no other type with the same error message).

jjbayer · 2023-10-02T13:18:58Z

src/sentry/relay/config/__init__.py

+    enable_chunk_upload_error_filter = project.get_option("filters:chunk-load-error")
+    if enable_chunk_upload_error_filter:
+        # ChunkLoadError: Loading chunk 3662 failed.\n(error: https://xxx.com/_next/static/chunks/29107295-0151559bd23117ba.js)
+        error_messages += ["ChunkLoadError: Loading chunk * failed.\n(error: *)"]


You may want to check in a unit test in relay to make sure this pattern works. See for example getsentry/relay#1903.

I did write a unit test for it, just wasn't sure if I should make a PR with it. wydt?

iker-barriocanal

Have we verified this works as expected?

iker-barriocanal · 2023-10-02T13:17:43Z

tests/sentry/api/endpoints/test_project_details.py

+        with assume_test_silo_mode(SiloMode.CONTROL):
+            assert AuditLogEntry.objects.filter(
+                organization_id=project.organization_id,
+                event=audit_log.get_event_id("PROJECT_EDIT"),
+            ).exists()


Q: what does this piece do? It seems duplicated for each option.

I admit that I have replicated the test without thinking much about it but I have now removed it since semantically it does not make sense, since no mutation of the project options is happening between the checks.

iker-barriocanal · 2023-10-02T13:28:42Z

tests/sentry/relay/test_config.py

@@ -179,6 +179,7 @@ def test_project_config_uses_filter_features(
    default_project.update_option("sentry:error_messages", error_messages)
    default_project.update_option("sentry:releases", releases)
    default_project.update_option("filters:react-hydration-errors", False)
+    default_project.update_option("filters:chunk-load-error", False)


We should have a test to ensure this value produces a valid project config. If the feature is disabled, it won't be validated with the project config. If the config is invalid we will only see it in production when Relay is unable to parse the project config.

Yep I was going to add it.

Added the test in a follow up commit.

RaduW

Looks OK to me

iambriccardo · 2023-10-02T13:38:52Z

Have we verified this works as expected?

I am going to test locally.

schew2381 · 2023-10-02T18:25:07Z

Hi there, if we plan on eventually adding this inbound filter to the UI, I'd prefer to useProjectFilterDetailsEndpoint PUT over the ProjectDetailsEndpoint PUT to update the project option.
I recently documented the endpoint to update inbound filters here, and unfortunately some existing filters like hydration errors are already tied to the project endpoint (see docs description).

From an API perspective, it's a lot easier for our users if we group all the filter updates to this existing endpoint, and keep that documentation up-to-date. I'd be happy to help with anything as I've been trying to improve our API/documentation for a while now 🤠

iambriccardo · 2023-10-03T05:51:43Z

ProjectFilterDetailsEndpoint

Oh, nice, I didn't know we had this, I followed the implementation I was sent assuming it was the most up-to-date one, but it looks like not. I will fix it now.

codecov · 2023-10-03T13:53:55Z

Codecov Report

Merging #57277 (df9c8b3) into master (c5b84a3) will increase coverage by 1.37%.
Report is 54 commits behind head on master.
The diff coverage is 100.00%.

❗ Current head df9c8b3 differs from pull request most recent head eeb0c9a. Consider uploading reports for the commit eeb0c9a to get more accurate results

@@            Coverage Diff             @@
##           master   #57277      +/-   ##
==========================================
+ Coverage   77.56%   78.93%   +1.37%     
==========================================
  Files        5113     5117       +4     
  Lines      220581   222077    +1496     
  Branches    37346    37758     +412     
==========================================
+ Hits       171094   175306    +4212     
+ Misses      43783    41109    -2674     
+ Partials     5704     5662      -42

Files	Coverage Δ
src/sentry/api/endpoints/project_details.py	`78.43% <100.00%> (+0.08%)`	⬆️
src/sentry/api/serializers/models/project.py	`95.21% <ø> (+6.23%)`	⬆️
src/sentry/apidocs/examples/project_examples.py	`100.00% <ø> (ø)`
src/sentry/models/options/project_option.py	`97.46% <ø> (ø)`
src/sentry/projectoptions/defaults.py	`100.00% <100.00%> (ø)`
src/sentry/relay/config/__init__.py	`92.59% <100.00%> (+0.08%)`	⬆️

... and 266 files with indirect coverage changes

vercel · 2023-10-03T14:07:06Z

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 2:09pm

iambriccardo · 2023-10-03T14:21:35Z

@schew2381 as discussed privately, we will continue with this implementation for now but we have created internally some tickets to track the work needed to make the system more extensible by extending the abstractions you mentioned and enabling proper outcomes generation for sub-filters inside of errorMessages.

The spike ticket is here: #57359

iambriccardo · 2023-10-03T14:23:36Z

For reference, this was the PR that contained the experimentation with the new abstractions: #57343

RaduW

Looks OK,

I personally prefer converting the option to bool ( and working with bool after that) but I guess it is a question of preference.

If you can, have a look at how we deal with this in other places in sentry.

iambriccardo · 2023-10-04T05:46:52Z

Looks OK,

I personally prefer converting the option to bool ( and working with bool after that) but I guess it is a question of preference.

If you can, have a look at how we deal with this in other places in sentry.

I explained in chat the reason for having it as 1 and 0, it's to simplify the porting to the new set of abstractions.

feat(inbound-filters): Add inbound filter for ChunkLoadError

fb811de

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Oct 2, 2023

Fix snapshots

ddb91b9

vercel bot deployed to Preview October 2, 2023 11:49 View deployment

iambriccardo requested review from HazAT, obostjancic and RaduW and removed request for obostjancic October 2, 2023 11:49

iambriccardo marked this pull request as ready for review October 2, 2023 12:03

iambriccardo requested a review from a team as a code owner October 2, 2023 12:03

iambriccardo requested a review from a team October 2, 2023 12:03

iambriccardo commented Oct 2, 2023

View reviewed changes

iambriccardo requested a review from ArthurKnaus October 2, 2023 12:25

iambriccardo mentioned this pull request Oct 2, 2023

feat(inbound-filters): Add UI toggle for ChunkLoadError inbound filter #57278

Merged

iambriccardo added 2 commits October 2, 2023 14:39

fiX

862acfe

fiX

7949be9

vercel bot deployed to Preview October 2, 2023 12:45 View deployment

Fix

773f2d4

vercel bot deployed to Preview October 2, 2023 13:14 View deployment

jjbayer approved these changes Oct 2, 2023

View reviewed changes

iker-barriocanal reviewed Oct 2, 2023

View reviewed changes

RaduW approved these changes Oct 2, 2023

View reviewed changes

Add tests

df9c8b3

vercel bot deployed to Preview October 2, 2023 14:06 View deployment

iker-barriocanal approved these changes Oct 3, 2023

View reviewed changes

iambriccardo changed the title ~~feat(inbound-filters): Add inbound filter for ChunkLoadError~~ feat(inbound-filters): Add inbound filter for ChunkLoadError (OLD) Oct 3, 2023

iambriccardo closed this Oct 3, 2023

iambriccardo reopened this Oct 3, 2023

iambriccardo changed the title ~~feat(inbound-filters): Add inbound filter for ChunkLoadError (OLD)~~ feat(inbound-filters): Add inbound filter for ChunkLoadError Oct 3, 2023

Use 0 and 1 instead of bools

eeb0c9a

vercel bot deployed to Preview October 3, 2023 14:09 View deployment

iambriccardo removed request for HazAT, a team and ArthurKnaus October 3, 2023 14:27

RaduW approved these changes Oct 3, 2023

View reviewed changes

iambriccardo merged commit 5f4e2a1 into master Oct 4, 2023
49 of 50 checks passed

iambriccardo deleted the riccardo/feat/chunk-load-error-ibf branch October 4, 2023 05:45

github-actions bot locked and limited conversation to collaborators Oct 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(inbound-filters): Add inbound filter for ChunkLoadError #57277

feat(inbound-filters): Add inbound filter for ChunkLoadError #57277

iambriccardo commented Oct 2, 2023 •

edited

Loading

iambriccardo Oct 2, 2023 •

edited

Loading

jjbayer Oct 2, 2023

iambriccardo Oct 2, 2023

iker-barriocanal left a comment

iker-barriocanal Oct 2, 2023

iambriccardo Oct 2, 2023

iker-barriocanal Oct 2, 2023

iambriccardo Oct 2, 2023

iambriccardo Oct 2, 2023

RaduW left a comment •

edited

Loading

iambriccardo commented Oct 2, 2023

schew2381 commented Oct 2, 2023 •

edited

Loading

iambriccardo commented Oct 3, 2023

codecov bot commented Oct 3, 2023 •

edited

Loading

vercel bot commented Oct 3, 2023 •

edited

Loading

iambriccardo commented Oct 3, 2023 •

edited

Loading

iambriccardo commented Oct 3, 2023

RaduW left a comment

iambriccardo commented Oct 4, 2023

feat(inbound-filters): Add inbound filter for ChunkLoadError #57277

feat(inbound-filters): Add inbound filter for ChunkLoadError #57277

Conversation

iambriccardo commented Oct 2, 2023 • edited Loading

iambriccardo Oct 2, 2023 • edited Loading

Choose a reason for hiding this comment

jjbayer Oct 2, 2023

Choose a reason for hiding this comment

iambriccardo Oct 2, 2023

Choose a reason for hiding this comment

iker-barriocanal left a comment

Choose a reason for hiding this comment

iker-barriocanal Oct 2, 2023

Choose a reason for hiding this comment

iambriccardo Oct 2, 2023

Choose a reason for hiding this comment

iker-barriocanal Oct 2, 2023

Choose a reason for hiding this comment

iambriccardo Oct 2, 2023

Choose a reason for hiding this comment

iambriccardo Oct 2, 2023

Choose a reason for hiding this comment

RaduW left a comment • edited Loading

Choose a reason for hiding this comment

iambriccardo commented Oct 2, 2023

schew2381 commented Oct 2, 2023 • edited Loading

iambriccardo commented Oct 3, 2023

codecov bot commented Oct 3, 2023 • edited Loading

Codecov Report

vercel bot commented Oct 3, 2023 • edited Loading

iambriccardo commented Oct 3, 2023 • edited Loading

iambriccardo commented Oct 3, 2023

RaduW left a comment

Choose a reason for hiding this comment

iambriccardo commented Oct 4, 2023

iambriccardo commented Oct 2, 2023 •

edited

Loading

iambriccardo Oct 2, 2023 •

edited

Loading

RaduW left a comment •

edited

Loading

schew2381 commented Oct 2, 2023 •

edited

Loading

codecov bot commented Oct 3, 2023 •

edited

Loading

vercel bot commented Oct 3, 2023 •

edited

Loading

iambriccardo commented Oct 3, 2023 •

edited

Loading