From 938bfcfc5dc7056b89f8339c568cffddc10c7f7c Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Mon, 3 Apr 2023 11:30:41 +0200 Subject: [PATCH 01/32] backup --- src/sentry/tasks/post_process.py | 19 +++++++++++++++++++ .../utils/sdk_crashes/sdk_crash_detection.py | 2 +- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/src/sentry/tasks/post_process.py b/src/sentry/tasks/post_process.py index bf5acd7d91f5d4..0e02733b49470d 100644 --- a/src/sentry/tasks/post_process.py +++ b/src/sentry/tasks/post_process.py @@ -23,6 +23,9 @@ from sentry.utils.locking.manager import LockManager from sentry.utils.safe import safe_execute from sentry.utils.sdk import bind_organization_context, set_current_event_project +from sentry.utils.sdk_crashes.cocoa_sdk_crash_detector import CocoaSDKCrashDetector +from sentry.utils.sdk_crashes.event_stripper import EventStripper +from sentry.utils.sdk_crashes.sdk_crash_detection import SDKCrashDetection, SDKCrashReporter from sentry.utils.services import build_instance_from_options if TYPE_CHECKING: @@ -960,6 +963,21 @@ def fire_error_processed(job: PostProcessJob): ) +def sdk_crash_monitoring(job: PostProcessJob): + if job["is_reprocessed"]: + return + + event = job["event"] + + crash_reporter = SDKCrashReporter() + cocoa_sdk_crash_detector = CocoaSDKCrashDetector() + event_stripper = EventStripper(sdk_crash_detector=cocoa_sdk_crash_detector) + + crash_detection = SDKCrashDetection(crash_reporter, cocoa_sdk_crash_detector, event_stripper) + + crash_detection.detect_sdk_crash(event=event) + + def plugin_post_process_group(plugin_slug, event, **kwargs): """ Fires post processing hooks for a group. @@ -995,6 +1013,7 @@ def plugin_post_process_group(plugin_slug, event, **kwargs): process_similarity, update_existing_attachments, fire_error_processed, + sdk_crash_monitoring, ], GroupCategory.PERFORMANCE: [ process_snoozes, diff --git a/src/sentry/utils/sdk_crashes/sdk_crash_detection.py b/src/sentry/utils/sdk_crashes/sdk_crash_detection.py index 9ba37b450edbcf..03e08ab39fb885 100644 --- a/src/sentry/utils/sdk_crashes/sdk_crash_detection.py +++ b/src/sentry/utils/sdk_crashes/sdk_crash_detection.py @@ -11,7 +11,7 @@ def __init__(self): self def report(self, event: Event) -> None: - pass + return class SDKCrashDetection: From bbe8ee410363c549c3e56e46fa5df7f2157355ac Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Tue, 4 Apr 2023 12:34:02 +0200 Subject: [PATCH 02/32] import via local scope --- src/sentry/tasks/post_process.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/sentry/tasks/post_process.py b/src/sentry/tasks/post_process.py index 0e02733b49470d..8c24fedeb2368d 100644 --- a/src/sentry/tasks/post_process.py +++ b/src/sentry/tasks/post_process.py @@ -23,9 +23,6 @@ from sentry.utils.locking.manager import LockManager from sentry.utils.safe import safe_execute from sentry.utils.sdk import bind_organization_context, set_current_event_project -from sentry.utils.sdk_crashes.cocoa_sdk_crash_detector import CocoaSDKCrashDetector -from sentry.utils.sdk_crashes.event_stripper import EventStripper -from sentry.utils.sdk_crashes.sdk_crash_detection import SDKCrashDetection, SDKCrashReporter from sentry.utils.services import build_instance_from_options if TYPE_CHECKING: @@ -964,6 +961,11 @@ def fire_error_processed(job: PostProcessJob): def sdk_crash_monitoring(job: PostProcessJob): + # Importing this at the top of the file doesn't work. + from sentry.utils.sdk_crashes.cocoa_sdk_crash_detector import CocoaSDKCrashDetector + from sentry.utils.sdk_crashes.event_stripper import EventStripper + from sentry.utils.sdk_crashes.sdk_crash_detection import SDKCrashDetection, SDKCrashReporter + if job["is_reprocessed"]: return From 625c04550606ba8a059228dcb15857561db0d11b Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Thu, 6 Apr 2023 11:46:29 +0200 Subject: [PATCH 03/32] add test --- src/sentry/tasks/post_process.py | 14 ++------------ .../utils/sdk_crashes/sdk_crash_detection.py | 8 ++++++++ tests/sentry/tasks/test_post_process.py | 16 ++++++++++++++++ 3 files changed, 26 insertions(+), 12 deletions(-) diff --git a/src/sentry/tasks/post_process.py b/src/sentry/tasks/post_process.py index 8c24fedeb2368d..9a3aa14ad99da4 100644 --- a/src/sentry/tasks/post_process.py +++ b/src/sentry/tasks/post_process.py @@ -961,23 +961,13 @@ def fire_error_processed(job: PostProcessJob): def sdk_crash_monitoring(job: PostProcessJob): - # Importing this at the top of the file doesn't work. - from sentry.utils.sdk_crashes.cocoa_sdk_crash_detector import CocoaSDKCrashDetector - from sentry.utils.sdk_crashes.event_stripper import EventStripper - from sentry.utils.sdk_crashes.sdk_crash_detection import SDKCrashDetection, SDKCrashReporter + from sentry.utils.sdk_crashes.sdk_crash_detection import sdk_crash_detection if job["is_reprocessed"]: return event = job["event"] - - crash_reporter = SDKCrashReporter() - cocoa_sdk_crash_detector = CocoaSDKCrashDetector() - event_stripper = EventStripper(sdk_crash_detector=cocoa_sdk_crash_detector) - - crash_detection = SDKCrashDetection(crash_reporter, cocoa_sdk_crash_detector, event_stripper) - - crash_detection.detect_sdk_crash(event=event) + sdk_crash_detection.detect_sdk_crash(event=event) def plugin_post_process_group(plugin_slug, event, **kwargs): diff --git a/src/sentry/utils/sdk_crashes/sdk_crash_detection.py b/src/sentry/utils/sdk_crashes/sdk_crash_detection.py index 66f5044a468480..6f5f200af21b73 100644 --- a/src/sentry/utils/sdk_crashes/sdk_crash_detection.py +++ b/src/sentry/utils/sdk_crashes/sdk_crash_detection.py @@ -2,6 +2,7 @@ from sentry.eventstore.models import Event from sentry.utils.safe import get_path, set_path +from sentry.utils.sdk_crashes.cocoa_sdk_crash_detector import CocoaSDKCrashDetector from sentry.utils.sdk_crashes.event_stripper import EventStripper from sentry.utils.sdk_crashes.sdk_crash_detector import SDKCrashDetector @@ -47,3 +48,10 @@ def detect_sdk_crash(self, event: Event) -> None: set_path(sdk_crash_event, "contexts", "sdk_crash_detection", value={"detected": True}) self.sdk_crash_reporter.report(sdk_crash_event) + + +_crash_reporter = SDKCrashReporter() +_cocoa_sdk_crash_detector = CocoaSDKCrashDetector() +_event_stripper = EventStripper(sdk_crash_detector=_cocoa_sdk_crash_detector) + +sdk_crash_detection = SDKCrashDetection(_crash_reporter, _cocoa_sdk_crash_detector, _event_stripper) diff --git a/tests/sentry/tasks/test_post_process.py b/tests/sentry/tasks/test_post_process.py index 932143b32c2caf..2730e745377fc1 100644 --- a/tests/sentry/tasks/test_post_process.py +++ b/tests/sentry/tasks/test_post_process.py @@ -1289,6 +1289,21 @@ def test_maintains_valid_snooze(self, mock_processor): assert GroupSnooze.objects.filter(id=snooze.id).exists() +class SDKCrashMonitoringTestMixin(BasePostProgressGroupMixin): + @patch("sentry.utils.sdk_crashes.sdk_crash_detection.sdk_crash_detection") + def test_sdk_crash_monitoring_is_called_with_event(self, mock_sdk_crash_detection): + event = self.create_event(data={}, project_id=self.project.id) + + self.call_post_process_group( + is_new=True, + is_regression=False, + is_new_group_environment=True, + event=event, + ) + + mock_sdk_crash_detection.detect_sdk_crash.assert_called_once() + + @region_silo_test class PostProcessGroupErrorTest( TestCase, @@ -1301,6 +1316,7 @@ class PostProcessGroupErrorTest( RuleProcessorTestMixin, ServiceHooksTestMixin, SnoozeTestMixin, + SDKCrashMonitoringTestMixin, ): def create_event(self, data, project_id, assert_no_errors=True): return self.store_event(data=data, project_id=project_id, assert_no_errors=assert_no_errors) From 7a8115e80f6340c88218d42820de2eb8546f7431 Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Thu, 6 Apr 2023 11:48:46 +0200 Subject: [PATCH 04/32] todo note --- src/sentry/tasks/post_process.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/sentry/tasks/post_process.py b/src/sentry/tasks/post_process.py index 9a3aa14ad99da4..8abae914ce98a7 100644 --- a/src/sentry/tasks/post_process.py +++ b/src/sentry/tasks/post_process.py @@ -967,6 +967,7 @@ def sdk_crash_monitoring(job: PostProcessJob): return event = job["event"] + # TODO: ensure event is the proper event as expected by the sdk_crash_detection sdk_crash_detection.detect_sdk_crash(event=event) From e1ee353a23ea3293f585baf16e2431b59d79f7f3 Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Thu, 11 May 2023 11:03:53 +0200 Subject: [PATCH 05/32] refactor tests to use silos --- src/sentry/tasks/post_process.py | 6 +- .../utils/sdk_crashes/event_stripper.py | 2 +- .../utils/sdk_crashes/sdk_crash_detection.py | 17 ++++- tests/sentry/tasks/test_post_process.py | 15 +++- .../sentry/utils/sdk_crashes/test_fixture.py | 73 +++++++++---------- .../sdk_crashes/test_sdk_crash_detection.py | 50 +++++++++++-- 6 files changed, 108 insertions(+), 55 deletions(-) diff --git a/src/sentry/tasks/post_process.py b/src/sentry/tasks/post_process.py index 502d51684c58a0..c568355dc1227a 100644 --- a/src/sentry/tasks/post_process.py +++ b/src/sentry/tasks/post_process.py @@ -988,8 +988,10 @@ def sdk_crash_monitoring(job: PostProcessJob): return event = job["event"] - # TODO: ensure event is the proper event as expected by the sdk_crash_detection - sdk_crash_detection.detect_sdk_crash(event=event) + + with metrics.timer("post_process.sdk_crash_monitoring.duration"): + with sentry_sdk.start_span(op="tasks.post_process_group.sdk_crash_monitoring"): + sdk_crash_detection.detect_sdk_crash(event=event) def plugin_post_process_group(plugin_slug, event, **kwargs): diff --git a/src/sentry/utils/sdk_crashes/event_stripper.py b/src/sentry/utils/sdk_crashes/event_stripper.py index 4f137d91777009..4bd19eb46b724e 100644 --- a/src/sentry/utils/sdk_crashes/event_stripper.py +++ b/src/sentry/utils/sdk_crashes/event_stripper.py @@ -27,7 +27,7 @@ def __init__( } def strip_event_data(self, event: Event) -> Event: - new_event = dict(filter(self._filter_event, event.items())) + new_event = dict(filter(self._filter_event, event.data.items())) new_event["contexts"] = dict(filter(self._filter_contexts, new_event["contexts"].items())) stripped_frames = [] diff --git a/src/sentry/utils/sdk_crashes/sdk_crash_detection.py b/src/sentry/utils/sdk_crashes/sdk_crash_detection.py index 6f5f200af21b73..6e1610fb7a2645 100644 --- a/src/sentry/utils/sdk_crashes/sdk_crash_detection.py +++ b/src/sentry/utils/sdk_crashes/sdk_crash_detection.py @@ -1,6 +1,7 @@ from __future__ import annotations from sentry.eventstore.models import Event +from sentry.issues.grouptype import GroupCategory from sentry.utils.safe import get_path, set_path from sentry.utils.sdk_crashes.cocoa_sdk_crash_detector import CocoaSDKCrashDetector from sentry.utils.sdk_crashes.event_stripper import EventStripper @@ -28,18 +29,26 @@ def __init__( self.event_stripper = event_stripper def detect_sdk_crash(self, event: Event) -> None: - if event.get("type", None) != "error" or event.get("platform") != "cocoa": + + should_detect_sdk_crash = ( + event.group + and event.group.issue_category == GroupCategory.ERROR + and event.group.platform == "cocoa" + ) + if should_detect_sdk_crash is False: return - context = get_path(event, "contexts", "sdk_crash_detection") + context = get_path(event.data, "contexts", "sdk_crash_detection") if context is not None and context.get("detected", False): return - is_unhandled = get_path(event, "exception", "values", -1, "mechanism", "handled") is False + is_unhandled = ( + get_path(event.data, "exception", "values", -1, "mechanism", "data", "handled") is False + ) if is_unhandled is False: return - frames = get_path(event, "exception", "values", -1, "stacktrace", "frames") + frames = get_path(event.data, "exception", "values", -1, "stacktrace", "frames") if not frames: return diff --git a/tests/sentry/tasks/test_post_process.py b/tests/sentry/tasks/test_post_process.py index fe3e7d2b9f665f..05858a04deea3b 100644 --- a/tests/sentry/tasks/test_post_process.py +++ b/tests/sentry/tasks/test_post_process.py @@ -1437,9 +1437,16 @@ def test_maintains_valid_snooze(self, mock_processor): class SDKCrashMonitoringTestMixin(BasePostProgressGroupMixin): - @patch("sentry.utils.sdk_crashes.sdk_crash_detection.sdk_crash_detection") - def test_sdk_crash_monitoring_is_called_with_event(self, mock_sdk_crash_detection): - event = self.create_event(data={}, project_id=self.project.id) + # @patch("sentry.utils.sdk_crashes.sdk_crash_detection.sdk_crash_detection") + def test_sdk_crash_monitoring_is_called_with_event(self): + event = self.create_event( + data={ + "message": "oh no", + "platform": "cocoa", + "stacktrace": {"frames": [{"filename": "src/app/example.py"}]}, + }, + project_id=self.project.id, + ) self.call_post_process_group( is_new=True, @@ -1448,7 +1455,7 @@ def test_sdk_crash_monitoring_is_called_with_event(self, mock_sdk_crash_detectio event=event, ) - mock_sdk_crash_detection.detect_sdk_crash.assert_called_once() + # mock_sdk_crash_detection.detect_sdk_crash.assert_called_once() @region_silo_test diff --git a/tests/sentry/utils/sdk_crashes/test_fixture.py b/tests/sentry/utils/sdk_crashes/test_fixture.py index edfbb21e1eba50..4ebe079aa2b392 100644 --- a/tests/sentry/utils/sdk_crashes/test_fixture.py +++ b/tests/sentry/utils/sdk_crashes/test_fixture.py @@ -82,14 +82,11 @@ def get_crash_event_with_frames( ) -> Sequence[Mapping[str, Any]]: result = { "event_id": "80e3496eff734ab0ac993167aaa0d1cd", - "project": 5218188, "release": "5.222.5", "type": "error", "level": "fatal", "platform": "cocoa", "tags": {"level": "fatal"}, - "datetime": "2023-02-08T23:51:35.000000Z", - "timestamp": 1675936223.0, "exception": { "values": [ { @@ -157,41 +154,41 @@ def get_crash_event_with_frames( "image_vmaddr": "0x187011000", "type": "macho", }, - { - "code_file": "/private/var/containers/Bundle/Application/9EB557CD-D653-4F51-BFCE-AECE691D4347/iOS-Swift.app/iOS-Swift", - "debug_id": "97862189-a5be-3af8-bcaa-7066ae872591", - "arch": "arm64", - "image_addr": "0x102b8c000", - "image_size": 180224, - "image_vmaddr": "0x100000000", - "type": "macho", - }, - { - "code_file": "/private/var/containers/Bundle/Application/9EB557CD-D653-4F51-BFCE-AECE691D4347/iOS-Swift.app/Frameworks/Sentry.framework/Sentry", - "debug_id": "4726c693-2359-3e9a-bd3a-559760d87486", - "arch": "arm64", - "image_addr": "0x102f68000", - "image_size": 786432, - "type": "macho", - }, - { - "code_file": "/System/Library/PrivateFrameworks/UIKitCore.framework/UIKitCore", - "debug_id": "b0858d8e-7220-37bf-873f-ecc2b0a358c3", - "arch": "arm64e", - "image_addr": "0x19c9eb000", - "image_size": 25309184, - "image_vmaddr": "0x18907f000", - "type": "macho", - }, - { - "code_file": "/System/Library/Frameworks/CFNetwork.framework/CFNetwork", - "debug_id": "b2273be9-538a-3f56-b9c7-801f39550f58", - "arch": "arm64e", - "image_addr": "0x19b98e000", - "image_size": 3977216, - "image_vmaddr": "0x188022000", - "in_app": False, - }, + # { + # "code_file": "/private/var/containers/Bundle/Application/9EB557CD-D653-4F51-BFCE-AECE691D4347/iOS-Swift.app/iOS-Swift", + # "debug_id": "97862189-a5be-3af8-bcaa-7066ae872591", + # "arch": "arm64", + # "image_addr": "0x102b8c000", + # "image_size": 180224, + # "image_vmaddr": "0x100000000", + # "type": "macho", + # }, + # { + # "code_file": "/private/var/containers/Bundle/Application/9EB557CD-D653-4F51-BFCE-AECE691D4347/iOS-Swift.app/Frameworks/Sentry.framework/Sentry", + # "debug_id": "4726c693-2359-3e9a-bd3a-559760d87486", + # "arch": "arm64", + # "image_addr": "0x102f68000", + # "image_size": 786432, + # "type": "macho", + # }, + # { + # "code_file": "/System/Library/PrivateFrameworks/UIKitCore.framework/UIKitCore", + # "debug_id": "b0858d8e-7220-37bf-873f-ecc2b0a358c3", + # "arch": "arm64e", + # "image_addr": "0x19c9eb000", + # "image_size": 25309184, + # "image_vmaddr": "0x18907f000", + # "type": "macho", + # }, + # { + # "code_file": "/System/Library/Frameworks/CFNetwork.framework/CFNetwork", + # "debug_id": "b2273be9-538a-3f56-b9c7-801f39550f58", + # "arch": "arm64e", + # "image_addr": "0x19b98e000", + # "image_size": 3977216, + # "image_vmaddr": "0x188022000", + # "in_app": False, + # }, ] }, "environment": "test-app", diff --git a/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py b/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py index 65949b71932383..a6c49a5170e6a5 100644 --- a/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py +++ b/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py @@ -1,10 +1,18 @@ +import abc from typing import Any, Mapping, Sequence, Tuple -from unittest.mock import MagicMock, Mock +from unittest.mock import MagicMock, Mock, patch import pytest +from sentry.testutils import TestCase +from sentry.testutils.cases import BaseTestCase +from sentry.testutils.silo import region_silo_test from sentry.utils.sdk_crashes.cocoa_sdk_crash_detector import CocoaSDKCrashDetector -from sentry.utils.sdk_crashes.sdk_crash_detection import SDKCrashDetection, SDKCrashReporter +from sentry.utils.sdk_crashes.sdk_crash_detection import ( + SDKCrashDetection, + SDKCrashReporter, + sdk_crash_detection, +) from tests.sentry.utils.sdk_crashes.test_fixture import ( IN_APP_FRAME, get_crash_event, @@ -13,6 +21,36 @@ ) +class BaseSDKCrashDetectionMixin(BaseTestCase, metaclass=abc.ABCMeta): + @abc.abstractmethod + def create_event(self, data, project_id, assert_no_errors=True): + pass + + +class CococaSDKTestMixin(BaseSDKCrashDetectionMixin): + @patch("sentry.utils.sdk_crashes.sdk_crash_detection.sdk_crash_detection.sdk_crash_reporter") + def test_detect_sdk_crash_unhandled_is_detected(self, mock_sdk_crash_reporter): + + event_data = get_crash_event() + event = self.create_event( + data=event_data, + project_id=self.project.id, + ) + + sdk_crash_detection.detect_sdk_crash(event=event) + + mock_sdk_crash_reporter.report.assert_called_once() + + +@region_silo_test +class SDKCrashDetectionTest( + TestCase, + CococaSDKTestMixin, +): + def create_event(self, data, project_id, assert_no_errors=True): + return self.store_event(data=data, project_id=project_id, assert_no_errors=assert_no_errors) + + @pytest.mark.parametrize( "event,should_be_reported", [ @@ -171,20 +209,20 @@ def test_report_cocoa_sdk_crash_filename(filename, should_be_reported): "only_inapp_after_sentry_frame_not_detected", ], ) -def test_report_cocoa_sdk_crash_frames(frames, should_be_reported): +def test_report_cocoa_sdk_crash_frames(self, frames, should_be_reported): event = get_crash_event_with_frames(frames) _run_report_test_with_event(event, should_be_reported) -def test_sdk_crash_detected_event_is_not_reported(): +def test_sdk_crash_detected_event_is_not_reported(self): event = get_crash_event() event["contexts"]["sdk_crash_detection"] = {"detected": True} _run_report_test_with_event(event, should_be_reported=False) -def test_cocoa_sdk_crash_detection_without_context(): +def test_cocoa_sdk_crash_detection_without_context(self): event = get_crash_event(function="-[SentryHub getScope]") event["contexts"] = {} @@ -223,5 +261,5 @@ def assert_sdk_crash_reported( assert reported_event["contexts"]["sdk_crash_detection"]["detected"] is True -def assert_no_sdk_crash_reported(crash_reporter: SDKCrashReporter): +def assert_no_sdk_crash_reported(self, crash_reporter: SDKCrashReporter): crash_reporter.report.assert_not_called() From d9a167bd2c7f459834156832015bd2a0fce70255 Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Mon, 15 May 2023 10:34:17 +0200 Subject: [PATCH 06/32] convert test_detect_sdk_crash --- .../sdk_crashes/test_sdk_crash_detection.py | 74 ++++++++++--------- 1 file changed, 41 insertions(+), 33 deletions(-) diff --git a/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py b/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py index a6c49a5170e6a5..7f29e04ed1aec3 100644 --- a/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py +++ b/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py @@ -4,8 +4,10 @@ import pytest +from sentry.issues.grouptype import PerformanceNPlusOneGroupType from sentry.testutils import TestCase from sentry.testutils.cases import BaseTestCase +from sentry.testutils.performance_issues.store_transaction import PerfIssueTransactionTestMixin from sentry.testutils.silo import region_silo_test from sentry.utils.sdk_crashes.cocoa_sdk_crash_detector import CocoaSDKCrashDetector from sentry.utils.sdk_crashes.sdk_crash_detection import ( @@ -27,11 +29,43 @@ def create_event(self, data, project_id, assert_no_errors=True): pass -class CococaSDKTestMixin(BaseSDKCrashDetectionMixin): +class CococaSDKTestMixin(BaseSDKCrashDetectionMixin, PerfIssueTransactionTestMixin): @patch("sentry.utils.sdk_crashes.sdk_crash_detection.sdk_crash_detection.sdk_crash_reporter") - def test_detect_sdk_crash_unhandled_is_detected(self, mock_sdk_crash_reporter): + def test_unhandled_is_detected(self, mock_sdk_crash_reporter): + self.execute_test(mock_sdk_crash_reporter, get_crash_event(), True) - event_data = get_crash_event() + @patch("sentry.utils.sdk_crashes.sdk_crash_detection.sdk_crash_detection.sdk_crash_reporter") + def test_handled_is_not_detected(self, mock_sdk_crash_reporter): + self.execute_test(mock_sdk_crash_reporter, get_crash_event(handled=True), False) + + @patch("sentry.utils.sdk_crashes.sdk_crash_detection.sdk_crash_detection.sdk_crash_reporter") + def test_wrong_function_not_detected(self, mock_sdk_crash_reporter): + self.execute_test(mock_sdk_crash_reporter, get_crash_event(function="Senry"), False) + + @patch("sentry.utils.sdk_crashes.sdk_crash_detection.sdk_crash_detection.sdk_crash_reporter") + def test_wrong_platform_not_detected(self, mock_sdk_crash_reporter): + self.execute_test(mock_sdk_crash_reporter, get_crash_event(platform="coco"), False) + + @patch("sentry.utils.sdk_crashes.sdk_crash_detection.sdk_crash_detection.sdk_crash_reporter") + def test_no_exception_not_detected(self, mock_sdk_crash_reporter): + self.execute_test(mock_sdk_crash_reporter, get_crash_event(exception=[]), False) + + @patch("sentry.utils.sdk_crashes.sdk_crash_detection.sdk_crash_detection.sdk_crash_reporter") + def test_performance_event_not_detected(self, mock_sdk_crash_reporter): + + fingerprint = "some_group" + fingerprint = f"{PerformanceNPlusOneGroupType.type_id}-{fingerprint}" + event = self.store_transaction( + project_id=self.project.id, + user_id="hi", + fingerprint=[fingerprint], + ) + + sdk_crash_detection.detect_sdk_crash(event=event) + + mock_sdk_crash_reporter.report.assert_not_called() + + def execute_test(self, mock_sdk_crash_reporter, event_data, should_be_reported): event = self.create_event( data=event_data, project_id=self.project.id, @@ -39,7 +73,10 @@ def test_detect_sdk_crash_unhandled_is_detected(self, mock_sdk_crash_reporter): sdk_crash_detection.detect_sdk_crash(event=event) - mock_sdk_crash_reporter.report.assert_called_once() + if should_be_reported: + mock_sdk_crash_reporter.report.assert_called_once() + else: + mock_sdk_crash_reporter.report.assert_not_called() @region_silo_test @@ -51,35 +88,6 @@ def create_event(self, data, project_id, assert_no_errors=True): return self.store_event(data=data, project_id=project_id, assert_no_errors=assert_no_errors) -@pytest.mark.parametrize( - "event,should_be_reported", - [ - ( - get_crash_event(), - True, - ), - ( - get_crash_event(handled=True), - False, - ), - (get_crash_event(function="Senry"), False), - (get_crash_event(platform="coco"), False), - (get_crash_event(type="erro"), False), - (get_crash_event(exception=[]), False), - ], - ids=[ - "unhandled_is_detected", - "handled_not_detected", - "wrong_function_not_detected", - "wrong_platform_not_detected", - "wrong_type_not_detected", - "no_exception_not_detected", - ], -) -def test_detect_sdk_crash(event, should_be_reported): - _run_report_test_with_event(event, should_be_reported) - - @pytest.mark.parametrize( "function,should_be_reported", [ From ca1bde8bb47fd0f7a78be07782b95d8c0cafa496 Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Mon, 15 May 2023 10:59:44 +0200 Subject: [PATCH 07/32] add CococaSDKFunctionTestMixin --- .../sdk_crashes/test_sdk_crash_detection.py | 131 ++++++++++++------ 1 file changed, 87 insertions(+), 44 deletions(-) diff --git a/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py b/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py index 7f29e04ed1aec3..f651cfede6d0ad 100644 --- a/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py +++ b/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py @@ -28,28 +28,102 @@ class BaseSDKCrashDetectionMixin(BaseTestCase, metaclass=abc.ABCMeta): def create_event(self, data, project_id, assert_no_errors=True): pass + def execute_test(self, event_data, should_be_reported, mock_sdk_crash_reporter): + event = self.create_event( + data=event_data, + project_id=self.project.id, + ) -class CococaSDKTestMixin(BaseSDKCrashDetectionMixin, PerfIssueTransactionTestMixin): - @patch("sentry.utils.sdk_crashes.sdk_crash_detection.sdk_crash_detection.sdk_crash_reporter") + sdk_crash_detection.detect_sdk_crash(event=event) + + if should_be_reported: + mock_sdk_crash_reporter.report.assert_called_once() + else: + mock_sdk_crash_reporter.report.assert_not_called() + + +@patch("sentry.utils.sdk_crashes.sdk_crash_detection.sdk_crash_detection.sdk_crash_reporter") +class CococaSDKTestMixin(BaseSDKCrashDetectionMixin): def test_unhandled_is_detected(self, mock_sdk_crash_reporter): - self.execute_test(mock_sdk_crash_reporter, get_crash_event(), True) + self.execute_test(get_crash_event(), True, mock_sdk_crash_reporter) - @patch("sentry.utils.sdk_crashes.sdk_crash_detection.sdk_crash_detection.sdk_crash_reporter") def test_handled_is_not_detected(self, mock_sdk_crash_reporter): - self.execute_test(mock_sdk_crash_reporter, get_crash_event(handled=True), False) + self.execute_test(get_crash_event(handled=True), False, mock_sdk_crash_reporter) - @patch("sentry.utils.sdk_crashes.sdk_crash_detection.sdk_crash_detection.sdk_crash_reporter") def test_wrong_function_not_detected(self, mock_sdk_crash_reporter): - self.execute_test(mock_sdk_crash_reporter, get_crash_event(function="Senry"), False) + self.execute_test(get_crash_event(function="Senry"), False, mock_sdk_crash_reporter) - @patch("sentry.utils.sdk_crashes.sdk_crash_detection.sdk_crash_detection.sdk_crash_reporter") def test_wrong_platform_not_detected(self, mock_sdk_crash_reporter): - self.execute_test(mock_sdk_crash_reporter, get_crash_event(platform="coco"), False) + self.execute_test(get_crash_event(platform="coco"), False, mock_sdk_crash_reporter) - @patch("sentry.utils.sdk_crashes.sdk_crash_detection.sdk_crash_detection.sdk_crash_reporter") def test_no_exception_not_detected(self, mock_sdk_crash_reporter): - self.execute_test(mock_sdk_crash_reporter, get_crash_event(exception=[]), False) + self.execute_test(get_crash_event(exception=[]), False, mock_sdk_crash_reporter) + + +@patch("sentry.utils.sdk_crashes.sdk_crash_detection.sdk_crash_detection.sdk_crash_reporter") +class CococaSDKFunctionTestMixin(BaseSDKCrashDetectionMixin): + def test_hub_reported(self, mock_sdk_crash_reporter): + self.execute_test( + get_crash_event(function="-[SentryHub getScope]"), True, mock_sdk_crash_reporter + ) + + def test_sentrycrash_reported(self, mock_sdk_crash_reporter): + self.execute_test( + get_crash_event(function="sentrycrashdl_getBinaryImage"), True, mock_sdk_crash_reporter + ) + + def test_sentryisgreat_reported(self, mock_sdk_crash_reporter): + self.execute_test( + get_crash_event(function="-[sentryisgreat]"), True, mock_sdk_crash_reporter + ) + def test_sentryswizzle_reported(self, mock_sdk_crash_reporter): + self.execute_test( + get_crash_event( + function="__47-[SentryBreadcrumbTracker swizzleViewDidAppear]_block_invoke_2" + ), + True, + mock_sdk_crash_reporter, + ) + + def test_sentrycrash_crash_reported(self, mock_sdk_crash_reporter): + self.execute_test( + get_crash_event(function="-[SentryCrash crash]"), + True, + mock_sdk_crash_reporter, + ) + + def test_senryhub_not_reported(self, mock_sdk_crash_reporter): + self.execute_test( + get_crash_event(function="-[SenryHub getScope]"), + False, + mock_sdk_crash_reporter, + ) + + def test_senryhub_no_brackets_not_reported(self, mock_sdk_crash_reporter): + self.execute_test( + get_crash_event(function="-SentryHub getScope]"), + False, + mock_sdk_crash_reporter, + ) + + def test_somesentryhub_not_reported(self, mock_sdk_crash_reporter): + self.execute_test( + get_crash_event(function="-[SomeSentryHub getScope]"), + False, + mock_sdk_crash_reporter, + ) + + # This method is used for testing, so we can ignore it. + def test_sentrycrash_not_reported(self, mock_sdk_crash_reporter): + self.execute_test( + get_crash_event(function="+[SentrySDK crash]"), + False, + mock_sdk_crash_reporter, + ) + + +class PerformanceEventTestMixin(BaseSDKCrashDetectionMixin, PerfIssueTransactionTestMixin): @patch("sentry.utils.sdk_crashes.sdk_crash_detection.sdk_crash_detection.sdk_crash_reporter") def test_performance_event_not_detected(self, mock_sdk_crash_reporter): @@ -65,49 +139,18 @@ def test_performance_event_not_detected(self, mock_sdk_crash_reporter): mock_sdk_crash_reporter.report.assert_not_called() - def execute_test(self, mock_sdk_crash_reporter, event_data, should_be_reported): - event = self.create_event( - data=event_data, - project_id=self.project.id, - ) - - sdk_crash_detection.detect_sdk_crash(event=event) - - if should_be_reported: - mock_sdk_crash_reporter.report.assert_called_once() - else: - mock_sdk_crash_reporter.report.assert_not_called() - @region_silo_test class SDKCrashDetectionTest( TestCase, CococaSDKTestMixin, + CococaSDKFunctionTestMixin, + PerformanceEventTestMixin, ): def create_event(self, data, project_id, assert_no_errors=True): return self.store_event(data=data, project_id=project_id, assert_no_errors=assert_no_errors) -@pytest.mark.parametrize( - "function,should_be_reported", - [ - ("-[SentryHub getScope]", True), - ("sentrycrashdl_getBinaryImage", True), - ("-[sentryisgreat]", True), - ("__47-[SentryBreadcrumbTracker swizzleViewDidAppear]_block_invoke_2", True), - ("-[SentryCrash crash]", True), - ("-[SenryHub getScope]", False), - ("-SentryHub getScope]", False), - ("-[SomeSentryHub getScope]", False), - ("+[SentrySDK crash]", False), - ], -) -def test_cocoa_sdk_crash_detection(function, should_be_reported): - event = get_crash_event(function=function) - - _run_report_test_with_event(event, should_be_reported) - - @pytest.mark.parametrize( "filename,should_be_reported", [ From 6b2a4c79befa453410c18ba9ab31766733ae5aff Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Mon, 15 May 2023 11:35:23 +0200 Subject: [PATCH 08/32] finish converting sdk_crash_detection tests --- .../sdk_crashes/test_sdk_crash_detection.py | 320 +++++++++--------- 1 file changed, 152 insertions(+), 168 deletions(-) diff --git a/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py b/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py index f651cfede6d0ad..3c2e4f18597145 100644 --- a/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py +++ b/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py @@ -1,20 +1,13 @@ import abc -from typing import Any, Mapping, Sequence, Tuple -from unittest.mock import MagicMock, Mock, patch - -import pytest +from typing import Any, Mapping, Sequence +from unittest.mock import patch from sentry.issues.grouptype import PerformanceNPlusOneGroupType from sentry.testutils import TestCase from sentry.testutils.cases import BaseTestCase from sentry.testutils.performance_issues.store_transaction import PerfIssueTransactionTestMixin from sentry.testutils.silo import region_silo_test -from sentry.utils.sdk_crashes.cocoa_sdk_crash_detector import CocoaSDKCrashDetector -from sentry.utils.sdk_crashes.sdk_crash_detection import ( - SDKCrashDetection, - SDKCrashReporter, - sdk_crash_detection, -) +from sentry.utils.sdk_crashes.sdk_crash_detection import sdk_crash_detection from tests.sentry.utils.sdk_crashes.test_fixture import ( IN_APP_FRAME, get_crash_event, @@ -38,10 +31,30 @@ def execute_test(self, event_data, should_be_reported, mock_sdk_crash_reporter): if should_be_reported: mock_sdk_crash_reporter.report.assert_called_once() + + reported_event = mock_sdk_crash_reporter.report.call_args.args[0] + assert reported_event["contexts"]["sdk_crash_detection"]["detected"] is True else: mock_sdk_crash_reporter.report.assert_not_called() +class PerformanceEventTestMixin(BaseSDKCrashDetectionMixin, PerfIssueTransactionTestMixin): + @patch("sentry.utils.sdk_crashes.sdk_crash_detection.sdk_crash_detection.sdk_crash_reporter") + def test_performance_event_not_detected(self, mock_sdk_crash_reporter): + + fingerprint = "some_group" + fingerprint = f"{PerformanceNPlusOneGroupType.type_id}-{fingerprint}" + event = self.store_transaction( + project_id=self.project.id, + user_id="hi", + fingerprint=[fingerprint], + ) + + sdk_crash_detection.detect_sdk_crash(event=event) + + mock_sdk_crash_reporter.report.assert_not_called() + + @patch("sentry.utils.sdk_crashes.sdk_crash_detection.sdk_crash_detection.sdk_crash_reporter") class CococaSDKTestMixin(BaseSDKCrashDetectionMixin): def test_unhandled_is_detected(self, mock_sdk_crash_reporter): @@ -59,6 +72,18 @@ def test_wrong_platform_not_detected(self, mock_sdk_crash_reporter): def test_no_exception_not_detected(self, mock_sdk_crash_reporter): self.execute_test(get_crash_event(exception=[]), False, mock_sdk_crash_reporter) + def test_sdk_crash_detected_event_is_not_reported(self, mock_sdk_crash_reporter): + event = get_crash_event() + event["contexts"]["sdk_crash_detection"] = {"detected": True} + + self.execute_test(event, False, mock_sdk_crash_reporter) + + def test_cocoa_sdk_crash_detection_without_context(self, mock_sdk_crash_reporter): + event = get_crash_event(function="-[SentryHub getScope]") + event["contexts"] = {} + + self.execute_test(event, True, mock_sdk_crash_reporter) + @patch("sentry.utils.sdk_crashes.sdk_crash_detection.sdk_crash_detection.sdk_crash_reporter") class CococaSDKFunctionTestMixin(BaseSDKCrashDetectionMixin): @@ -114,7 +139,7 @@ def test_somesentryhub_not_reported(self, mock_sdk_crash_reporter): mock_sdk_crash_reporter, ) - # This method is used for testing, so we can ignore it. + # "+[SentrySDK crash]" is used for testing, so we must ignore it. def test_sentrycrash_not_reported(self, mock_sdk_crash_reporter): self.execute_test( get_crash_event(function="+[SentrySDK crash]"), @@ -123,86 +148,32 @@ def test_sentrycrash_not_reported(self, mock_sdk_crash_reporter): ) -class PerformanceEventTestMixin(BaseSDKCrashDetectionMixin, PerfIssueTransactionTestMixin): - @patch("sentry.utils.sdk_crashes.sdk_crash_detection.sdk_crash_detection.sdk_crash_reporter") - def test_performance_event_not_detected(self, mock_sdk_crash_reporter): - - fingerprint = "some_group" - fingerprint = f"{PerformanceNPlusOneGroupType.type_id}-{fingerprint}" - event = self.store_transaction( - project_id=self.project.id, - user_id="hi", - fingerprint=[fingerprint], +@patch("sentry.utils.sdk_crashes.sdk_crash_detection.sdk_crash_detection.sdk_crash_reporter") +class CococaSDKFilenameTestMixin(BaseSDKCrashDetectionMixin): + def test_filename_includes_sentrycrash_reported(self, mock_sdk_crash_reporter): + self.execute_test( + self._get_crash_event("SentryCrashMonitor_CPPException.cpp"), + True, + mock_sdk_crash_reporter, ) - sdk_crash_detection.detect_sdk_crash(event=event) - - mock_sdk_crash_reporter.report.assert_not_called() - - -@region_silo_test -class SDKCrashDetectionTest( - TestCase, - CococaSDKTestMixin, - CococaSDKFunctionTestMixin, - PerformanceEventTestMixin, -): - def create_event(self, data, project_id, assert_no_errors=True): - return self.store_event(data=data, project_id=project_id, assert_no_errors=assert_no_errors) + def test_filename_includes_sentrymonitor_reported(self, mock_sdk_crash_reporter): + self.execute_test( + self._get_crash_event("SentryMonitor_CPPException.cpp"), + True, + mock_sdk_crash_reporter, + ) + def test_filename_includes_senry_not_reported(self, mock_sdk_crash_reporter): + self.execute_test( + self._get_crash_event("SentrMonitor_CPPException.cpp"), + False, + mock_sdk_crash_reporter, + ) -@pytest.mark.parametrize( - "filename,should_be_reported", - [ - ("SentryCrashMonitor_CPPException.cpp", True), - ("SentryMonitor_CPPException.cpp", True), - ("SentrMonitor_CPPException.cpp", False), - ], -) -def test_report_cocoa_sdk_crash_filename(filename, should_be_reported): - event = get_crash_event_with_frames( - frames=[ - { - "function": "__handleUncaughtException", - "symbol": "__handleUncaughtException", - "package": "CoreFoundation", - "in_app": False, - }, - { - "function": "_objc_terminate", - "symbol": "_ZL15_objc_terminatev", - "package": "libobjc.A.dylib", - "in_app": False, - }, - { - "function": "CPPExceptionTerminate", - "raw_function": "CPPExceptionTerminate()", - "filename": filename, - "symbol": "_ZL21CPPExceptionTerminatev", - "package": "MainApp", - "in_app": False, - }, - { - "function": "std::__terminate", - "symbol": "_ZSt11__terminatePFvvE", - "package": "libc++abi.dylib", - "in_app": False, - }, - ] - ) - - _run_report_test_with_event(event, should_be_reported) - - -@pytest.mark.parametrize( - "frames,should_be_reported", - [ - ([], False), - ([{"empty": "frame"}], False), - ([get_sentry_frame("-[Sentry]")], True), - ([get_sentry_frame("-[Sentry]", in_app=True)], True), - ( - [ + def _get_crash_event(self, filename) -> Sequence[Mapping[str, Any]]: + return get_crash_event_with_frames( + frames=[ { "function": "__handleUncaughtException", "symbol": "__handleUncaughtException", @@ -215,102 +186,115 @@ def test_report_cocoa_sdk_crash_filename(filename, should_be_reported): "package": "libobjc.A.dylib", "in_app": False, }, - get_sentry_frame("sentrycrashdl_getBinaryImage"), - { - "function": "std::__terminate", - "symbol": "_ZSt11__terminatePFvvE", - "package": "libc++abi.dylib", - "in_app": False, - }, - ], - True, - ), - ( - [ - IN_APP_FRAME, - { - "function": "__handleUncaughtException", - "symbol": "__handleUncaughtException", - "package": "CoreFoundation", - "in_app": False, - }, { - "function": "_objc_terminate", - "symbol": "_ZL15_objc_terminatev", - "package": "libobjc.A.dylib", + "function": "CPPExceptionTerminate", + "raw_function": "CPPExceptionTerminate()", + "filename": filename, + "symbol": "_ZL21CPPExceptionTerminatev", + "package": "MainApp", "in_app": False, }, - get_sentry_frame("sentrycrashdl_getBinaryImage"), { "function": "std::__terminate", "symbol": "_ZSt11__terminatePFvvE", "package": "libc++abi.dylib", "in_app": False, }, - ], - False, - ), - ], - ids=[ - "no_frames_not_detected", - "empty_frame_not_detected", - "single_frame_is_detected", - "single_in_app_frame_is_detected", - "only_non_inapp_after_sentry_frame_is_detected", - "only_inapp_after_sentry_frame_not_detected", - ], -) -def test_report_cocoa_sdk_crash_frames(self, frames, should_be_reported): - event = get_crash_event_with_frames(frames) - - _run_report_test_with_event(event, should_be_reported) - - -def test_sdk_crash_detected_event_is_not_reported(self): - event = get_crash_event() - event["contexts"]["sdk_crash_detection"] = {"detected": True} - - _run_report_test_with_event(event, should_be_reported=False) - - -def test_cocoa_sdk_crash_detection_without_context(self): - event = get_crash_event(function="-[SentryHub getScope]") - event["contexts"] = {} - - _run_report_test_with_event(event, True) - - -def given_crash_detector() -> Tuple[SDKCrashDetection, SDKCrashReporter]: - crash_reporter = Mock(spec=SDKCrashReporter) - cocoa_sdk_crash_detector = CocoaSDKCrashDetector() - - event_stripper = Mock() - event_stripper.strip_event_data = MagicMock(side_effect=lambda x: x) + ] + ) - crash_detection = SDKCrashDetection(crash_reporter, cocoa_sdk_crash_detector, event_stripper) - return crash_detection, crash_reporter +@patch("sentry.utils.sdk_crashes.sdk_crash_detection.sdk_crash_detection.sdk_crash_reporter") +class CococaSDKFramesTestMixin(BaseSDKCrashDetectionMixin): + def test_frames_empty_frame_not_reported(self, mock_sdk_crash_reporter): + self.execute_test( + get_crash_event_with_frames([{"empty": "frame"}]), + False, + mock_sdk_crash_reporter, + ) + def test_frames_single_frame_reported(self, mock_sdk_crash_reporter): + self.execute_test( + get_crash_event_with_frames([get_sentry_frame("-[Sentry]")]), + True, + mock_sdk_crash_reporter, + ) -def _run_report_test_with_event(event, should_be_reported): - crash_detector, crash_reporter = given_crash_detector() + def test_frames_in_app_frame_frame_reported(self, mock_sdk_crash_reporter): + self.execute_test( + get_crash_event_with_frames([get_sentry_frame("-[Sentry]", in_app=True)]), + True, + mock_sdk_crash_reporter, + ) - crash_detector.detect_sdk_crash(event) + def test_frames_only_non_in_app_after_sentry_frame_is_reported(self, mock_sdk_crash_reporter): + self.execute_test( + get_crash_event_with_frames( + [ + { + "function": "__handleUncaughtException", + "symbol": "__handleUncaughtException", + "package": "CoreFoundation", + "in_app": False, + }, + { + "function": "_objc_terminate", + "symbol": "_ZL15_objc_terminatev", + "package": "libobjc.A.dylib", + "in_app": False, + }, + get_sentry_frame("sentrycrashdl_getBinaryImage"), + { + "function": "std::__terminate", + "symbol": "_ZSt11__terminatePFvvE", + "package": "libc++abi.dylib", + "in_app": False, + }, + ] + ), + True, + mock_sdk_crash_reporter, + ) - if should_be_reported: - assert_sdk_crash_reported(crash_reporter, event) - else: - assert_no_sdk_crash_reported(crash_reporter) + def test_frames_only_in_app_after_sentry_frame_not_reported(self, mock_sdk_crash_reporter): + self.execute_test( + get_crash_event_with_frames( + [ + IN_APP_FRAME, + { + "function": "__handleUncaughtException", + "symbol": "__handleUncaughtException", + "package": "CoreFoundation", + "in_app": False, + }, + { + "function": "_objc_terminate", + "symbol": "_ZL15_objc_terminatev", + "package": "libobjc.A.dylib", + "in_app": False, + }, + get_sentry_frame("sentrycrashdl_getBinaryImage"), + { + "function": "std::__terminate", + "symbol": "_ZSt11__terminatePFvvE", + "package": "libc++abi.dylib", + "in_app": False, + }, + ] + ), + False, + mock_sdk_crash_reporter, + ) -def assert_sdk_crash_reported( - crash_reporter: SDKCrashReporter, expected_event: Sequence[Mapping[str, Any]] +@region_silo_test +class SDKCrashDetectionTest( + TestCase, + CococaSDKTestMixin, + CococaSDKFunctionTestMixin, + CococaSDKFilenameTestMixin, + CococaSDKFramesTestMixin, + PerformanceEventTestMixin, ): - crash_reporter.report.assert_called_once_with(expected_event) - - reported_event = crash_reporter.report.call_args.args[0] - assert reported_event["contexts"]["sdk_crash_detection"]["detected"] is True - - -def assert_no_sdk_crash_reported(self, crash_reporter: SDKCrashReporter): - crash_reporter.report.assert_not_called() + def create_event(self, data, project_id, assert_no_errors=True): + return self.store_event(data=data, project_id=project_id, assert_no_errors=assert_no_errors) From f358fd5a6c23d4702808074da52d229660cd7469 Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Mon, 15 May 2023 11:49:41 +0200 Subject: [PATCH 09/32] fix debug meta --- .../sentry/utils/sdk_crashes/test_fixture.py | 90 +++++++++---------- 1 file changed, 41 insertions(+), 49 deletions(-) diff --git a/tests/sentry/utils/sdk_crashes/test_fixture.py b/tests/sentry/utils/sdk_crashes/test_fixture.py index 4ebe079aa2b392..b7f81928fc222a 100644 --- a/tests/sentry/utils/sdk_crashes/test_fixture.py +++ b/tests/sentry/utils/sdk_crashes/test_fixture.py @@ -9,9 +9,9 @@ "abs_path": "/Users/sentry/git/iOS/Sentry/LoggedOut/LoginViewController.swift", "lineno": 196, "in_app": True, - "image_addr": "0x1025e8000", + "image_addr": "0x100260000", "instruction_addr": "0x102b16630", - "symbol_addr": "0x1025e8000", + "symbol_addr": "0x100260000", } @@ -20,7 +20,7 @@ def get_sentry_frame(function: str, in_app: bool = False) -> Mapping[str, Any]: "function": function, "package": "Sentry", "in_app": in_app, - "image_addr": "0x102f68000", + "image_addr": "0x100304000", } @@ -32,7 +32,7 @@ def get_frames(function: str, sentry_frame_in_app: bool = False) -> Sequence[Map "symbol": "$s8Sentry9LoginViewControllerC13viewDidAppearyySbF", "package": "SentryApp", "filename": "LoginViewController.swift", - "image_addr": "0x102b8c000", + "image_addr": "0x100260000", }, IN_APP_FRAME, { @@ -40,35 +40,35 @@ def get_frames(function: str, sentry_frame_in_app: bool = False) -> Sequence[Map "symbol": "-[UIViewController _setViewAppearState:isAnimating:]", "package": "UIKitCore", "in_app": False, - "image_addr": "0x19c9eb000", + "image_addr": "0x1a4e8f000", }, { "function": "-[UIViewController __viewDidAppear:]", "symbol": "-[UIViewController __viewDidAppear:]", "package": "UIKitCore", "in_app": False, - "image_addr": "0x19c9eb000", + "image_addr": "0x1a4e8f000", }, { "function": "-[UIViewController _endAppearanceTransition:]", "symbol": "-[UIViewController _endAppearanceTransition:]", "package": "UIKitCore", "in_app": False, - "image_addr": "0x19c9eb000", + "image_addr": "0x1a4e8f000", }, { "function": "-[UINavigationController navigationTransitionView:didEndTransition:fromView:toView:]", "symbol": "-[UINavigationController navigationTransitionView:didEndTransition:fromView:toView:]", "package": "UIKitCore", "in_app": False, - "image_addr": "0x19c9eb000", + "image_addr": "0x1a4e8f000", }, { "function": "__49-[UINavigationController _startCustomTransition:]_block_invoke", "symbol": "__49-[UINavigationController _startCustomTransition:]_block_invoke", "package": "UIKitCore", "in_app": False, - "image_addr": "0x19c9eb000", + "image_addr": "0x1a4e8f000", }, ] @@ -146,49 +146,41 @@ def get_crash_event_with_frames( "debug_meta": { "images": [ { - "code_file": "/private/var/containers/Bundle/Application/9EB557CD-D653-4F51-BFCE-AECE691D4347/SentryApp.app/iOS-SentryApp", - "debug_id": "97862189-a5be-3af8-bcaa-7066ae872592", + "code_file": "/private/var/containers/Bundle/Application/895DA2DE-5FE3-44A0-8C0F-900519EA5516/iOS-Swift.app/iOS-Swift", + "debug_id": "aa8a3697-c88a-36f9-a687-3d3596568c8d", "arch": "arm64", - "image_addr": "0x1025e8000", - "image_size": 170224, - "image_vmaddr": "0x187011000", + "image_addr": "0x100260000", + "image_size": 180224, + "image_vmaddr": "0x100000000", + "type": "macho", + }, + { + "code_file": "/private/var/containers/Bundle/Application/9EB557CD-D653-4F51-BFCE-AECE691D4347/iOS-Swift.app/Frameworks/Sentry.framework/Sentry", + "debug_id": "e2623c4d-79c5-3cdf-90ab-2cf44e026bdd", + "arch": "arm64", + "image_addr": "0x100304000", + "image_size": 802816, + "type": "macho", + }, + { + "code_file": "/System/Library/PrivateFrameworks/UIKitCore.framework/UIKitCore", + "debug_id": "b0858d8e-7220-37bf-873f-ecc2b0a358c3", + "arch": "arm64e", + "image_addr": "0x1a4e8f000", + "image_size": 25309184, + "image_vmaddr": "0x188ff7000", + "type": "macho", + }, + { + "code_file": "/System/Library/Frameworks/CFNetwork.framework/CFNetwork", + "debug_id": "b2273be9-538a-3f56-b9c7-801f39550f58", + "arch": "arm64e", + "image_addr": "0x1a3e32000", + "image_size": 3977216, + "image_vmaddr": "0x187f9a000", + "in_app": False, "type": "macho", }, - # { - # "code_file": "/private/var/containers/Bundle/Application/9EB557CD-D653-4F51-BFCE-AECE691D4347/iOS-Swift.app/iOS-Swift", - # "debug_id": "97862189-a5be-3af8-bcaa-7066ae872591", - # "arch": "arm64", - # "image_addr": "0x102b8c000", - # "image_size": 180224, - # "image_vmaddr": "0x100000000", - # "type": "macho", - # }, - # { - # "code_file": "/private/var/containers/Bundle/Application/9EB557CD-D653-4F51-BFCE-AECE691D4347/iOS-Swift.app/Frameworks/Sentry.framework/Sentry", - # "debug_id": "4726c693-2359-3e9a-bd3a-559760d87486", - # "arch": "arm64", - # "image_addr": "0x102f68000", - # "image_size": 786432, - # "type": "macho", - # }, - # { - # "code_file": "/System/Library/PrivateFrameworks/UIKitCore.framework/UIKitCore", - # "debug_id": "b0858d8e-7220-37bf-873f-ecc2b0a358c3", - # "arch": "arm64e", - # "image_addr": "0x19c9eb000", - # "image_size": 25309184, - # "image_vmaddr": "0x18907f000", - # "type": "macho", - # }, - # { - # "code_file": "/System/Library/Frameworks/CFNetwork.framework/CFNetwork", - # "debug_id": "b2273be9-538a-3f56-b9c7-801f39550f58", - # "arch": "arm64e", - # "image_addr": "0x19b98e000", - # "image_size": 3977216, - # "image_vmaddr": "0x188022000", - # "in_app": False, - # }, ] }, "environment": "test-app", From 5c6c0b6dee761313db3218de7d39508a9cc16b35 Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Tue, 16 May 2023 10:30:17 +0200 Subject: [PATCH 10/32] Convert event stripper tests --- .../utils/sdk_crashes/event_stripper.py | 19 +- .../utils/sdk_crashes/test_event_stripper.py | 190 +++++++++++------- .../sentry/utils/sdk_crashes/test_fixture.py | 1 + 3 files changed, 132 insertions(+), 78 deletions(-) diff --git a/src/sentry/utils/sdk_crashes/event_stripper.py b/src/sentry/utils/sdk_crashes/event_stripper.py index 4bd19eb46b724e..fae6653cbb6406 100644 --- a/src/sentry/utils/sdk_crashes/event_stripper.py +++ b/src/sentry/utils/sdk_crashes/event_stripper.py @@ -27,22 +27,25 @@ def __init__( } def strip_event_data(self, event: Event) -> Event: - new_event = dict(filter(self._filter_event, event.data.items())) - new_event["contexts"] = dict(filter(self._filter_contexts, new_event["contexts"].items())) + new_event_data = dict(filter(self._filter_event, event.data.items())) + new_event_data["contexts"] = dict( + filter(self._filter_contexts, new_event_data["contexts"].items()) + ) stripped_frames = [] - frames = get_path(event, "exception", "values", -1, "stacktrace", "frames") + frames = get_path(new_event_data, "exception", "values", -1, "stacktrace", "frames") if frames is not None: stripped_frames = self._strip_frames(frames) - new_event["exception"]["values"][0]["stacktrace"]["frames"] = stripped_frames + new_event_data["exception"]["values"][0]["stacktrace"]["frames"] = stripped_frames - debug_meta_images = get_path(event, "debug_meta", "images") + debug_meta_images = get_path(new_event_data, "debug_meta", "images") if debug_meta_images is not None: stripped_debug_meta_images = self._strip_debug_meta(debug_meta_images, stripped_frames) - new_event["debug_meta"]["images"] = stripped_debug_meta_images + new_event_data["debug_meta"]["images"] = stripped_debug_meta_images - return new_event + event.data = new_event_data + return event def _filter_event(self, pair): key, _ = pair @@ -74,5 +77,5 @@ def _strip_frames(self, frames: Sequence[Mapping[str, Any]]) -> Sequence[Mapping return [ frame for frame in frames - if self.sdk_crash_detector.is_sdk_frame(frame) or frame.get("in_app", True) is False + if self.sdk_crash_detector.is_sdk_frame(frame) or frame.get("in_app", None) is False ] diff --git a/tests/sentry/utils/sdk_crashes/test_event_stripper.py b/tests/sentry/utils/sdk_crashes/test_event_stripper.py index c58b15a5e2a2dd..e563528ebc2e3a 100644 --- a/tests/sentry/utils/sdk_crashes/test_event_stripper.py +++ b/tests/sentry/utils/sdk_crashes/test_event_stripper.py @@ -1,7 +1,9 @@ +import abc from unittest.mock import Mock -import pytest - +from sentry.testutils import TestCase +from sentry.testutils.cases import BaseTestCase +from sentry.testutils.silo import region_silo_test from sentry.utils.safe import get_path from sentry.utils.sdk_crashes.cocoa_sdk_crash_detector import CocoaSDKCrashDetector from sentry.utils.sdk_crashes.event_stripper import EventStripper @@ -13,100 +15,148 @@ ) -def test_strip_event_data_keeps_allowed_keys(): - event_stripper = EventStripper(Mock()) +class BaseEventStripperMixin(BaseTestCase, metaclass=abc.ABCMeta): + @abc.abstractmethod + def create_event(self, data, project_id, assert_no_errors=True): + pass - stripped_event = event_stripper.strip_event_data(get_crash_event()) + def execute_test(self, event_data, should_be_reported, mock_sdk_crash_reporter): + pass - keys_removed = {"tags", "user", "threads", "breadcrumbs", "environment"} - for key in keys_removed: - assert stripped_event.get(key) is None - keys_kept = { - "type", - "datetime", - "timestamp", - "platform", - "sdk", - "level", - "logger", - "exception", - "debug_meta", - "contexts", - } +class EventStripperTestMixin(BaseEventStripperMixin): + def test_strip_event_data_keeps_allowed_keys(self): + event_stripper = EventStripper(CocoaSDKCrashDetector()) - for key in keys_kept: - assert stripped_event.get(key) is not None + event = self.create_event( + data=get_crash_event(), + project_id=self.project.id, + ) + stripped_event = event_stripper.strip_event_data(event) -def test_strip_event_data_strips_context(): - event_stripper = EventStripper(Mock()) + keys_removed = {"tags", "user", "threads", "breadcrumbs", "environment"} + for key in keys_removed: + assert stripped_event.data.get(key) is None, f"key {key} should be removed" - stripped_event = event_stripper.strip_event_data(get_crash_event()) + keys_kept = { + "type", + "timestamp", + "platform", + "sdk", + "level", + "logger", + "exception", + "debug_meta", + "contexts", + } - contexts = stripped_event.get("contexts") - assert contexts is not None - assert contexts.get("app") is None - assert contexts.get("os") is not None - assert contexts.get("device") is not None + for key in keys_kept: + assert stripped_event.data.get(key) is not None, f"key {key} should be kept" + def test_strip_event_data_removes_ip_address(self): + event_stripper = EventStripper(CocoaSDKCrashDetector()) -@pytest.mark.parametrize( - "function,in_app", - [ - ("SentryCrashMonitor_CPPException.cpp", True), - ("SentryCrashMonitor_CPPException.cpp", False), - ], - ids=["sentry_in_app_frame_kept", "sentry_not_in_app_frame_kept"], -) -def test_strip_frames(function, in_app): - event_stripper = EventStripper(CocoaSDKCrashDetector()) + event = self.create_event( + data=get_crash_event(), + project_id=self.project.id, + ) + + assert event.ip_address is not None + + stripped_event = event_stripper.strip_event_data(event) + + assert stripped_event.ip_address is None + + def test_strip_event_data_without_debug_meta(self): + event_stripper = EventStripper(CocoaSDKCrashDetector()) + + event_data = get_crash_event() + event_data["debug_meta"]["images"] = None + + event = self.create_event( + data=event_data, + project_id=self.project.id, + ) + + stripped_event = event_stripper.strip_event_data(event) + + debug_meta_images = get_path(stripped_event.data, "debug_meta", "images") + assert debug_meta_images is None - frames = get_frames(function, sentry_frame_in_app=in_app) - event = get_crash_event_with_frames(frames) + def test_strip_event_data_strips_context(self): + event_stripper = EventStripper(CocoaSDKCrashDetector()) - stripped_event = event_stripper.strip_event_data(event) + event = self.create_event( + data=get_crash_event(), + project_id=self.project.id, + ) - stripped_frames = get_path(stripped_event, "exception", "values", -1, "stacktrace", "frames") + stripped_event = event_stripper.strip_event_data(event) - assert len(stripped_frames) == 6 - assert ( - len([frame for frame in stripped_frames if frame["function"] == IN_APP_FRAME["function"]]) - == 0 - ), "in_app frame should be removed" + contexts = stripped_event.data.get("contexts") + assert contexts is not None + assert contexts.get("app") is None + assert contexts.get("os") is not None + assert contexts.get("device") is not None + def test_strip_event_data_strips_non_referenced_dsyms(self): + event_stripper = EventStripper(Mock()) -def test_strip_event_data_strips_non_referenced_dsyms(): - event_stripper = EventStripper(Mock()) + event = self.create_event( + data=get_crash_event(), + project_id=self.project.id, + ) - stripped_event = event_stripper.strip_event_data(get_crash_event()) + stripped_event = event_stripper.strip_event_data(event) - debug_meta_images = get_path(stripped_event, "debug_meta", "images") + debug_meta_images = get_path(stripped_event.data, "debug_meta", "images") - image_addresses = set(map(lambda image: image["image_addr"], debug_meta_images)) - expected_image_addresses = {"0x1025e8000", "0x102b8c000", "0x102f68000", "0x19c9eb000"} - assert image_addresses == expected_image_addresses + image_addresses = set(map(lambda image: image["image_addr"], debug_meta_images)) + expected_image_addresses = {"0x1a4e8f000", "0x100304000", "0x100260000"} + assert image_addresses == expected_image_addresses + def test_strip_frames_sentry_in_app_frame_kept(self): + frames = get_frames("SentryCrashMonitor_CPPException.cpp", sentry_frame_in_app=True) + self._execute_strip_frames_test(frames) -def test_strip_event_data_without_frames(): - event_stripper = EventStripper(Mock()) + def test_strip_frames_sentry_non_in_app_frame_kept(self): + frames = get_frames("SentryCrashMonitor_CPPException.cpp", sentry_frame_in_app=False) + self._execute_strip_frames_test(frames) - crash_event = get_crash_event() - crash_event["exception"]["values"][0]["stacktrace"]["frames"] = None + def _execute_strip_frames_test(self, frames): + event_stripper = EventStripper(CocoaSDKCrashDetector()) - stripped_event = event_stripper.strip_event_data(crash_event) + event_data = get_crash_event_with_frames(frames) - debug_meta_images = get_path(stripped_event, "debug_meta", "images") - assert len(debug_meta_images) == 0 + event = self.create_event( + data=event_data, + project_id=self.project.id, + ) + stripped_event = event_stripper.strip_event_data(event) -def test_strip_event_data_without_debug_meta(): - event_stripper = EventStripper(Mock()) + stripped_frames = get_path( + stripped_event.data, "exception", "values", -1, "stacktrace", "frames" + ) - crash_event = get_crash_event() - crash_event["debug_meta"]["images"] = None + assert len(stripped_frames) == 6 + assert ( + len( + [ + frame + for frame in stripped_frames + if frame["function"] == IN_APP_FRAME["function"] + ] + ) + == 0 + ), "in_app frame should be removed" - stripped_event = event_stripper.strip_event_data(crash_event) - debug_meta_images = get_path(stripped_event, "debug_meta", "images") - assert debug_meta_images is None +@region_silo_test +class EventStripperTest( + TestCase, + EventStripperTestMixin, +): + def create_event(self, data, project_id, assert_no_errors=True): + return self.store_event(data=data, project_id=project_id, assert_no_errors=assert_no_errors) diff --git a/tests/sentry/utils/sdk_crashes/test_fixture.py b/tests/sentry/utils/sdk_crashes/test_fixture.py index b7f81928fc222a..f2a4dab467335d 100644 --- a/tests/sentry/utils/sdk_crashes/test_fixture.py +++ b/tests/sentry/utils/sdk_crashes/test_fixture.py @@ -31,6 +31,7 @@ def get_frames(function: str, sentry_frame_in_app: bool = False) -> Sequence[Map "function": "LoginViewController.viewDidAppear", "symbol": "$s8Sentry9LoginViewControllerC13viewDidAppearyySbF", "package": "SentryApp", + "in_app": True, "filename": "LoginViewController.swift", "image_addr": "0x100260000", }, From 296ba23fe2ea4f0b21ecb483ebc2ea0a63ece93c Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Tue, 16 May 2023 10:33:12 +0200 Subject: [PATCH 11/32] fix tests --- src/sentry/utils/sdk_crashes/sdk_crash_detection.py | 4 +++- tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/sentry/utils/sdk_crashes/sdk_crash_detection.py b/src/sentry/utils/sdk_crashes/sdk_crash_detection.py index 6e1610fb7a2645..725af3ef60474f 100644 --- a/src/sentry/utils/sdk_crashes/sdk_crash_detection.py +++ b/src/sentry/utils/sdk_crashes/sdk_crash_detection.py @@ -55,7 +55,9 @@ def detect_sdk_crash(self, event: Event) -> None: if self.cocoa_sdk_crash_detector.is_sdk_crash(frames): sdk_crash_event = self.event_stripper.strip_event_data(event) - set_path(sdk_crash_event, "contexts", "sdk_crash_detection", value={"detected": True}) + set_path( + sdk_crash_event.data, "contexts", "sdk_crash_detection", value={"detected": True} + ) self.sdk_crash_reporter.report(sdk_crash_event) diff --git a/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py b/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py index 3c2e4f18597145..d922c968ce2c22 100644 --- a/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py +++ b/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py @@ -33,7 +33,7 @@ def execute_test(self, event_data, should_be_reported, mock_sdk_crash_reporter): mock_sdk_crash_reporter.report.assert_called_once() reported_event = mock_sdk_crash_reporter.report.call_args.args[0] - assert reported_event["contexts"]["sdk_crash_detection"]["detected"] is True + assert reported_event.data["contexts"]["sdk_crash_detection"]["detected"] is True else: mock_sdk_crash_reporter.report.assert_not_called() From 995aa4e271773e3a900c0cf2aea3e34b545abd26 Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Tue, 16 May 2023 10:38:03 +0200 Subject: [PATCH 12/32] fix post processing test --- tests/sentry/tasks/test_post_process.py | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/tests/sentry/tasks/test_post_process.py b/tests/sentry/tasks/test_post_process.py index 05858a04deea3b..88425aef05bd10 100644 --- a/tests/sentry/tasks/test_post_process.py +++ b/tests/sentry/tasks/test_post_process.py @@ -64,6 +64,7 @@ from sentry.types.group import GroupSubStatus from sentry.utils.cache import cache from tests.sentry.issues.test_utils import OccurrenceTestMixin +from tests.sentry.utils.sdk_crashes.test_fixture import get_crash_event class EventMatcher: @@ -1436,15 +1437,11 @@ def test_maintains_valid_snooze(self, mock_processor): assert GroupSnooze.objects.filter(id=snooze.id).exists() +@patch("sentry.utils.sdk_crashes.sdk_crash_detection.sdk_crash_detection.sdk_crash_reporter") class SDKCrashMonitoringTestMixin(BasePostProgressGroupMixin): - # @patch("sentry.utils.sdk_crashes.sdk_crash_detection.sdk_crash_detection") - def test_sdk_crash_monitoring_is_called_with_event(self): + def test_sdk_crash_monitoring_is_called_with_event(self, mock_sdk_crash_reporter): event = self.create_event( - data={ - "message": "oh no", - "platform": "cocoa", - "stacktrace": {"frames": [{"filename": "src/app/example.py"}]}, - }, + data=get_crash_event(), project_id=self.project.id, ) @@ -1455,7 +1452,7 @@ def test_sdk_crash_monitoring_is_called_with_event(self): event=event, ) - # mock_sdk_crash_detection.detect_sdk_crash.assert_called_once() + mock_sdk_crash_reporter.report.assert_called_once() @region_silo_test From 6a21f54ea042555ad3cef5948d5e2646bd501f68 Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Fri, 19 May 2023 14:32:33 +0200 Subject: [PATCH 13/32] backup --- src/sentry/conf/server.py | 2 ++ src/sentry/features/__init__.py | 1 + src/sentry/utils/sdk_crashes/sdk_crash_detection.py | 3 +++ tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py | 3 +++ 4 files changed, 9 insertions(+) diff --git a/src/sentry/conf/server.py b/src/sentry/conf/server.py index af008ff9360633..c181c2e24922b9 100644 --- a/src/sentry/conf/server.py +++ b/src/sentry/conf/server.py @@ -1504,6 +1504,8 @@ def SOCIAL_AUTH_DEFAULT_USERNAME(): "organizations:ds-sliding-window": False, # If true certain Slack messages will be escaped to prevent rendering markdown "organizations:slack-escape-messages": False, + # Enable detecting sdk crashes during event processing + "organizations:sdk-crash-monitoring": False, # Adds additional filters and a new section to issue alert rules. "projects:alert-filters": True, # Enable functionality to specify custom inbound filters on events. diff --git a/src/sentry/features/__init__.py b/src/sentry/features/__init__.py index d1109cd0b49272..2653760448f2b7 100644 --- a/src/sentry/features/__init__.py +++ b/src/sentry/features/__init__.py @@ -178,6 +178,7 @@ default_manager.add("organizations:set-grouping-config", OrganizationFeature, FeatureHandlerStrategy.INTERNAL) default_manager.add("organizations:slack-escape-messages", OrganizationFeature, FeatureHandlerStrategy.INTERNAL) default_manager.add("organizations:slack-overage-notifications", OrganizationFeature, FeatureHandlerStrategy.REMOTE) +default_manager.add("organizations:sdk-crash-monitoring", OrganizationFeature, FeatureHandlerStrategy.REMOTE) default_manager.add("organizations:starfish-view", OrganizationFeature, FeatureHandlerStrategy.REMOTE) default_manager.add("organizations:starfish-test-endpoint", OrganizationFeature, FeatureHandlerStrategy.REMOTE) default_manager.add("organizations:streamline-targeting-context", OrganizationFeature, FeatureHandlerStrategy.REMOTE) diff --git a/src/sentry/utils/sdk_crashes/sdk_crash_detection.py b/src/sentry/utils/sdk_crashes/sdk_crash_detection.py index 725af3ef60474f..ba1cc584099ede 100644 --- a/src/sentry/utils/sdk_crashes/sdk_crash_detection.py +++ b/src/sentry/utils/sdk_crashes/sdk_crash_detection.py @@ -30,6 +30,9 @@ def __init__( def detect_sdk_crash(self, event: Event) -> None: + # if not features.has("organizations:sdk-crash-monitoring", event.group.project.organization) + # return + should_detect_sdk_crash = ( event.group and event.group.issue_category == GroupCategory.ERROR diff --git a/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py b/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py index d922c968ce2c22..5f640dbf4151ab 100644 --- a/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py +++ b/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py @@ -5,6 +5,7 @@ from sentry.issues.grouptype import PerformanceNPlusOneGroupType from sentry.testutils import TestCase from sentry.testutils.cases import BaseTestCase +from sentry.testutils.helpers.features import with_feature from sentry.testutils.performance_issues.store_transaction import PerfIssueTransactionTestMixin from sentry.testutils.silo import region_silo_test from sentry.utils.sdk_crashes.sdk_crash_detection import sdk_crash_detection @@ -38,6 +39,7 @@ def execute_test(self, event_data, should_be_reported, mock_sdk_crash_reporter): mock_sdk_crash_reporter.report.assert_not_called() +@with_feature("organizations:sdk-crash-monitoring") class PerformanceEventTestMixin(BaseSDKCrashDetectionMixin, PerfIssueTransactionTestMixin): @patch("sentry.utils.sdk_crashes.sdk_crash_detection.sdk_crash_detection.sdk_crash_reporter") def test_performance_event_not_detected(self, mock_sdk_crash_reporter): @@ -72,6 +74,7 @@ def test_wrong_platform_not_detected(self, mock_sdk_crash_reporter): def test_no_exception_not_detected(self, mock_sdk_crash_reporter): self.execute_test(get_crash_event(exception=[]), False, mock_sdk_crash_reporter) + @with_feature("organizations:sdk-crash-monitoring") def test_sdk_crash_detected_event_is_not_reported(self, mock_sdk_crash_reporter): event = get_crash_event() event["contexts"]["sdk_crash_detection"] = {"detected": True} From fdf61bcdfe8ef5669cb58bf8003590a0cce269ff Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Mon, 22 May 2023 08:55:00 +0200 Subject: [PATCH 14/32] add test --- .../utils/sdk_crashes/sdk_crash_detection.py | 5 ++- .../sdk_crashes/test_sdk_crash_detection.py | 41 +++++++++++-------- 2 files changed, 28 insertions(+), 18 deletions(-) diff --git a/src/sentry/utils/sdk_crashes/sdk_crash_detection.py b/src/sentry/utils/sdk_crashes/sdk_crash_detection.py index ba1cc584099ede..7159c439a999e3 100644 --- a/src/sentry/utils/sdk_crashes/sdk_crash_detection.py +++ b/src/sentry/utils/sdk_crashes/sdk_crash_detection.py @@ -1,5 +1,6 @@ from __future__ import annotations +from sentry import features from sentry.eventstore.models import Event from sentry.issues.grouptype import GroupCategory from sentry.utils.safe import get_path, set_path @@ -30,8 +31,8 @@ def __init__( def detect_sdk_crash(self, event: Event) -> None: - # if not features.has("organizations:sdk-crash-monitoring", event.group.project.organization) - # return + if not features.has("organizations:sdk-crash-monitoring", event.project.organization): + return should_detect_sdk_crash = ( event.group diff --git a/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py b/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py index 5f640dbf4151ab..1fe82a7f8ffd56 100644 --- a/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py +++ b/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py @@ -5,7 +5,7 @@ from sentry.issues.grouptype import PerformanceNPlusOneGroupType from sentry.testutils import TestCase from sentry.testutils.cases import BaseTestCase -from sentry.testutils.helpers.features import with_feature +from sentry.testutils.helpers.features import Feature from sentry.testutils.performance_issues.store_transaction import PerfIssueTransactionTestMixin from sentry.testutils.silo import region_silo_test from sentry.utils.sdk_crashes.sdk_crash_detection import sdk_crash_detection @@ -22,28 +22,35 @@ class BaseSDKCrashDetectionMixin(BaseTestCase, metaclass=abc.ABCMeta): def create_event(self, data, project_id, assert_no_errors=True): pass - def execute_test(self, event_data, should_be_reported, mock_sdk_crash_reporter): - event = self.create_event( - data=event_data, - project_id=self.project.id, - ) + def execute_test( + self, event_data, should_be_reported, mock_sdk_crash_reporter, feature_enabled=True + ): + def _execute_test(self): + event = self.create_event( + data=event_data, + project_id=self.project.id, + ) - sdk_crash_detection.detect_sdk_crash(event=event) + sdk_crash_detection.detect_sdk_crash(event=event) + + if should_be_reported: + mock_sdk_crash_reporter.report.assert_called_once() - if should_be_reported: - mock_sdk_crash_reporter.report.assert_called_once() + reported_event = mock_sdk_crash_reporter.report.call_args.args[0] + assert reported_event.data["contexts"]["sdk_crash_detection"]["detected"] is True + else: + mock_sdk_crash_reporter.report.assert_not_called() - reported_event = mock_sdk_crash_reporter.report.call_args.args[0] - assert reported_event.data["contexts"]["sdk_crash_detection"]["detected"] is True + if feature_enabled: + with Feature("organizations:sdk-crash-monitoring"): + _execute_test(self) else: - mock_sdk_crash_reporter.report.assert_not_called() + _execute_test(self) -@with_feature("organizations:sdk-crash-monitoring") +@patch("sentry.utils.sdk_crashes.sdk_crash_detection.sdk_crash_detection.sdk_crash_reporter") class PerformanceEventTestMixin(BaseSDKCrashDetectionMixin, PerfIssueTransactionTestMixin): - @patch("sentry.utils.sdk_crashes.sdk_crash_detection.sdk_crash_detection.sdk_crash_reporter") def test_performance_event_not_detected(self, mock_sdk_crash_reporter): - fingerprint = "some_group" fingerprint = f"{PerformanceNPlusOneGroupType.type_id}-{fingerprint}" event = self.store_transaction( @@ -62,6 +69,9 @@ class CococaSDKTestMixin(BaseSDKCrashDetectionMixin): def test_unhandled_is_detected(self, mock_sdk_crash_reporter): self.execute_test(get_crash_event(), True, mock_sdk_crash_reporter) + def test_feature_disabled_unhandled_is_not_detected(self, mock_sdk_crash_reporter): + self.execute_test(get_crash_event(), False, mock_sdk_crash_reporter, feature_enabled=False) + def test_handled_is_not_detected(self, mock_sdk_crash_reporter): self.execute_test(get_crash_event(handled=True), False, mock_sdk_crash_reporter) @@ -74,7 +84,6 @@ def test_wrong_platform_not_detected(self, mock_sdk_crash_reporter): def test_no_exception_not_detected(self, mock_sdk_crash_reporter): self.execute_test(get_crash_event(exception=[]), False, mock_sdk_crash_reporter) - @with_feature("organizations:sdk-crash-monitoring") def test_sdk_crash_detected_event_is_not_reported(self, mock_sdk_crash_reporter): event = get_crash_event() event["contexts"]["sdk_crash_detection"] = {"detected": True} From ebb759a3be10a0abed124ca83de8c81950531d7f Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Mon, 22 May 2023 08:56:00 +0200 Subject: [PATCH 15/32] fix feature comment --- src/sentry/conf/server.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sentry/conf/server.py b/src/sentry/conf/server.py index 60150cf1071586..998e66022da156 100644 --- a/src/sentry/conf/server.py +++ b/src/sentry/conf/server.py @@ -1505,7 +1505,7 @@ def SOCIAL_AUTH_DEFAULT_USERNAME(): "organizations:ds-sliding-window": False, # If true certain Slack messages will be escaped to prevent rendering markdown "organizations:slack-escape-messages": False, - # Enable detecting sdk crashes during event processing + # Enable detecting SDK crashes during event processing "organizations:sdk-crash-monitoring": False, # Adds additional filters and a new section to issue alert rules. "projects:alert-filters": True, From 8c08c4e873f6cf1420c76c6658cd56363c5bc0ad Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Mon, 22 May 2023 09:46:47 +0200 Subject: [PATCH 16/32] add project ID setting --- src/sentry/conf/server.py | 3 +++ src/sentry/utils/sdk_crashes/sdk_crash_detection.py | 11 ++++++++++- .../utils/sdk_crashes/test_sdk_crash_detection.py | 13 ++++++++++++- 3 files changed, 25 insertions(+), 2 deletions(-) diff --git a/src/sentry/conf/server.py b/src/sentry/conf/server.py index 998e66022da156..1ad4e76bcd85e6 100644 --- a/src/sentry/conf/server.py +++ b/src/sentry/conf/server.py @@ -3336,3 +3336,6 @@ def build_cdc_postgres_init_db_volume(settings): # Raise schema validation errors and make the indexer crash (only useful in # tests) SENTRY_METRICS_INDEXER_RAISE_VALIDATION_ERRORS = False + +# The project ID for SDK Crash Monitoring to save the detected SDK crashed to. +SDK_CRASH_MONITORING_PROJECT_ID = None diff --git a/src/sentry/utils/sdk_crashes/sdk_crash_detection.py b/src/sentry/utils/sdk_crashes/sdk_crash_detection.py index 7159c439a999e3..dd8f422923329a 100644 --- a/src/sentry/utils/sdk_crashes/sdk_crash_detection.py +++ b/src/sentry/utils/sdk_crashes/sdk_crash_detection.py @@ -1,5 +1,7 @@ from __future__ import annotations +from django.conf import settings + from sentry import features from sentry.eventstore.models import Event from sentry.issues.grouptype import GroupCategory @@ -31,6 +33,9 @@ def __init__( def detect_sdk_crash(self, event: Event) -> None: + if settings.SDK_CRASH_MONITORING_PROJECT_ID is None: + return + if not features.has("organizations:sdk-crash-monitoring", event.project.organization): return @@ -69,4 +74,8 @@ def detect_sdk_crash(self, event: Event) -> None: _cocoa_sdk_crash_detector = CocoaSDKCrashDetector() _event_stripper = EventStripper(sdk_crash_detector=_cocoa_sdk_crash_detector) -sdk_crash_detection = SDKCrashDetection(_crash_reporter, _cocoa_sdk_crash_detector, _event_stripper) +sdk_crash_detection = SDKCrashDetection( + _crash_reporter, + _cocoa_sdk_crash_detector, + _event_stripper, +) diff --git a/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py b/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py index 1fe82a7f8ffd56..30057e404f3fb4 100644 --- a/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py +++ b/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py @@ -2,6 +2,8 @@ from typing import Any, Mapping, Sequence from unittest.mock import patch +from django.test.utils import override_settings + from sentry.issues.grouptype import PerformanceNPlusOneGroupType from sentry.testutils import TestCase from sentry.testutils.cases import BaseTestCase @@ -23,8 +25,14 @@ def create_event(self, data, project_id, assert_no_errors=True): pass def execute_test( - self, event_data, should_be_reported, mock_sdk_crash_reporter, feature_enabled=True + self, + event_data, + should_be_reported, + mock_sdk_crash_reporter, + feature_enabled=True, + project_id=1234, ): + @override_settings(SDK_CRASH_MONITORING_PROJECT_ID=project_id) def _execute_test(self): event = self.create_event( data=event_data, @@ -72,6 +80,9 @@ def test_unhandled_is_detected(self, mock_sdk_crash_reporter): def test_feature_disabled_unhandled_is_not_detected(self, mock_sdk_crash_reporter): self.execute_test(get_crash_event(), False, mock_sdk_crash_reporter, feature_enabled=False) + def test_no_project_id_is_not_detected(self, mock_sdk_crash_reporter): + self.execute_test(get_crash_event(), False, mock_sdk_crash_reporter, project_id=None) + def test_handled_is_not_detected(self, mock_sdk_crash_reporter): self.execute_test(get_crash_event(handled=True), False, mock_sdk_crash_reporter) From 5dc542005605a5967e1606e5b812dbad7938a866 Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Mon, 22 May 2023 11:11:19 +0200 Subject: [PATCH 17/32] first version --- src/sentry/conf/server.py | 2 +- .../utils/sdk_crashes/event_stripper.py | 7 ++-- .../utils/sdk_crashes/sdk_crash_detection.py | 35 +++++++++------- .../utils/sdk_crashes/test_event_stripper.py | 36 +++++----------- .../sdk_crashes/test_sdk_crash_detection.py | 42 +++++++++++++++++-- 5 files changed, 74 insertions(+), 48 deletions(-) diff --git a/src/sentry/conf/server.py b/src/sentry/conf/server.py index 1ad4e76bcd85e6..807c923f60c337 100644 --- a/src/sentry/conf/server.py +++ b/src/sentry/conf/server.py @@ -3338,4 +3338,4 @@ def build_cdc_postgres_init_db_volume(settings): SENTRY_METRICS_INDEXER_RAISE_VALIDATION_ERRORS = False # The project ID for SDK Crash Monitoring to save the detected SDK crashed to. -SDK_CRASH_MONITORING_PROJECT_ID = None +SDK_CRASH_DETECTION_PROJECT_ID = None diff --git a/src/sentry/utils/sdk_crashes/event_stripper.py b/src/sentry/utils/sdk_crashes/event_stripper.py index fae6653cbb6406..4213364661a980 100644 --- a/src/sentry/utils/sdk_crashes/event_stripper.py +++ b/src/sentry/utils/sdk_crashes/event_stripper.py @@ -1,4 +1,4 @@ -from typing import Any, Mapping, Sequence +from typing import Any, Dict, Mapping, Sequence from sentry.eventstore.models import Event from sentry.utils.safe import get_path @@ -26,7 +26,7 @@ def __init__( "contexts", } - def strip_event_data(self, event: Event) -> Event: + def strip_event_data(self, event: Event) -> Dict[str, Any]: new_event_data = dict(filter(self._filter_event, event.data.items())) new_event_data["contexts"] = dict( filter(self._filter_contexts, new_event_data["contexts"].items()) @@ -44,8 +44,7 @@ def strip_event_data(self, event: Event) -> Event: stripped_debug_meta_images = self._strip_debug_meta(debug_meta_images, stripped_frames) new_event_data["debug_meta"]["images"] = stripped_debug_meta_images - event.data = new_event_data - return event + return new_event_data def _filter_event(self, pair): key, _ = pair diff --git a/src/sentry/utils/sdk_crashes/sdk_crash_detection.py b/src/sentry/utils/sdk_crashes/sdk_crash_detection.py index dd8f422923329a..1a416ea2ff28ef 100644 --- a/src/sentry/utils/sdk_crashes/sdk_crash_detection.py +++ b/src/sentry/utils/sdk_crashes/sdk_crash_detection.py @@ -1,5 +1,7 @@ from __future__ import annotations +from typing import Any, Dict + from django.conf import settings from sentry import features @@ -15,8 +17,11 @@ class SDKCrashReporter: def __init__(self): self - def report(self, event: Event) -> None: - return + def report(self, event_data: Dict[str, Any], event_project_id: int) -> Event: + from sentry.event_manager import EventManager + + manager = EventManager(event_data) + return manager.save(project_id=event_project_id) class SDKCrashDetection: @@ -31,13 +36,9 @@ def __init__( self.cocoa_sdk_crash_detector = sdk_crash_detector self.event_stripper = event_stripper - def detect_sdk_crash(self, event: Event) -> None: - - if settings.SDK_CRASH_MONITORING_PROJECT_ID is None: - return - + def detect_sdk_crash(self, event: Event) -> Event: if not features.has("organizations:sdk-crash-monitoring", event.project.organization): - return + return None should_detect_sdk_crash = ( event.group @@ -49,25 +50,31 @@ def detect_sdk_crash(self, event: Event) -> None: context = get_path(event.data, "contexts", "sdk_crash_detection") if context is not None and context.get("detected", False): - return + return None is_unhandled = ( get_path(event.data, "exception", "values", -1, "mechanism", "data", "handled") is False ) if is_unhandled is False: - return + return None frames = get_path(event.data, "exception", "values", -1, "stacktrace", "frames") if not frames: - return + return None if self.cocoa_sdk_crash_detector.is_sdk_crash(frames): - sdk_crash_event = self.event_stripper.strip_event_data(event) + sdk_crash_event_data = self.event_stripper.strip_event_data(event) set_path( - sdk_crash_event.data, "contexts", "sdk_crash_detection", value={"detected": True} + sdk_crash_event_data, "contexts", "sdk_crash_detection", value={"detected": True} + ) + + if settings.SDK_CRASH_DETECTION_PROJECT_ID is None: + return None + + return self.sdk_crash_reporter.report( + sdk_crash_event_data, settings.SDK_CRASH_DETECTION_PROJECT_ID ) - self.sdk_crash_reporter.report(sdk_crash_event) _crash_reporter = SDKCrashReporter() diff --git a/tests/sentry/utils/sdk_crashes/test_event_stripper.py b/tests/sentry/utils/sdk_crashes/test_event_stripper.py index e563528ebc2e3a..e78d34558a7b16 100644 --- a/tests/sentry/utils/sdk_crashes/test_event_stripper.py +++ b/tests/sentry/utils/sdk_crashes/test_event_stripper.py @@ -33,11 +33,11 @@ def test_strip_event_data_keeps_allowed_keys(self): project_id=self.project.id, ) - stripped_event = event_stripper.strip_event_data(event) + stripped_event_data = event_stripper.strip_event_data(event) keys_removed = {"tags", "user", "threads", "breadcrumbs", "environment"} for key in keys_removed: - assert stripped_event.data.get(key) is None, f"key {key} should be removed" + assert stripped_event_data.get(key) is None, f"key {key} should be removed" keys_kept = { "type", @@ -52,21 +52,7 @@ def test_strip_event_data_keeps_allowed_keys(self): } for key in keys_kept: - assert stripped_event.data.get(key) is not None, f"key {key} should be kept" - - def test_strip_event_data_removes_ip_address(self): - event_stripper = EventStripper(CocoaSDKCrashDetector()) - - event = self.create_event( - data=get_crash_event(), - project_id=self.project.id, - ) - - assert event.ip_address is not None - - stripped_event = event_stripper.strip_event_data(event) - - assert stripped_event.ip_address is None + assert stripped_event_data.get(key) is not None, f"key {key} should be kept" def test_strip_event_data_without_debug_meta(self): event_stripper = EventStripper(CocoaSDKCrashDetector()) @@ -79,9 +65,9 @@ def test_strip_event_data_without_debug_meta(self): project_id=self.project.id, ) - stripped_event = event_stripper.strip_event_data(event) + stripped_event_data = event_stripper.strip_event_data(event) - debug_meta_images = get_path(stripped_event.data, "debug_meta", "images") + debug_meta_images = get_path(stripped_event_data, "debug_meta", "images") assert debug_meta_images is None def test_strip_event_data_strips_context(self): @@ -92,9 +78,9 @@ def test_strip_event_data_strips_context(self): project_id=self.project.id, ) - stripped_event = event_stripper.strip_event_data(event) + stripped_event_data = event_stripper.strip_event_data(event) - contexts = stripped_event.data.get("contexts") + contexts = stripped_event_data.get("contexts") assert contexts is not None assert contexts.get("app") is None assert contexts.get("os") is not None @@ -108,9 +94,9 @@ def test_strip_event_data_strips_non_referenced_dsyms(self): project_id=self.project.id, ) - stripped_event = event_stripper.strip_event_data(event) + stripped_event_data = event_stripper.strip_event_data(event) - debug_meta_images = get_path(stripped_event.data, "debug_meta", "images") + debug_meta_images = get_path(stripped_event_data, "debug_meta", "images") image_addresses = set(map(lambda image: image["image_addr"], debug_meta_images)) expected_image_addresses = {"0x1a4e8f000", "0x100304000", "0x100260000"} @@ -134,10 +120,10 @@ def _execute_strip_frames_test(self, frames): project_id=self.project.id, ) - stripped_event = event_stripper.strip_event_data(event) + stripped_event_data = event_stripper.strip_event_data(event) stripped_frames = get_path( - stripped_event.data, "exception", "values", -1, "stacktrace", "frames" + stripped_event_data, "exception", "values", -1, "stacktrace", "frames" ) assert len(stripped_frames) == 6 diff --git a/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py b/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py index 30057e404f3fb4..8970a3be797eb0 100644 --- a/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py +++ b/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py @@ -2,11 +2,14 @@ from typing import Any, Mapping, Sequence from unittest.mock import patch +import pytest from django.test.utils import override_settings +from sentry.eventstore.snuba.backend import SnubaEventStorage from sentry.issues.grouptype import PerformanceNPlusOneGroupType from sentry.testutils import TestCase -from sentry.testutils.cases import BaseTestCase +from sentry.testutils.cases import BaseTestCase, SnubaTestCase +from sentry.testutils.helpers import with_feature from sentry.testutils.helpers.features import Feature from sentry.testutils.performance_issues.store_transaction import PerfIssueTransactionTestMixin from sentry.testutils.silo import region_silo_test @@ -32,7 +35,7 @@ def execute_test( feature_enabled=True, project_id=1234, ): - @override_settings(SDK_CRASH_MONITORING_PROJECT_ID=project_id) + @override_settings(SDK_CRASH_DETECTION_PROJECT_ID=project_id) def _execute_test(self): event = self.create_event( data=event_data, @@ -44,8 +47,8 @@ def _execute_test(self): if should_be_reported: mock_sdk_crash_reporter.report.assert_called_once() - reported_event = mock_sdk_crash_reporter.report.call_args.args[0] - assert reported_event.data["contexts"]["sdk_crash_detection"]["detected"] is True + reported_event_data = mock_sdk_crash_reporter.report.call_args.args[0] + assert reported_event_data["contexts"]["sdk_crash_detection"]["detected"] is True else: mock_sdk_crash_reporter.report.assert_not_called() @@ -310,6 +313,36 @@ def test_frames_only_in_app_after_sentry_frame_not_reported(self, mock_sdk_crash ) +class SDKCrashReportTestMixin(BaseSDKCrashDetectionMixin, SnubaTestCase): + @pytest.mark.django_db + @with_feature("organizations:sdk-crash-monitoring") + def test_event_stored_to_project(self): + eventstore = SnubaEventStorage() + + cocoa_sdk_crashes_project = self.create_project( + name="Cocoa SDK Crashes", + slug="cocoa-sdk-crashes", + teams=[self.team], + fire_project_created=True, + ) + + event = self.create_event( + data=get_crash_event(), + project_id=self.project.id, + ) + + with override_settings(SDK_CRASH_DETECTION_PROJECT_ID=cocoa_sdk_crashes_project.id): + sdk_crash_event = sdk_crash_detection.detect_sdk_crash(event=event) + + fetched_sdk_crash_event = eventstore.get_event_by_id( + cocoa_sdk_crashes_project.id, sdk_crash_event.event_id + ) + + assert sdk_crash_event.project_id == cocoa_sdk_crashes_project.id + + assert sdk_crash_event.event_id == fetched_sdk_crash_event.event_id + + @region_silo_test class SDKCrashDetectionTest( TestCase, @@ -318,6 +351,7 @@ class SDKCrashDetectionTest( CococaSDKFilenameTestMixin, CococaSDKFramesTestMixin, PerformanceEventTestMixin, + SDKCrashReportTestMixin, ): def create_event(self, data, project_id, assert_no_errors=True): return self.store_event(data=data, project_id=project_id, assert_no_errors=assert_no_errors) From 48f7f6f6aee21212192d747125b6ae33e4e42fc1 Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Mon, 22 May 2023 11:12:18 +0200 Subject: [PATCH 18/32] rename flag to crash-reporting --- src/sentry/conf/server.py | 2 +- src/sentry/features/__init__.py | 2 +- src/sentry/utils/sdk_crashes/sdk_crash_detection.py | 2 +- tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/sentry/conf/server.py b/src/sentry/conf/server.py index 998e66022da156..a3153d78fda731 100644 --- a/src/sentry/conf/server.py +++ b/src/sentry/conf/server.py @@ -1506,7 +1506,7 @@ def SOCIAL_AUTH_DEFAULT_USERNAME(): # If true certain Slack messages will be escaped to prevent rendering markdown "organizations:slack-escape-messages": False, # Enable detecting SDK crashes during event processing - "organizations:sdk-crash-monitoring": False, + "organizations:sdk-crash-reporting": False, # Adds additional filters and a new section to issue alert rules. "projects:alert-filters": True, # Enable functionality to specify custom inbound filters on events. diff --git a/src/sentry/features/__init__.py b/src/sentry/features/__init__.py index a2a01440fc969e..fdbe20b563b807 100644 --- a/src/sentry/features/__init__.py +++ b/src/sentry/features/__init__.py @@ -178,7 +178,7 @@ default_manager.add("organizations:set-grouping-config", OrganizationFeature, FeatureHandlerStrategy.INTERNAL) default_manager.add("organizations:slack-escape-messages", OrganizationFeature, FeatureHandlerStrategy.INTERNAL) default_manager.add("organizations:slack-overage-notifications", OrganizationFeature, FeatureHandlerStrategy.REMOTE) -default_manager.add("organizations:sdk-crash-monitoring", OrganizationFeature, FeatureHandlerStrategy.REMOTE) +default_manager.add("organizations:sdk-crash-reporting", OrganizationFeature, FeatureHandlerStrategy.REMOTE) default_manager.add("organizations:starfish-view", OrganizationFeature, FeatureHandlerStrategy.REMOTE) default_manager.add("organizations:starfish-test-endpoint", OrganizationFeature, FeatureHandlerStrategy.REMOTE) default_manager.add("organizations:streamline-targeting-context", OrganizationFeature, FeatureHandlerStrategy.REMOTE) diff --git a/src/sentry/utils/sdk_crashes/sdk_crash_detection.py b/src/sentry/utils/sdk_crashes/sdk_crash_detection.py index 7159c439a999e3..3333b1bb9451a6 100644 --- a/src/sentry/utils/sdk_crashes/sdk_crash_detection.py +++ b/src/sentry/utils/sdk_crashes/sdk_crash_detection.py @@ -31,7 +31,7 @@ def __init__( def detect_sdk_crash(self, event: Event) -> None: - if not features.has("organizations:sdk-crash-monitoring", event.project.organization): + if not features.has("organizations:sdk-crash-reporting", event.project.organization): return should_detect_sdk_crash = ( diff --git a/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py b/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py index 1fe82a7f8ffd56..75bea184b3f069 100644 --- a/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py +++ b/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py @@ -42,7 +42,7 @@ def _execute_test(self): mock_sdk_crash_reporter.report.assert_not_called() if feature_enabled: - with Feature("organizations:sdk-crash-monitoring"): + with Feature("organizations:sdk-crash-reporting"): _execute_test(self) else: _execute_test(self) From 17e6e567f434aa0cfcbf46f7b0472439398e88d4 Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Tue, 23 May 2023 09:08:54 +0200 Subject: [PATCH 19/32] Update src/sentry/features/__init__.py Co-authored-by: Joris Bayer --- src/sentry/features/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sentry/features/__init__.py b/src/sentry/features/__init__.py index fdbe20b563b807..8dcb55742239a1 100644 --- a/src/sentry/features/__init__.py +++ b/src/sentry/features/__init__.py @@ -178,7 +178,7 @@ default_manager.add("organizations:set-grouping-config", OrganizationFeature, FeatureHandlerStrategy.INTERNAL) default_manager.add("organizations:slack-escape-messages", OrganizationFeature, FeatureHandlerStrategy.INTERNAL) default_manager.add("organizations:slack-overage-notifications", OrganizationFeature, FeatureHandlerStrategy.REMOTE) -default_manager.add("organizations:sdk-crash-reporting", OrganizationFeature, FeatureHandlerStrategy.REMOTE) +default_manager.add("organizations:sdk-crash-reporting", OrganizationFeature, FeatureHandlerStrategy.INTERNAL) default_manager.add("organizations:starfish-view", OrganizationFeature, FeatureHandlerStrategy.REMOTE) default_manager.add("organizations:starfish-test-endpoint", OrganizationFeature, FeatureHandlerStrategy.REMOTE) default_manager.add("organizations:streamline-targeting-context", OrganizationFeature, FeatureHandlerStrategy.REMOTE) From 018caa558064a4a08cd17f08b1aae6aab694d9e4 Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Tue, 23 May 2023 09:54:00 +0200 Subject: [PATCH 20/32] fix failing test --- tests/sentry/tasks/test_post_process.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/sentry/tasks/test_post_process.py b/tests/sentry/tasks/test_post_process.py index a2a5c302ed7b87..2726b1c2568f6e 100644 --- a/tests/sentry/tasks/test_post_process.py +++ b/tests/sentry/tasks/test_post_process.py @@ -1450,6 +1450,7 @@ def test_maintains_valid_snooze(self, mock_processor): @patch("sentry.utils.sdk_crashes.sdk_crash_detection.sdk_crash_detection.sdk_crash_reporter") class SDKCrashMonitoringTestMixin(BasePostProgressGroupMixin): + @with_feature("organizations:sdk-crash-reporting") def test_sdk_crash_monitoring_is_called_with_event(self, mock_sdk_crash_reporter): event = self.create_event( data=get_crash_event(), From 79d3b401de4ae9eec0972fdfe982cc9d7bbd0937 Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Tue, 23 May 2023 10:02:01 +0200 Subject: [PATCH 21/32] fix test --- .../sentry/utils/sdk_crashes/test_sdk_crash_detection.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py b/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py index c7804fe264083d..06d620d65ce1a1 100644 --- a/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py +++ b/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py @@ -315,9 +315,8 @@ def test_frames_only_in_app_after_sentry_frame_not_reported(self, mock_sdk_crash class SDKCrashReportTestMixin(BaseSDKCrashDetectionMixin, SnubaTestCase): @pytest.mark.django_db - @with_feature("organizations:sdk-crash-monitoring") + @with_feature("organizations:sdk-crash-reporting") def test_event_stored_to_project(self): - eventstore = SnubaEventStorage() cocoa_sdk_crashes_project = self.create_project( name="Cocoa SDK Crashes", @@ -334,12 +333,14 @@ def test_event_stored_to_project(self): with override_settings(SDK_CRASH_DETECTION_PROJECT_ID=cocoa_sdk_crashes_project.id): sdk_crash_event = sdk_crash_detection.detect_sdk_crash(event=event) - fetched_sdk_crash_event = eventstore.get_event_by_id( + assert sdk_crash_event is not None + + event_store = SnubaEventStorage() + fetched_sdk_crash_event = event_store.get_event_by_id( cocoa_sdk_crashes_project.id, sdk_crash_event.event_id ) assert sdk_crash_event.project_id == cocoa_sdk_crashes_project.id - assert sdk_crash_event.event_id == fetched_sdk_crash_event.event_id From cf566c843207480fb2cccb2fe264a9ad9189a264 Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Tue, 23 May 2023 10:23:41 +0200 Subject: [PATCH 22/32] fix test --- tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py b/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py index 06d620d65ce1a1..1277a7fd4264db 100644 --- a/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py +++ b/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py @@ -316,7 +316,7 @@ def test_frames_only_in_app_after_sentry_frame_not_reported(self, mock_sdk_crash class SDKCrashReportTestMixin(BaseSDKCrashDetectionMixin, SnubaTestCase): @pytest.mark.django_db @with_feature("organizations:sdk-crash-reporting") - def test_event_stored_to_project(self): + def test_sdk_crash_event_stored_to_sdk_crash_project(self): cocoa_sdk_crashes_project = self.create_project( name="Cocoa SDK Crashes", @@ -340,7 +340,7 @@ def test_event_stored_to_project(self): cocoa_sdk_crashes_project.id, sdk_crash_event.event_id ) - assert sdk_crash_event.project_id == cocoa_sdk_crashes_project.id + assert cocoa_sdk_crashes_project.id == fetched_sdk_crash_event.project_id assert sdk_crash_event.event_id == fetched_sdk_crash_event.event_id From 9f791fcfb71d8a67e2077b91dafe8bb7f286ba07 Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Thu, 25 May 2023 09:34:18 +0200 Subject: [PATCH 23/32] Code review --- .../sentry/utils/sdk_crashes/test_sdk_crash_detection.py | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py b/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py index 75bea184b3f069..97f3b15074e428 100644 --- a/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py +++ b/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py @@ -25,7 +25,7 @@ def create_event(self, data, project_id, assert_no_errors=True): def execute_test( self, event_data, should_be_reported, mock_sdk_crash_reporter, feature_enabled=True ): - def _execute_test(self): + with Feature({"organizations:sdk-crash-reporting": feature_enabled}): event = self.create_event( data=event_data, project_id=self.project.id, @@ -41,12 +41,6 @@ def _execute_test(self): else: mock_sdk_crash_reporter.report.assert_not_called() - if feature_enabled: - with Feature("organizations:sdk-crash-reporting"): - _execute_test(self) - else: - _execute_test(self) - @patch("sentry.utils.sdk_crashes.sdk_crash_detection.sdk_crash_detection.sdk_crash_reporter") class PerformanceEventTestMixin(BaseSDKCrashDetectionMixin, PerfIssueTransactionTestMixin): From dbbab1ce888425c662fbe63e96b3f403f4871df6 Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Thu, 25 May 2023 10:36:51 +0200 Subject: [PATCH 24/32] fix test_sdk_crash_monitoring_is_called_with_event --- tests/sentry/tasks/test_post_process.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/sentry/tasks/test_post_process.py b/tests/sentry/tasks/test_post_process.py index a4b4ddbeeb4bd7..848e13740df4b6 100644 --- a/tests/sentry/tasks/test_post_process.py +++ b/tests/sentry/tasks/test_post_process.py @@ -1485,6 +1485,7 @@ def test_forecast_in_activity(self, mock_is_escalating): @patch("sentry.utils.sdk_crashes.sdk_crash_detection.sdk_crash_detection.sdk_crash_reporter") class SDKCrashMonitoringTestMixin(BasePostProgressGroupMixin): @with_feature("organizations:sdk-crash-reporting") + @override_settings(SDK_CRASH_DETECTION_PROJECT_ID=1234) def test_sdk_crash_monitoring_is_called_with_event(self, mock_sdk_crash_reporter): event = self.create_event( data=get_crash_event(), From f3720c0491177a2b4e4cb311422f25b6761fdf05 Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Tue, 30 May 2023 10:20:30 +0200 Subject: [PATCH 25/32] Move feature check to post_process --- src/sentry/tasks/post_process.py | 3 ++ .../utils/sdk_crashes/sdk_crash_detection.py | 4 --- tests/sentry/tasks/test_post_process.py | 20 +++++++++++- .../sdk_crashes/test_sdk_crash_detection.py | 32 ++++++++----------- 4 files changed, 35 insertions(+), 24 deletions(-) diff --git a/src/sentry/tasks/post_process.py b/src/sentry/tasks/post_process.py index 48f0900666a107..69b0e151192a08 100644 --- a/src/sentry/tasks/post_process.py +++ b/src/sentry/tasks/post_process.py @@ -1017,6 +1017,9 @@ def sdk_crash_monitoring(job: PostProcessJob): event = job["event"] + if not features.has("organizations:sdk-crash-reporting", event.project.organization): + return + with metrics.timer("post_process.sdk_crash_monitoring.duration"): with sentry_sdk.start_span(op="tasks.post_process_group.sdk_crash_monitoring"): sdk_crash_detection.detect_sdk_crash(event=event) diff --git a/src/sentry/utils/sdk_crashes/sdk_crash_detection.py b/src/sentry/utils/sdk_crashes/sdk_crash_detection.py index 3333b1bb9451a6..725af3ef60474f 100644 --- a/src/sentry/utils/sdk_crashes/sdk_crash_detection.py +++ b/src/sentry/utils/sdk_crashes/sdk_crash_detection.py @@ -1,6 +1,5 @@ from __future__ import annotations -from sentry import features from sentry.eventstore.models import Event from sentry.issues.grouptype import GroupCategory from sentry.utils.safe import get_path, set_path @@ -31,9 +30,6 @@ def __init__( def detect_sdk_crash(self, event: Event) -> None: - if not features.has("organizations:sdk-crash-reporting", event.project.organization): - return - should_detect_sdk_crash = ( event.group and event.group.issue_category == GroupCategory.ERROR diff --git a/tests/sentry/tasks/test_post_process.py b/tests/sentry/tasks/test_post_process.py index a4b4ddbeeb4bd7..24bff08b901601 100644 --- a/tests/sentry/tasks/test_post_process.py +++ b/tests/sentry/tasks/test_post_process.py @@ -1482,9 +1482,9 @@ def test_forecast_in_activity(self, mock_is_escalating): ).exists() -@patch("sentry.utils.sdk_crashes.sdk_crash_detection.sdk_crash_detection.sdk_crash_reporter") class SDKCrashMonitoringTestMixin(BasePostProgressGroupMixin): @with_feature("organizations:sdk-crash-reporting") + @patch("sentry.utils.sdk_crashes.sdk_crash_detection.sdk_crash_detection.sdk_crash_reporter") def test_sdk_crash_monitoring_is_called_with_event(self, mock_sdk_crash_reporter): event = self.create_event( data=get_crash_event(), @@ -1500,6 +1500,24 @@ def test_sdk_crash_monitoring_is_called_with_event(self, mock_sdk_crash_reporter mock_sdk_crash_reporter.report.assert_called_once() + @patch("sentry.utils.sdk_crashes.sdk_crash_detection.sdk_crash_detection") + def test_sdk_crash_monitoring_is_not_called_with_disabled_feature( + self, mock_sdk_crash_detection + ): + event = self.create_event( + data=get_crash_event(), + project_id=self.project.id, + ) + + self.call_post_process_group( + is_new=True, + is_regression=False, + is_new_group_environment=True, + event=event, + ) + + mock_sdk_crash_detection.detect_sdk_crash.assert_not_called() + @region_silo_test class PostProcessGroupErrorTest( diff --git a/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py b/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py index 97f3b15074e428..3da71d5bbbe26e 100644 --- a/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py +++ b/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py @@ -5,7 +5,6 @@ from sentry.issues.grouptype import PerformanceNPlusOneGroupType from sentry.testutils import TestCase from sentry.testutils.cases import BaseTestCase -from sentry.testutils.helpers.features import Feature from sentry.testutils.performance_issues.store_transaction import PerfIssueTransactionTestMixin from sentry.testutils.silo import region_silo_test from sentry.utils.sdk_crashes.sdk_crash_detection import sdk_crash_detection @@ -22,24 +21,22 @@ class BaseSDKCrashDetectionMixin(BaseTestCase, metaclass=abc.ABCMeta): def create_event(self, data, project_id, assert_no_errors=True): pass - def execute_test( - self, event_data, should_be_reported, mock_sdk_crash_reporter, feature_enabled=True - ): - with Feature({"organizations:sdk-crash-reporting": feature_enabled}): - event = self.create_event( - data=event_data, - project_id=self.project.id, - ) + def execute_test(self, event_data, should_be_reported, mock_sdk_crash_reporter): - sdk_crash_detection.detect_sdk_crash(event=event) + event = self.create_event( + data=event_data, + project_id=self.project.id, + ) - if should_be_reported: - mock_sdk_crash_reporter.report.assert_called_once() + sdk_crash_detection.detect_sdk_crash(event=event) - reported_event = mock_sdk_crash_reporter.report.call_args.args[0] - assert reported_event.data["contexts"]["sdk_crash_detection"]["detected"] is True - else: - mock_sdk_crash_reporter.report.assert_not_called() + if should_be_reported: + mock_sdk_crash_reporter.report.assert_called_once() + + reported_event = mock_sdk_crash_reporter.report.call_args.args[0] + assert reported_event.data["contexts"]["sdk_crash_detection"]["detected"] is True + else: + mock_sdk_crash_reporter.report.assert_not_called() @patch("sentry.utils.sdk_crashes.sdk_crash_detection.sdk_crash_detection.sdk_crash_reporter") @@ -63,9 +60,6 @@ class CococaSDKTestMixin(BaseSDKCrashDetectionMixin): def test_unhandled_is_detected(self, mock_sdk_crash_reporter): self.execute_test(get_crash_event(), True, mock_sdk_crash_reporter) - def test_feature_disabled_unhandled_is_not_detected(self, mock_sdk_crash_reporter): - self.execute_test(get_crash_event(), False, mock_sdk_crash_reporter, feature_enabled=False) - def test_handled_is_not_detected(self, mock_sdk_crash_reporter): self.execute_test(get_crash_event(handled=True), False, mock_sdk_crash_reporter) From 04d112f964bcad3b709b5090212e256d3aa1cfa5 Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Tue, 30 May 2023 10:21:44 +0200 Subject: [PATCH 26/32] rename feature to detection --- src/sentry/conf/server.py | 2 +- src/sentry/features/__init__.py | 2 +- src/sentry/tasks/post_process.py | 2 +- tests/sentry/tasks/test_post_process.py | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/sentry/conf/server.py b/src/sentry/conf/server.py index 1da694484979c6..68badaf4c1273d 100644 --- a/src/sentry/conf/server.py +++ b/src/sentry/conf/server.py @@ -1505,7 +1505,7 @@ def SOCIAL_AUTH_DEFAULT_USERNAME(): # If true certain Slack messages will be escaped to prevent rendering markdown "organizations:slack-escape-messages": False, # Enable detecting SDK crashes during event processing - "organizations:sdk-crash-reporting": False, + "organizations:sdk-crash-detection": False, # Adds additional filters and a new section to issue alert rules. "projects:alert-filters": True, # Enable functionality to specify custom inbound filters on events. diff --git a/src/sentry/features/__init__.py b/src/sentry/features/__init__.py index 5b12e394f7579f..b3d6d376415b66 100644 --- a/src/sentry/features/__init__.py +++ b/src/sentry/features/__init__.py @@ -175,7 +175,7 @@ default_manager.add("organizations:set-grouping-config", OrganizationFeature, FeatureHandlerStrategy.INTERNAL) default_manager.add("organizations:slack-escape-messages", OrganizationFeature, FeatureHandlerStrategy.INTERNAL) default_manager.add("organizations:slack-overage-notifications", OrganizationFeature, FeatureHandlerStrategy.REMOTE) -default_manager.add("organizations:sdk-crash-reporting", OrganizationFeature, FeatureHandlerStrategy.INTERNAL) +default_manager.add("organizations:sdk-crash-detection", OrganizationFeature, FeatureHandlerStrategy.INTERNAL) default_manager.add("organizations:starfish-view", OrganizationFeature, FeatureHandlerStrategy.REMOTE) default_manager.add("organizations:starfish-test-endpoint", OrganizationFeature, FeatureHandlerStrategy.REMOTE) default_manager.add("organizations:streamline-targeting-context", OrganizationFeature, FeatureHandlerStrategy.REMOTE) diff --git a/src/sentry/tasks/post_process.py b/src/sentry/tasks/post_process.py index 69b0e151192a08..79e92c24637e3c 100644 --- a/src/sentry/tasks/post_process.py +++ b/src/sentry/tasks/post_process.py @@ -1017,7 +1017,7 @@ def sdk_crash_monitoring(job: PostProcessJob): event = job["event"] - if not features.has("organizations:sdk-crash-reporting", event.project.organization): + if not features.has("organizations:sdk-crash-detection", event.project.organization): return with metrics.timer("post_process.sdk_crash_monitoring.duration"): diff --git a/tests/sentry/tasks/test_post_process.py b/tests/sentry/tasks/test_post_process.py index 24bff08b901601..66f21803c2821e 100644 --- a/tests/sentry/tasks/test_post_process.py +++ b/tests/sentry/tasks/test_post_process.py @@ -1483,7 +1483,7 @@ def test_forecast_in_activity(self, mock_is_escalating): class SDKCrashMonitoringTestMixin(BasePostProgressGroupMixin): - @with_feature("organizations:sdk-crash-reporting") + @with_feature("organizations:sdk-crash-detection") @patch("sentry.utils.sdk_crashes.sdk_crash_detection.sdk_crash_detection.sdk_crash_reporter") def test_sdk_crash_monitoring_is_called_with_event(self, mock_sdk_crash_reporter): event = self.create_event( From 172e89dcaf710fbe932296a8eec929e769616019 Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Tue, 30 May 2023 10:35:47 +0200 Subject: [PATCH 27/32] Update src/sentry/utils/sdk_crashes/sdk_crash_detection.py Co-authored-by: Joris Bayer --- src/sentry/utils/sdk_crashes/sdk_crash_detection.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sentry/utils/sdk_crashes/sdk_crash_detection.py b/src/sentry/utils/sdk_crashes/sdk_crash_detection.py index 725af3ef60474f..32da137124274c 100644 --- a/src/sentry/utils/sdk_crashes/sdk_crash_detection.py +++ b/src/sentry/utils/sdk_crashes/sdk_crash_detection.py @@ -35,7 +35,7 @@ def detect_sdk_crash(self, event: Event) -> None: and event.group.issue_category == GroupCategory.ERROR and event.group.platform == "cocoa" ) - if should_detect_sdk_crash is False: + if not should_detect_sdk_crash: return context = get_path(event.data, "contexts", "sdk_crash_detection") From a7bde15d0f79d5b773ad79ac7b7e9ca88f93b801 Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Tue, 30 May 2023 10:46:19 +0200 Subject: [PATCH 28/32] turn event_stripper into a function --- .../utils/sdk_crashes/event_stripper.py | 142 +++++++++--------- .../utils/sdk_crashes/sdk_crash_detection.py | 9 +- .../utils/sdk_crashes/test_event_stripper.py | 26 +--- 3 files changed, 78 insertions(+), 99 deletions(-) diff --git a/src/sentry/utils/sdk_crashes/event_stripper.py b/src/sentry/utils/sdk_crashes/event_stripper.py index fae6653cbb6406..0a8f0bba0cef72 100644 --- a/src/sentry/utils/sdk_crashes/event_stripper.py +++ b/src/sentry/utils/sdk_crashes/event_stripper.py @@ -4,78 +4,72 @@ from sentry.utils.safe import get_path from sentry.utils.sdk_crashes.sdk_crash_detector import SDKCrashDetector +ALLOWED_EVENT_KEYS = { + "type", + "datetime", + "timestamp", + "platform", + "sdk", + "level", + "logger", + "exception", + "debug_meta", + "contexts", +} -class EventStripper: - def __init__( - self, - sdk_crash_detector: SDKCrashDetector, - ): - self - self.sdk_crash_detector = sdk_crash_detector - - ALLOWED_EVENT_KEYS = { - "type", - "datetime", - "timestamp", - "platform", - "sdk", - "level", - "logger", - "exception", - "debug_meta", - "contexts", - } - - def strip_event_data(self, event: Event) -> Event: - new_event_data = dict(filter(self._filter_event, event.data.items())) - new_event_data["contexts"] = dict( - filter(self._filter_contexts, new_event_data["contexts"].items()) - ) - - stripped_frames = [] - frames = get_path(new_event_data, "exception", "values", -1, "stacktrace", "frames") - - if frames is not None: - stripped_frames = self._strip_frames(frames) - new_event_data["exception"]["values"][0]["stacktrace"]["frames"] = stripped_frames - - debug_meta_images = get_path(new_event_data, "debug_meta", "images") - if debug_meta_images is not None: - stripped_debug_meta_images = self._strip_debug_meta(debug_meta_images, stripped_frames) - new_event_data["debug_meta"]["images"] = stripped_debug_meta_images - - event.data = new_event_data - return event - - def _filter_event(self, pair): - key, _ = pair - if key in self.ALLOWED_EVENT_KEYS: - return True - - return False - - def _filter_contexts(self, pair): - key, _ = pair - if key in {"os", "device"}: - return True - return False - - def _strip_debug_meta( - self, debug_meta_images: Sequence[Mapping[str, Any]], frames: Sequence[Mapping[str, Any]] - ) -> Sequence[Mapping[str, Any]]: - - frame_image_addresses = {frame["image_addr"] for frame in frames} - - return [ - image for image in debug_meta_images if image["image_addr"] in frame_image_addresses - ] - - def _strip_frames(self, frames: Sequence[Mapping[str, Any]]) -> Sequence[Mapping[str, Any]]: - """ - Only keep SDK frames or non in app frames. - """ - return [ - frame - for frame in frames - if self.sdk_crash_detector.is_sdk_frame(frame) or frame.get("in_app", None) is False - ] + +def strip_event_data(event: Event, sdk_crash_detector: SDKCrashDetector) -> Event: + new_event_data = dict(filter(_filter_event, event.data.items())) + new_event_data["contexts"] = dict(filter(_filter_contexts, new_event_data["contexts"].items())) + + stripped_frames = [] + frames = get_path(new_event_data, "exception", "values", -1, "stacktrace", "frames") + + if frames is not None: + stripped_frames = _strip_frames(frames, sdk_crash_detector) + new_event_data["exception"]["values"][0]["stacktrace"]["frames"] = stripped_frames + + debug_meta_images = get_path(new_event_data, "debug_meta", "images") + if debug_meta_images is not None: + stripped_debug_meta_images = _strip_debug_meta(debug_meta_images, stripped_frames) + new_event_data["debug_meta"]["images"] = stripped_debug_meta_images + + event.data = new_event_data + return event + + +def _filter_event(pair): + key, _ = pair + if key in ALLOWED_EVENT_KEYS: + return True + + return False + + +def _filter_contexts(pair): + key, _ = pair + if key in {"os", "device"}: + return True + return False + + +def _strip_debug_meta( + debug_meta_images: Sequence[Mapping[str, Any]], frames: Sequence[Mapping[str, Any]] +) -> Sequence[Mapping[str, Any]]: + + frame_image_addresses = {frame["image_addr"] for frame in frames} + + return [image for image in debug_meta_images if image["image_addr"] in frame_image_addresses] + + +def _strip_frames( + frames: Sequence[Mapping[str, Any]], sdk_crash_detector: SDKCrashDetector +) -> Sequence[Mapping[str, Any]]: + """ + Only keep SDK frames or non in app frames. + """ + return [ + frame + for frame in frames + if sdk_crash_detector.is_sdk_frame(frame) or frame.get("in_app", None) is False + ] diff --git a/src/sentry/utils/sdk_crashes/sdk_crash_detection.py b/src/sentry/utils/sdk_crashes/sdk_crash_detection.py index 32da137124274c..ad7da9de592258 100644 --- a/src/sentry/utils/sdk_crashes/sdk_crash_detection.py +++ b/src/sentry/utils/sdk_crashes/sdk_crash_detection.py @@ -4,7 +4,7 @@ from sentry.issues.grouptype import GroupCategory from sentry.utils.safe import get_path, set_path from sentry.utils.sdk_crashes.cocoa_sdk_crash_detector import CocoaSDKCrashDetector -from sentry.utils.sdk_crashes.event_stripper import EventStripper +from sentry.utils.sdk_crashes.event_stripper import strip_event_data from sentry.utils.sdk_crashes.sdk_crash_detector import SDKCrashDetector @@ -21,12 +21,10 @@ def __init__( self, sdk_crash_reporter: SDKCrashReporter, sdk_crash_detector: SDKCrashDetector, - event_stripper: EventStripper, ): self self.sdk_crash_reporter = sdk_crash_reporter self.cocoa_sdk_crash_detector = sdk_crash_detector - self.event_stripper = event_stripper def detect_sdk_crash(self, event: Event) -> None: @@ -53,7 +51,7 @@ def detect_sdk_crash(self, event: Event) -> None: return if self.cocoa_sdk_crash_detector.is_sdk_crash(frames): - sdk_crash_event = self.event_stripper.strip_event_data(event) + sdk_crash_event = strip_event_data(event, self.cocoa_sdk_crash_detector) set_path( sdk_crash_event.data, "contexts", "sdk_crash_detection", value={"detected": True} @@ -63,6 +61,5 @@ def detect_sdk_crash(self, event: Event) -> None: _crash_reporter = SDKCrashReporter() _cocoa_sdk_crash_detector = CocoaSDKCrashDetector() -_event_stripper = EventStripper(sdk_crash_detector=_cocoa_sdk_crash_detector) -sdk_crash_detection = SDKCrashDetection(_crash_reporter, _cocoa_sdk_crash_detector, _event_stripper) +sdk_crash_detection = SDKCrashDetection(_crash_reporter, _cocoa_sdk_crash_detector) diff --git a/tests/sentry/utils/sdk_crashes/test_event_stripper.py b/tests/sentry/utils/sdk_crashes/test_event_stripper.py index e563528ebc2e3a..4eadc5077ae0c6 100644 --- a/tests/sentry/utils/sdk_crashes/test_event_stripper.py +++ b/tests/sentry/utils/sdk_crashes/test_event_stripper.py @@ -6,7 +6,7 @@ from sentry.testutils.silo import region_silo_test from sentry.utils.safe import get_path from sentry.utils.sdk_crashes.cocoa_sdk_crash_detector import CocoaSDKCrashDetector -from sentry.utils.sdk_crashes.event_stripper import EventStripper +from sentry.utils.sdk_crashes.event_stripper import strip_event_data from tests.sentry.utils.sdk_crashes.test_fixture import ( IN_APP_FRAME, get_crash_event, @@ -26,14 +26,12 @@ def execute_test(self, event_data, should_be_reported, mock_sdk_crash_reporter): class EventStripperTestMixin(BaseEventStripperMixin): def test_strip_event_data_keeps_allowed_keys(self): - event_stripper = EventStripper(CocoaSDKCrashDetector()) - event = self.create_event( data=get_crash_event(), project_id=self.project.id, ) - stripped_event = event_stripper.strip_event_data(event) + stripped_event = strip_event_data(event, CocoaSDKCrashDetector()) keys_removed = {"tags", "user", "threads", "breadcrumbs", "environment"} for key in keys_removed: @@ -55,8 +53,6 @@ def test_strip_event_data_keeps_allowed_keys(self): assert stripped_event.data.get(key) is not None, f"key {key} should be kept" def test_strip_event_data_removes_ip_address(self): - event_stripper = EventStripper(CocoaSDKCrashDetector()) - event = self.create_event( data=get_crash_event(), project_id=self.project.id, @@ -64,13 +60,11 @@ def test_strip_event_data_removes_ip_address(self): assert event.ip_address is not None - stripped_event = event_stripper.strip_event_data(event) + stripped_event = strip_event_data(event, CocoaSDKCrashDetector()) assert stripped_event.ip_address is None def test_strip_event_data_without_debug_meta(self): - event_stripper = EventStripper(CocoaSDKCrashDetector()) - event_data = get_crash_event() event_data["debug_meta"]["images"] = None @@ -79,20 +73,18 @@ def test_strip_event_data_without_debug_meta(self): project_id=self.project.id, ) - stripped_event = event_stripper.strip_event_data(event) + stripped_event = strip_event_data(event, CocoaSDKCrashDetector()) debug_meta_images = get_path(stripped_event.data, "debug_meta", "images") assert debug_meta_images is None def test_strip_event_data_strips_context(self): - event_stripper = EventStripper(CocoaSDKCrashDetector()) - event = self.create_event( data=get_crash_event(), project_id=self.project.id, ) - stripped_event = event_stripper.strip_event_data(event) + stripped_event = strip_event_data(event, CocoaSDKCrashDetector()) contexts = stripped_event.data.get("contexts") assert contexts is not None @@ -101,14 +93,12 @@ def test_strip_event_data_strips_context(self): assert contexts.get("device") is not None def test_strip_event_data_strips_non_referenced_dsyms(self): - event_stripper = EventStripper(Mock()) - event = self.create_event( data=get_crash_event(), project_id=self.project.id, ) - stripped_event = event_stripper.strip_event_data(event) + stripped_event = strip_event_data(event, Mock()) debug_meta_images = get_path(stripped_event.data, "debug_meta", "images") @@ -125,8 +115,6 @@ def test_strip_frames_sentry_non_in_app_frame_kept(self): self._execute_strip_frames_test(frames) def _execute_strip_frames_test(self, frames): - event_stripper = EventStripper(CocoaSDKCrashDetector()) - event_data = get_crash_event_with_frames(frames) event = self.create_event( @@ -134,7 +122,7 @@ def _execute_strip_frames_test(self, frames): project_id=self.project.id, ) - stripped_event = event_stripper.strip_event_data(event) + stripped_event = strip_event_data(event, CocoaSDKCrashDetector()) stripped_frames = get_path( stripped_event.data, "exception", "values", -1, "stacktrace", "frames" From 4cf65602d9327fa5d95ee11963fa2696cb7139ca Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Tue, 30 May 2023 11:25:23 +0200 Subject: [PATCH 29/32] move project id check to post processing --- src/sentry/tasks/post_process.py | 6 ++++++ .../utils/sdk_crashes/sdk_crash_detection.py | 3 --- tests/sentry/tasks/test_post_process.py | 17 +++++++++++++++++ .../sdk_crashes/test_sdk_crash_detection.py | 6 +----- 4 files changed, 24 insertions(+), 8 deletions(-) diff --git a/src/sentry/tasks/post_process.py b/src/sentry/tasks/post_process.py index 79e92c24637e3c..7de4c74f03d66f 100644 --- a/src/sentry/tasks/post_process.py +++ b/src/sentry/tasks/post_process.py @@ -1020,6 +1020,12 @@ def sdk_crash_monitoring(job: PostProcessJob): if not features.has("organizations:sdk-crash-detection", event.project.organization): return + if settings.SDK_CRASH_DETECTION_PROJECT_ID is None: + logger.warning( + "SDK crash detection is enabled but SDK_CRASH_DETECTION_PROJECT_ID is not set" + ) + return None + with metrics.timer("post_process.sdk_crash_monitoring.duration"): with sentry_sdk.start_span(op="tasks.post_process_group.sdk_crash_monitoring"): sdk_crash_detection.detect_sdk_crash(event=event) diff --git a/src/sentry/utils/sdk_crashes/sdk_crash_detection.py b/src/sentry/utils/sdk_crashes/sdk_crash_detection.py index 314d8966594680..959977c4b0d089 100644 --- a/src/sentry/utils/sdk_crashes/sdk_crash_detection.py +++ b/src/sentry/utils/sdk_crashes/sdk_crash_detection.py @@ -63,9 +63,6 @@ def detect_sdk_crash(self, event: Event) -> Event: sdk_crash_event_data, "contexts", "sdk_crash_detection", value={"detected": True} ) - if settings.SDK_CRASH_DETECTION_PROJECT_ID is None: - return None - return self.sdk_crash_reporter.report( sdk_crash_event_data, settings.SDK_CRASH_DETECTION_PROJECT_ID ) diff --git a/tests/sentry/tasks/test_post_process.py b/tests/sentry/tasks/test_post_process.py index 7b9fec538ca42f..c7454f2366e4da 100644 --- a/tests/sentry/tasks/test_post_process.py +++ b/tests/sentry/tasks/test_post_process.py @@ -1519,6 +1519,23 @@ def test_sdk_crash_monitoring_is_not_called_with_disabled_feature( mock_sdk_crash_detection.detect_sdk_crash.assert_not_called() + @with_feature("organizations:sdk-crash-detection") + @patch("sentry.utils.sdk_crashes.sdk_crash_detection.sdk_crash_detection") + def test_sdk_crash_monitoring_is_not_called_without_project_id(self, mock_sdk_crash_detection): + event = self.create_event( + data=get_crash_event(), + project_id=self.project.id, + ) + + self.call_post_process_group( + is_new=True, + is_regression=False, + is_new_group_environment=True, + event=event, + ) + + mock_sdk_crash_detection.detect_sdk_crash.assert_not_called() + @region_silo_test class PostProcessGroupErrorTest( diff --git a/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py b/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py index a4f177e3c0d25b..3b5538b9844bc9 100644 --- a/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py +++ b/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py @@ -31,9 +31,8 @@ def execute_test( event_data, should_be_reported, mock_sdk_crash_reporter, - project_id=1234, ): - with override_settings(SDK_CRASH_DETECTION_PROJECT_ID=project_id): + with override_settings(SDK_CRASH_DETECTION_PROJECT_ID=1234): event = self.create_event( data=event_data, project_id=self.project.id, @@ -71,9 +70,6 @@ class CococaSDKTestMixin(BaseSDKCrashDetectionMixin): def test_unhandled_is_detected(self, mock_sdk_crash_reporter): self.execute_test(get_crash_event(), True, mock_sdk_crash_reporter) - def test_no_project_id_is_not_detected(self, mock_sdk_crash_reporter): - self.execute_test(get_crash_event(), False, mock_sdk_crash_reporter, project_id=None) - def test_handled_is_not_detected(self, mock_sdk_crash_reporter): self.execute_test(get_crash_event(handled=True), False, mock_sdk_crash_reporter) From 541820459c82fbb3dc44d13264cece971491c6e6 Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Tue, 30 May 2023 11:31:35 +0200 Subject: [PATCH 30/32] Update src/sentry/utils/sdk_crashes/sdk_crash_detection.py Co-authored-by: Joris Bayer --- src/sentry/utils/sdk_crashes/sdk_crash_detection.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/sentry/utils/sdk_crashes/sdk_crash_detection.py b/src/sentry/utils/sdk_crashes/sdk_crash_detection.py index 959977c4b0d089..28fd10da02aac9 100644 --- a/src/sentry/utils/sdk_crashes/sdk_crash_detection.py +++ b/src/sentry/utils/sdk_crashes/sdk_crash_detection.py @@ -20,6 +20,7 @@ def report(self, event_data: Dict[str, Any], event_project_id: int) -> Event: from sentry.event_manager import EventManager manager = EventManager(event_data) + manager.normalize() return manager.save(project_id=event_project_id) From c10affea4ea063a9e4764ec0f560c40ebc91f76b Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Tue, 30 May 2023 11:33:52 +0200 Subject: [PATCH 31/32] cleanup --- src/sentry/tasks/post_process.py | 2 +- src/sentry/utils/sdk_crashes/sdk_crash_detection.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/sentry/tasks/post_process.py b/src/sentry/tasks/post_process.py index 7de4c74f03d66f..15954a8919d5e7 100644 --- a/src/sentry/tasks/post_process.py +++ b/src/sentry/tasks/post_process.py @@ -1022,7 +1022,7 @@ def sdk_crash_monitoring(job: PostProcessJob): if settings.SDK_CRASH_DETECTION_PROJECT_ID is None: logger.warning( - "SDK crash detection is enabled but SDK_CRASH_DETECTION_PROJECT_ID is not set" + "SDK crash detection is enabled but SDK_CRASH_DETECTION_PROJECT_ID is not set." ) return None diff --git a/src/sentry/utils/sdk_crashes/sdk_crash_detection.py b/src/sentry/utils/sdk_crashes/sdk_crash_detection.py index 28fd10da02aac9..a7da84972e067e 100644 --- a/src/sentry/utils/sdk_crashes/sdk_crash_detection.py +++ b/src/sentry/utils/sdk_crashes/sdk_crash_detection.py @@ -1,6 +1,6 @@ from __future__ import annotations -from typing import Any, Dict +from typing import Any, Dict, Optional from django.conf import settings @@ -34,7 +34,7 @@ def __init__( self.sdk_crash_reporter = sdk_crash_reporter self.cocoa_sdk_crash_detector = sdk_crash_detector - def detect_sdk_crash(self, event: Event) -> Event: + def detect_sdk_crash(self, event: Event) -> Optional[Event]: should_detect_sdk_crash = ( event.group and event.group.issue_category == GroupCategory.ERROR From 426710b20b934dc3748aa567653a485096e43253 Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Mon, 5 Jun 2023 09:30:51 +0200 Subject: [PATCH 32/32] fix merge --- tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py b/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py index 1108a51973e7b8..621adf7c213cd8 100644 --- a/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py +++ b/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py @@ -9,7 +9,6 @@ from sentry.issues.grouptype import PerformanceNPlusOneGroupType from sentry.testutils import TestCase from sentry.testutils.cases import BaseTestCase, SnubaTestCase -from sentry.testutils.helpers import with_feature from sentry.testutils.performance_issues.store_transaction import PerfIssueTransactionTestMixin from sentry.testutils.silo import region_silo_test from sentry.utils.sdk_crashes.sdk_crash_detection import sdk_crash_detection @@ -38,8 +37,8 @@ def execute_test(self, event_data, should_be_reported, mock_sdk_crash_reporter): if should_be_reported: mock_sdk_crash_reporter.report.assert_called_once() - reported_event = mock_sdk_crash_reporter.report.call_args.args[0] - assert reported_event.data["contexts"]["sdk_crash_detection"]["detected"] is True + reported_event_data = mock_sdk_crash_reporter.report.call_args.args[0] + assert reported_event_data["contexts"]["sdk_crash_detection"]["detected"] is True else: mock_sdk_crash_reporter.report.assert_not_called() @@ -294,7 +293,6 @@ def test_frames_only_in_app_after_sentry_frame_not_reported(self, mock_sdk_crash class SDKCrashReportTestMixin(BaseSDKCrashDetectionMixin, SnubaTestCase): @pytest.mark.django_db - @with_feature("organizations:sdk-crash-reporting") def test_sdk_crash_event_stored_to_sdk_crash_project(self): cocoa_sdk_crashes_project = self.create_project(