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
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/sentry/api/endpoints/project_filter_details.py
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ def put(self, request: Request, project, filter_id) -> Response:
FilterStatKeys.LOCALHOST,
FilterStatKeys.WEB_CRAWLER,
FilterStatKeys.HEALTH_CHECK,
FilterStatKeys.CHUNK_LOAD_ERROR,
):
returned_state = filter_id
removed = current_state - new_state
Expand Down
11 changes: 11 additions & 0 deletions src/sentry/ingest/inbound_filters.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ class FilterStatKeys:
DISCARDED_HASH = "discarded-hash" # Not replicated in Relay
CRASH_REPORT_LIMIT = "crash-report-limit" # Not replicated in Relay
HEALTH_CHECK = "filtered-transaction" # Ignore health-check transactions
CHUNK_LOAD_ERROR = "chunk-load-error" # Ignore NextJS ChunkLoadError


FILTER_STAT_KEYS_TO_VALUES = {
Expand All @@ -38,6 +39,7 @@ class FilterStatKeys:
FilterStatKeys.CORS: TSDBModel.project_total_received_cors,
FilterStatKeys.DISCARDED_HASH: TSDBModel.project_total_received_discarded,
FilterStatKeys.HEALTH_CHECK: TSDBModel.project_total_healthcheck,
FilterStatKeys.CHUNK_LOAD_ERROR: TSDBModel.project_total_chunk_load_error,
}


Expand Down Expand Up @@ -65,6 +67,7 @@ def get_all_filter_specs():
_legacy_browsers_filter,
_web_crawlers_filter,
_healthcheck_filter,
_chunk_load_error_filter,
]

return tuple(filters) # returning tuple for backwards compatibility
Expand Down Expand Up @@ -270,3 +273,11 @@ class _LegacyBrowserFilterSerializer(_FilterSerializer):
serializer_cls=None,
config_name="ignoreTransactions",
)

_chunk_load_error_filter = _FilterSpec(
id=FilterStatKeys.CHUNK_LOAD_ERROR,
name="Filter out ChunkLoadError(s)",
description="It can happen that in full automatic deploy environments like Next.js & Vercel the frontend gets out "
"of sync with the backend which results in a ChunkLoadError. The application refreshes and everything "
"should work as expected.",
)
3 changes: 3 additions & 0 deletions src/sentry/projectoptions/defaults.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,9 @@
# Default react hydration errors filter
register(key="filters:react-hydration-errors", epoch_defaults={1: "1"})

# Default NextJS ChunkLoadError filter
register(key="filters:chunk-load-error", epoch_defaults={1: "1"})

