-
-
Notifications
You must be signed in to change notification settings - Fork 494
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Avoid duplicated capturing on the same exception object #1738
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1738 +/- ##
=======================================
Coverage 98.39% 98.39%
=======================================
Files 141 141
Lines 8021 8030 +9
=======================================
+ Hits 7892 7901 +9
Misses 129 129
Continue to review full report at Codecov.
|
@@ -104,7 +106,7 @@ def capture_exception(exception, **options, &block) | |||
|
|||
capture_event(event, **options, &block).tap do | |||
# mark the exception as captured so we can use this information to avoid duplicated capturing | |||
exception.instance_variable_set(:@__sentry_captured, true) | |||
exception.instance_variable_set(Sentry::CAPTURED_SIGNATURE, true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like the logic of Sentry.exception_captured?
may be used by users or integrations in the future for checking purpose so I extracted it. But marking exception as captured should only be done here so I didn't add a method for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx @st0012, looks good!
As described in #1731, the SDK can have duplicated exception report in Rails 7+ applications because of the new
Rails.error
support introduced in version5.1.0
.After a discussion with the feature author, we decided to let whichever mechanism captures the exception first report it (could be either
Rails.error
or Sentry's integration in different scenarios). Although it's possible that we'd loss some contextual data withRails.error
, the difference is acceptable.Also,
sentry-python
already has a similar mechanism to de-dup error reports, so it's been proven to work.Fixes #1731