Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid duplicated capturing on the same exception object #1738

Merged
merged 3 commits into from
Feb 20, 2022
Merged

Conversation

st0012
Copy link
Collaborator

@st0012 st0012 commented Feb 20, 2022

As described in #1731, the SDK can have duplicated exception report in Rails 7+ applications because of the new Rails.error support introduced in version 5.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 with Rails.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

@codecov-commenter
Copy link

codecov-commenter commented Feb 20, 2022

Codecov Report

Merging #1738 (6238d6b) into master (27c3a35) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1738   +/-   ##
=======================================
  Coverage   98.39%   98.39%           
=======================================
  Files         141      141           
  Lines        8021     8030    +9     
=======================================
+ Hits         7892     7901    +9     
  Misses        129      129           
Impacted Files Coverage Δ
sentry-ruby/lib/sentry-ruby.rb 95.58% <100.00%> (+0.13%) ⬆️
sentry-ruby/lib/sentry/hub.rb 100.00% <100.00%> (ø)
sentry-ruby/spec/initialization_check_spec.rb 100.00% <100.00%> (ø)
...y-ruby/spec/sentry/rack/capture_exceptions_spec.rb 100.00% <100.00%> (ø)
sentry-ruby/spec/sentry_spec.rb 99.74% <100.00%> (+<0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 27c3a35...6238d6b. Read the comment docs.

@st0012 st0012 modified the milestones: 5.2.0, 5.1.1 Feb 20, 2022
@@ -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)
Copy link
Collaborator Author

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.

Copy link
Member

@sl0thentr0py sl0thentr0py left a 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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicate reporting for Sidekiq errors
3 participants