Sentry SDK Crash Detection POC #49928
Conversation
Running test_discover.py with the test explorer in VSCode failed with an import file mismatch. This is fixed now by adding a missing __init__.py file to tests/sentry/data_export/processors.
Remove the debug images not referenced in the stack trace so the debug images don't contain customer data. Fixes GH-46174 Co-authored-by: Joris Bayer <joris.bayer@sentry.io>
Detect when an event has the context `sdk_crash_detection`, and skip crash monitoring in that case. Fixes GH-46173
There was a problem hiding this comment.
Pressing "Request changes" because I am still worried about user-specific data slipping through the cracks because of the Allow.ALL variant. I should have made that concern more clear when reviewing #50400 (review).
Even if the feature is disabled by default, I am afraid that if we merge it now we might overlook this risk when opting in more orgs.
@jjbayer, I get your point. I added #50710 and #50712 to #49749. I will open PRs to this PR soon. |
|
please instead of making this patch bigger utilize some sort of feature flagging to merge smaller changes into mainline just because something was "already reviewed" doesn't mean things shouldn't be ignored in a review |
|
I agree, @asottile-sentry. I should have started with a feature flag and then merge all the small batches into mainline. Smaller batches are better for tons of reasons. FYI, I already have a feature flag in place sentry/src/sentry/tasks/post_process.py Lines 1067 to 1068 in ea6b6dc @jjbayer, can we maybe agree on merging this PR and not enabling the feature until we address #50710 and #50712. This PR is already huge. |
I think you should still try and split this up -- merging large unreviewable changes is inherently risky |
Okay, I will split this PR up into multiple ones. It shouldn't be a big problem. I just wanted to be sure to align on the approach. @asottile-sentry, I plan on adding a feature flag first without any actual logic. The feature will be disabled by default. Then I open subsequent PRs until all of the changes from this PR are merged to master. Only when we are happy will all the changes, we enable the feature. How does that sound @asottile-sentry? |
|
As this PR is too big, I will split it into multiple ones to make it easier to review. |
This PR adds a feature flag disabled by default, the project id for the feature in the settings, and calls the placeholder class for the SDK crash detection (#44342) in post processing. This is the first PR of splitting up the POC for SDK crash detection (#49928) into multiple PRs. --------- Co-authored-by: Iker Barriocanal <32816711+iker-barriocanal@users.noreply.github.com> Co-authored-by: getsantry[bot] <66042841+getsantry[bot]@users.noreply.github.com>
Store all unhandled Cocoa SDK events to a dedicated project configurable via the settings. Before enabling this feature in production we must check if the unhandled event stems from the Cocoa SDK and we need to strip most of the event data to prevent collecting PII. This is the second PR of splitting up the POC for SDK crash detection (#49928) into multiple PRs and it is based on #50794. --------- Co-authored-by: Joris Bayer <joris.bayer@sentry.io>
This PR adds the Sentry SDK Crash Detection (#44342) only for the sentry-sdk organization for a proof of concept. The feature is disabled by default.
Most of the code was already reviewed in previous PRs (#46494, #46578, #49170, #49528, #49593, #49746, #50284) but giving it a final pass still makes sense.
Fixes GH-49749