# Default breakdowns config
register(
key="sentry:breakdowns",
Expand Down
16 changes: 14 additions & 2 deletions src/sentry/relay/config/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -120,21 +120,28 @@ def get_public_key_configs(
def get_filter_settings(project: Project) -> Mapping[str, Any]:
filter_settings = {}

error_messages: List[str] = []
for flt in get_all_filter_specs():
filter_id = get_filter_key(flt)
settings = _load_filter_settings(flt, project)

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
Comment on lines 128 to +134
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.


error_messages: List[str] = []
if features.has("projects:custom-inbound-filters", project):
invalid_releases = project.get_option(f"sentry:{FilterTypes.RELEASES}")
if invalid_releases:
filter_settings["releases"] = {"releases": invalid_releases}

error_messages += project.get_option(f"sentry:{FilterTypes.ERROR_MESSAGES}") or []

# TODO(iambriccardo): Move this project option to the new declarative mechanism and make sure to convert its
# boolean value which is set in the endpoint.
enable_react = project.get_option("filters:react-hydration-errors")
if enable_react:
# 418 - Hydration failed because the initial UI does not match what was rendered on the server.
Expand All @@ -146,6 +153,7 @@ def get_filter_settings(project: Project) -> Mapping[str, Any]:
"*https://reactjs.org/docs/error-decoder.html?invariant={418,419,422,423,425}*"
]

# This is where the actual error messages settings are generated.
if error_messages:
filter_settings["errorMessages"] = {"patterns": error_messages}

Expand Down Expand Up @@ -575,6 +583,10 @@ def _filter_option_to_config_setting(flt: _FilterSpec, setting: str) -> Mapping[
ret_val = {"patterns": HEALTH_CHECK_GLOBS, "isEnabled": True}
else:
ret_val = {"patterns": [], "isEnabled": False}
elif flt.id == FilterStatKeys.CHUNK_LOAD_ERROR:
if is_enabled:
ret_val["errorMessages"] = ["ChunkLoadError: Loading chunk * failed.\n(error: *)"]

return ret_val


Expand Down
1 change: 1 addition & 0 deletions src/sentry/snuba/referrer.py
Original file line number Diff line number Diff line change
Expand Up @@ -654,6 +654,7 @@ class Referrer(Enum):
TSDB_MODELID_609 = "tsdb-modelid:609"
TSDB_MODELID_610 = "tsdb-modelid:610"
TSDB_MODELID_611 = "tsdb-modelid:611"
TSDB_MODELID_612 = "tsdb-modelid:612"
TSDB_MODELID_700 = "tsdb-modelid:700"
TSDB_MODELID_800 = "tsdb-modelid:800"
TSDB_MODELID_801 = "tsdb-modelid:801"
Expand Down
2 changes: 2 additions & 0 deletions src/sentry/tsdb/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,8 @@ class TSDBModel(Enum):
project_total_received_discarded = 610
# the number of events filtered because they refer to a healthcheck endpoint
project_total_healthcheck = 611
# the number of events filtered because the error is a ChunkLoadError
project_total_chunk_load_error = 612

servicehook_fired = 700

Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
---
created: '2023-10-03T06:55:40.049770Z'
creator: sentry
source: tests/sentry/api/endpoints/test_project_filters.py
---
- active: false
id: browser-extensions
- active: true
id: chunk-load-error
- active: true
id: filtered-transaction
- active: false
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ source: tests/sentry/api/endpoints/test_project_filters.py
---
- active: false
id: browser-extensions
- active: true
id: chunk-load-error
- active: true
id: filtered-transaction
- active: false
Expand Down
22 changes: 22 additions & 0 deletions tests/sentry/api/endpoints/test_project_filter_details.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,28 @@ def test_put_health_check_filter(self):
# option was changed by the request
assert project.get_option("filters:filtered-transaction") == "0"

def test_put_chunk_load_error_filter(self):
"""
Tests that it accepts to set the ChunkLoadError filter.
"""
org = self.create_organization(name="baz", slug="1", owner=self.user)
team = self.create_team(organization=org, name="foo", slug="foo")
project = self.create_project(name="Bar", slug="bar", teams=[team])

project.update_option("filters:chunk-load-error", "0")
self.get_success_response(
org.slug, project.slug, "chunk-load-error", active=True, status_code=204
)
# option was changed by the request
assert project.get_option("filters:chunk-load-error") == "1"

project.update_option("filters:chunk-load-error", "1")
Comment on lines +60 to +63
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.

self.get_success_response(
org.slug, project.slug, "chunk-load-error", active=False, status_code=204
)
# option was changed by the request
assert project.get_option("filters:chunk-load-error") == "0"

def test_put_legacy_browsers(self):
org = self.create_organization(name="baz", slug="1", owner=self.user)
team = self.create_team(organization=org, name="foo", slug="foo")
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
---
created: '2023-09-15T07:41:54.167449Z'
created: '2023-10-03T07:37:36.283989Z'
creator: sentry
source: tests/sentry/relay/test_config.py
---
Expand Down Expand Up @@ -82,6 +82,9 @@ config:
- promfflinkdev.com
errorMessages:
patterns:
- 'ChunkLoadError: Loading chunk * failed.

(error: *)'
- '*https://reactjs.org/docs/error-decoder.html?invariant={418,419,422,423,425}*'
ignoreTransactions:
isEnabled: true
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
---
created: '2023-09-15T07:41:54.637425Z'
created: '2023-10-03T07:37:36.532678Z'
creator: sentry
source: tests/sentry/relay/test_config.py
---
Expand Down Expand Up @@ -82,6 +82,9 @@ config:
- promfflinkdev.com
errorMessages:
patterns:
- 'ChunkLoadError: Loading chunk * failed.

(error: *)'
- '*https://reactjs.org/docs/error-decoder.html?invariant={418,419,422,423,425}*'
ignoreTransactions:
isEnabled: true
Expand Down
18 changes: 18 additions & 0 deletions tests/sentry/relay/test_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -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", "0")

if has_blacklisted_ips:
default_project.update_option("sentry:blacklisted_ips", blacklisted_ips)
Expand Down Expand Up @@ -639,6 +640,23 @@ def test_project_config_get_at_path(default_project):
assert project_cfg.get_at_path() == project_cfg


@django_db_all
@region_silo_test(stable=True)
def test_project_config_with_chunk_load_error_filter(default_project):
# The react-hydration-errors option is set as string in the defaults but then its changed as a boolean in the
# options endpoint, which is something that should be changed.
default_project.update_option("filters:react-hydration-errors", False)
default_project.update_option("filters:chunk-load-error", "1")

project_cfg = get_project_config(default_project, full_config=True)

cfg = project_cfg.to_dict()
_validate_project_config(cfg["config"])
cfg_error_messages = get_path(cfg, "config", "filterSettings", "errorMessages")

assert len(cfg_error_messages) == 1


@django_db_all
@pytest.mark.parametrize(
"health_check_set",
Expand Down
Loading