From 29087c6cfc6f69d82d41f5222ecc115e2872e003 Mon Sep 17 00:00:00 2001 From: Jodi Jang Date: Wed, 29 Nov 2023 15:34:30 -0800 Subject: [PATCH 1/4] ref(grouping): Add ChunkLoadError feature flag --- src/sentry/conf/server.py | 2 ++ src/sentry/features/__init__.py | 1 + 2 files changed, 3 insertions(+) diff --git a/src/sentry/conf/server.py b/src/sentry/conf/server.py index f623e3b61e20d5..152840eb523048 100644 --- a/src/sentry/conf/server.py +++ b/src/sentry/conf/server.py @@ -1537,6 +1537,8 @@ def custom_parameter_sort(parameter: dict) -> tuple[str, int]: "organizations:gitlab-disable-on-broken": False, # Enable multi project selection "organizations:global-views": False, + # Enable grouping of ChunkLoadErrors + "organizations:group-chunk-load-errors": False, # Enable experimental new version of Merged Issues where sub-hashes are shown "organizations:grouping-tree-ui": False, # Enable experimental new version of stacktrace component where additional diff --git a/src/sentry/features/__init__.py b/src/sentry/features/__init__.py index 964f4a6947ba9c..af6595d53a90f7 100644 --- a/src/sentry/features/__init__.py +++ b/src/sentry/features/__init__.py @@ -81,6 +81,7 @@ default_manager.add("organizations:discover", OrganizationFeature, FeatureHandlerStrategy.INTERNAL) default_manager.add("organizations:discover-events-rate-limit", OrganizationFeature, FeatureHandlerStrategy.REMOTE) default_manager.add("organizations:enterprise-data-secrecy", OrganizationFeature, FeatureHandlerStrategy.INTERNAL) +default_manager.add("organizations:group-chunk-load-errors", OrganizationFeature, FeatureHandlerStrategy.INTERNAL) default_manager.add("organizations:grouping-stacktrace-ui", OrganizationFeature, FeatureHandlerStrategy.REMOTE) default_manager.add("organizations:grouping-title-ui", OrganizationFeature, FeatureHandlerStrategy.REMOTE) default_manager.add("organizations:grouping-tree-ui", OrganizationFeature, FeatureHandlerStrategy.REMOTE) From b6e60d42f778fe44d51031b60229cde60d1d5fbe Mon Sep 17 00:00:00 2001 From: Jodi Jang Date: Thu, 30 Nov 2023 08:58:44 -0800 Subject: [PATCH 2/4] feat(grouping): Group chunk load errors --- src/sentry/event_manager.py | 74 +++++++--- .../test_event_manager_grouping.py | 134 ++++++++++++++++++ 2 files changed, 187 insertions(+), 21 deletions(-) diff --git a/src/sentry/event_manager.py b/src/sentry/event_manager.py index 2ea3ee07eccae8..69e0effdad050d 100644 --- a/src/sentry/event_manager.py +++ b/src/sentry/event_manager.py @@ -1,6 +1,7 @@ from __future__ import annotations import copy +import hashlib import ipaddress import logging import mimetypes @@ -2561,28 +2562,34 @@ def _calculate_event_grouping( # Detect & set synthetic marker if necessary detect_synthetic_exception(event.data, grouping_config) - with metrics.timer("event_manager.apply_server_fingerprinting", tags=metric_tags): - # The active grouping config was put into the event in the - # normalize step before. We now also make sure that the - # fingerprint was set to `'{{ default }}' just in case someone - # removed it from the payload. The call to get_hashes will then - # look at `grouping_config` to pick the right parameters. - event.data["fingerprint"] = event.data.data.get("fingerprint") or ["{{ default }}"] - apply_server_fingerprinting( - event.data.data, - get_fingerprinting_config_for_project(project), - allow_custom_title=True, - ) + hashes = None + # If the event platform is node, it could have a ChunkLoadError that we want to force group + if event.platform == "node": + hashes = handle_chunk_load_error_hash(event) + + if not hashes: + with metrics.timer("event_manager.apply_server_fingerprinting", tags=metric_tags): + # The active grouping config was put into the event in the + # normalize step before. We now also make sure that the + # fingerprint was set to `'{{ default }}' just in case someone + # removed it from the payload. The call to get_hashes will then + # look at `grouping_config` to pick the right parameters. + event.data["fingerprint"] = event.data.data.get("fingerprint") or ["{{ default }}"] + apply_server_fingerprinting( + event.data.data, + get_fingerprinting_config_for_project(project), + allow_custom_title=True, + ) - with metrics.timer("event_manager.event.get_hashes", tags=metric_tags): - # Here we try to use the grouping config that was requested in the - # event. If that config has since been deleted (because it was an - # experimental grouping config) we fall back to the default. - try: - hashes = event.get_hashes(grouping_config) - except GroupingConfigNotFound: - event.data["grouping_config"] = get_grouping_config_dict_for_project(project) - hashes = event.get_hashes() + with metrics.timer("event_manager.event.get_hashes", tags=metric_tags): + # Here we try to use the grouping config that was requested in the + # event. If that config has since been deleted (because it was an + # experimental grouping config) we fall back to the default. + try: + hashes = event.get_hashes(grouping_config) + except GroupingConfigNotFound: + event.data["grouping_config"] = get_grouping_config_dict_for_project(project) + hashes = event.get_hashes() hashes.write_to_event(event.data) return hashes @@ -2621,6 +2628,31 @@ def _detect_performance_problems(jobs: Sequence[Job], projects: ProjectsMapping) ) +def handle_chunk_load_error_hash(event: Event) -> Optional[CalculatedHashes]: + """ + Return the same hash if the event is a ChunkLoadError, otherwise return None + """ + try: + exception_value = event.data["exception"]["values"][0]["value"] + except KeyError: + return None + + hashes = None + # Only check for the flag after it is established if it's a ChunkLoadError to avoid + # unnecessary querying + if "ChunkLoadError" in exception_value: + organization = Project.objects.get(id=event.project_id).organization + if features.has("organizations:group-chunk-load-errors", organization): + hashes = get_chunk_load_error_hash() + return hashes + + +def get_chunk_load_error_hash() -> CalculatedHashes: + return CalculatedHashes( + hashes=hashlib.md5(b"chunkloaderror").hexdigest(), hierarchical_hashes=[], tree_labels=[] + ) + + class PerformanceJob(TypedDict, total=False): performance_problems: Sequence[PerformanceProblem] event: Event diff --git a/tests/sentry/event_manager/test_event_manager_grouping.py b/tests/sentry/event_manager/test_event_manager_grouping.py index 1d0f5c74a6d701..4d457814480c70 100644 --- a/tests/sentry/event_manager/test_event_manager_grouping.py +++ b/tests/sentry/event_manager/test_event_manager_grouping.py @@ -9,6 +9,7 @@ from sentry.models.grouphash import GroupHash from sentry.testutils.cases import TestCase from sentry.testutils.helpers.datetime import before_now, freeze_time, iso_format +from sentry.testutils.helpers.features import with_feature from sentry.testutils.silo import region_silo_test from sentry.testutils.skips import requires_snuba from sentry.tsdb.base import TSDBModel @@ -342,3 +343,136 @@ def test_ignores_fingerprint_on_transaction_event(self): )[event1.group.id] == 1 ) + + @with_feature("organizations:group-chunk-load-errors") + def test_group_chunk_load_errors(self): + manager = EventManager( + make_event( + title="something", + message="ChunkLoadError: Loading chunk 123 failed", + event_id="a" * 32, + platform="node", + exception={ + "values": [ + { + "type": "Error", + "stacktrace": { + "frames": [ + { + "function": "in_app_function", + } + ] + }, + "value": "ChunkLoadError: Loading chunk 123 failed", + } + ] + }, + ) + ) + with self.tasks(): + manager.normalize() + event = manager.save(self.project.id) + + manager = EventManager( + make_event( + title="else", + message="ChunkLoadError: Loading chunk 321 failed", + event_id="b" * 32, + platform="node", + exception={ + "values": [ + { + "type": "Error", + "stacktrace": { + "frames": [ + { + "function": "different_in_app_function", + } + ] + }, + "value": "ChunkLoadError: Loading chunk 321 failed", + } + ] + }, + ) + ) + + with self.tasks(): + manager.normalize() + event2 = manager.save(self.project.id) + + assert event.group_id == event2.group_id + + def test_do_not_group_chunk_load_errors(self): + manager = EventManager( + make_event( + title="something", + message="ChunkLoadError: Loading chunk 123 failed", + event_id="a" * 32, + platform="node", + exception={ + "values": [ + { + "type": "Error", + "stacktrace": { + "frames": [ + { + "function": "in_app_function", + } + ] + }, + "value": "ChunkLoadError: Loading chunk 123 failed", + } + ] + }, + ) + ) + with self.tasks(): + manager.normalize() + event = manager.save(self.project.id) + + manager = EventManager( + make_event( + title="else", + message="ChunkLoadError: Loading chunk 321 failed", + event_id="b" * 32, + platform="node", + exception={ + "values": [ + { + "type": "Error", + "stacktrace": { + "frames": [ + { + "function": "different_in_app_function", + } + ] + }, + "value": "ChunkLoadError: Loading chunk 321 failed", + } + ] + }, + ) + ) + + with self.tasks(): + manager.normalize() + event2 = manager.save(self.project.id) + + assert event.group_id != event2.group_id + + def test_chunk_load_errors_exception(self): + manager = EventManager( + make_event( + title="something", + message="", + event_id="a" * 32, + platform="node", + exception={}, + ) + ) + with self.tasks(): + manager.normalize() + event = manager.save(self.project.id) + + assert event.group From d3e8e09ecd1b2e31eff3a3de26147f7a9e4e6cd7 Mon Sep 17 00:00:00 2001 From: Jodi Jang Date: Thu, 7 Dec 2023 09:54:47 -0800 Subject: [PATCH 3/4] ref: Respond to feedback --- src/sentry/event_manager.py | 66 +++++++++---------- .../test_event_manager_grouping.py | 46 ++++++------- 2 files changed, 56 insertions(+), 56 deletions(-) diff --git a/src/sentry/event_manager.py b/src/sentry/event_manager.py index 373148723b5781..9f8a851819daf9 100644 --- a/src/sentry/event_manager.py +++ b/src/sentry/event_manager.py @@ -2534,34 +2534,34 @@ def _calculate_event_grouping( # Detect & set synthetic marker if necessary detect_synthetic_exception(event.data, grouping_config) - hashes = None - # If the event platform is node, it could have a ChunkLoadError that we want to force group - if event.platform == "node": - hashes = handle_chunk_load_error_hash(event) - - if not hashes: - with metrics.timer("event_manager.apply_server_fingerprinting", tags=metric_tags): - # The active grouping config was put into the event in the - # normalize step before. We now also make sure that the - # fingerprint was set to `'{{ default }}' just in case someone - # removed it from the payload. The call to get_hashes will then - # look at `grouping_config` to pick the right parameters. - event.data["fingerprint"] = event.data.data.get("fingerprint") or ["{{ default }}"] - apply_server_fingerprinting( - event.data.data, - get_fingerprinting_config_for_project(project), - allow_custom_title=True, - ) + with metrics.timer("event_manager.apply_server_fingerprinting", tags=metric_tags): + # The active grouping config was put into the event in the + # normalize step before. We now also make sure that the + # fingerprint was set to `'{{ default }}' just in case someone + # removed it from the payload. The call to get_hashes will then + # look at `grouping_config` to pick the right parameters. + event.data["fingerprint"] = event.data.data.get("fingerprint") or ["{{ default }}"] + apply_server_fingerprinting( + event.data.data, + get_fingerprinting_config_for_project(project), + allow_custom_title=True, + ) - with metrics.timer("event_manager.event.get_hashes", tags=metric_tags): - # Here we try to use the grouping config that was requested in the - # event. If that config has since been deleted (because it was an - # experimental grouping config) we fall back to the default. - try: + with metrics.timer("event_manager.event.get_hashes", tags=metric_tags): + # Here we try to use the grouping config that was requested in the + # event. If that config has since been deleted (because it was an + # experimental grouping config) we fall back to the default. + try: + hashes = None + # If the event platform is javascript, it could have a ChunkLoadError that we want to force group + if event.platform == "javascript": + hashes = get_chunk_load_error_hash(event) + + if not hashes: hashes = event.get_hashes(grouping_config) - except GroupingConfigNotFound: - event.data["grouping_config"] = get_grouping_config_dict_for_project(project) - hashes = event.get_hashes() + except GroupingConfigNotFound: + event.data["grouping_config"] = get_grouping_config_dict_for_project(project) + hashes = event.get_hashes() hashes.write_to_event(event.data) return hashes @@ -2600,7 +2600,7 @@ def _detect_performance_problems(jobs: Sequence[Job], projects: ProjectsMapping) ) -def handle_chunk_load_error_hash(event: Event) -> Optional[CalculatedHashes]: +def get_chunk_load_error_hash(event: Event) -> Optional[CalculatedHashes]: """ Return the same hash if the event is a ChunkLoadError, otherwise return None """ @@ -2615,16 +2615,14 @@ def handle_chunk_load_error_hash(event: Event) -> Optional[CalculatedHashes]: if "ChunkLoadError" in exception_value: organization = Project.objects.get(id=event.project_id).organization if features.has("organizations:group-chunk-load-errors", organization): - hashes = get_chunk_load_error_hash() + hashes = CalculatedHashes( + hashes=[hashlib.md5(b"chunkloaderror").hexdigest()], + hierarchical_hashes=[], + tree_labels=[], + ) return hashes -def get_chunk_load_error_hash() -> CalculatedHashes: - return CalculatedHashes( - hashes=hashlib.md5(b"chunkloaderror").hexdigest(), hierarchical_hashes=[], tree_labels=[] - ) - - class PerformanceJob(TypedDict, total=False): performance_problems: Sequence[PerformanceProblem] event: Event diff --git a/tests/sentry/event_manager/test_event_manager_grouping.py b/tests/sentry/event_manager/test_event_manager_grouping.py index 4d457814480c70..701871b02ac237 100644 --- a/tests/sentry/event_manager/test_event_manager_grouping.py +++ b/tests/sentry/event_manager/test_event_manager_grouping.py @@ -345,13 +345,14 @@ def test_ignores_fingerprint_on_transaction_event(self): ) @with_feature("organizations:group-chunk-load-errors") - def test_group_chunk_load_errors(self): + def test_group_chunk_load_errors_flag_on(self): + """ + Test that when `group-chunk-load-errors` flag is on, ChunkLoadErrors with different stack + traces and values are grouped together + """ manager = EventManager( make_event( - title="something", - message="ChunkLoadError: Loading chunk 123 failed", - event_id="a" * 32, - platform="node", + platform="javascript", exception={ "values": [ { @@ -364,6 +365,7 @@ def test_group_chunk_load_errors(self): ] }, "value": "ChunkLoadError: Loading chunk 123 failed", + "in_app": True, } ] }, @@ -375,10 +377,7 @@ def test_group_chunk_load_errors(self): manager = EventManager( make_event( - title="else", - message="ChunkLoadError: Loading chunk 321 failed", - event_id="b" * 32, - platform="node", + platform="javascript", exception={ "values": [ { @@ -391,6 +390,7 @@ def test_group_chunk_load_errors(self): ] }, "value": "ChunkLoadError: Loading chunk 321 failed", + "in_app": True, } ] }, @@ -403,13 +403,15 @@ def test_group_chunk_load_errors(self): assert event.group_id == event2.group_id - def test_do_not_group_chunk_load_errors(self): + @with_feature({"organizations:group-chunk-load-errors": False}) + def test_do_not_group_chunk_load_errors_flag_off(self): + """ + Test that when `group-chunk-load-errors` flag is off, ChunkLoadErrors with different stack + traces and values are not grouped together + """ manager = EventManager( make_event( - title="something", - message="ChunkLoadError: Loading chunk 123 failed", - event_id="a" * 32, - platform="node", + platform="javascript", exception={ "values": [ { @@ -422,6 +424,7 @@ def test_do_not_group_chunk_load_errors(self): ] }, "value": "ChunkLoadError: Loading chunk 123 failed", + "in_app": True, } ] }, @@ -433,10 +436,7 @@ def test_do_not_group_chunk_load_errors(self): manager = EventManager( make_event( - title="else", - message="ChunkLoadError: Loading chunk 321 failed", - event_id="b" * 32, - platform="node", + platform="javascript", exception={ "values": [ { @@ -449,6 +449,7 @@ def test_do_not_group_chunk_load_errors(self): ] }, "value": "ChunkLoadError: Loading chunk 321 failed", + "in_app": True, } ] }, @@ -462,12 +463,13 @@ def test_do_not_group_chunk_load_errors(self): assert event.group_id != event2.group_id def test_chunk_load_errors_exception(self): + """ + Test that when an error does not have an exception, the `get_chunk_load_error_hash` + function catches the exception and does not stop execution + """ manager = EventManager( make_event( - title="something", - message="", - event_id="a" * 32, - platform="node", + platform="javascript", exception={}, ) ) From 40c5410d792c264d106f2be136a86a32d42cbe13 Mon Sep 17 00:00:00 2001 From: Jodi Jang Date: Mon, 11 Dec 2023 08:39:42 -0800 Subject: [PATCH 4/4] fix: Add error type grouping --- src/sentry/event_manager.py | 3 ++- tests/sentry/event_manager/test_event_manager_grouping.py | 8 ++++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/sentry/event_manager.py b/src/sentry/event_manager.py index 9f8a851819daf9..c72070803de380 100644 --- a/src/sentry/event_manager.py +++ b/src/sentry/event_manager.py @@ -2606,13 +2606,14 @@ def get_chunk_load_error_hash(event: Event) -> Optional[CalculatedHashes]: """ try: exception_value = event.data["exception"]["values"][0]["value"] + exception_type = event.data["exception"]["values"][0]["type"] except KeyError: return None hashes = None # Only check for the flag after it is established if it's a ChunkLoadError to avoid # unnecessary querying - if "ChunkLoadError" in exception_value: + if "ChunkLoadError" in exception_value or exception_type == "ChunkLoadError": organization = Project.objects.get(id=event.project_id).organization if features.has("organizations:group-chunk-load-errors", organization): hashes = CalculatedHashes( diff --git a/tests/sentry/event_manager/test_event_manager_grouping.py b/tests/sentry/event_manager/test_event_manager_grouping.py index 701871b02ac237..e9c4eeff4b9f4f 100644 --- a/tests/sentry/event_manager/test_event_manager_grouping.py +++ b/tests/sentry/event_manager/test_event_manager_grouping.py @@ -381,7 +381,7 @@ def test_group_chunk_load_errors_flag_on(self): exception={ "values": [ { - "type": "Error", + "type": "ChunkLoadError", "stacktrace": { "frames": [ { @@ -389,7 +389,7 @@ def test_group_chunk_load_errors_flag_on(self): } ] }, - "value": "ChunkLoadError: Loading chunk 321 failed", + "value": "Loading chunk 321 failed", "in_app": True, } ] @@ -440,7 +440,7 @@ def test_do_not_group_chunk_load_errors_flag_off(self): exception={ "values": [ { - "type": "Error", + "type": "ChunkLoadError", "stacktrace": { "frames": [ { @@ -448,7 +448,7 @@ def test_do_not_group_chunk_load_errors_flag_off(self): } ] }, - "value": "ChunkLoadError: Loading chunk 321 failed", + "value": "Loading chunk 321 failed", "in_app": True, } ]