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

Register Sentry's ErrorSubscriber for Rails 7.0+ apps #1705

Merged
merged 4 commits into from
Feb 1, 2022
Merged

Conversation

st0012
Copy link
Collaborator

@st0012 st0012 commented Jan 30, 2022

This implements the integration proposed in #1630.

After Rails 7.0, you'll be able to use the standardized Rails.error.record or Rails.error.handle interfaces to report exceptions to Sentry. Please check ActiveSupport::ErrorReporter for more information.

Gotcha & Workaround

If an operation is wrapped inside app.executor.wrap, any exception raised will be captured by Rails' error reporter,
even if it has been reported by either the SDK integration or user. So in some cases (e.g. ActionCable's channel actions), the same error could be reported twice.

I worked around this by marking if an exception has been captured by the SDK. And if it has, we don't capture it with the ErrorSubscriber.

(Dropping the original integration isn't an option because the contextual data may not be accessible from the error reporter)

Closes #1630

@st0012 st0012 added this to the 5.1.0 milestone Jan 30, 2022
@st0012 st0012 self-assigned this Jan 30, 2022
@st0012 st0012 added this to In progress in 5.x via automation Jan 30, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jan 30, 2022

Codecov Report

Merging #1705 (af3d659) into master (4217838) will decrease coverage by 0.17%.
The diff coverage is 39.13%.

❗ Current head af3d659 differs from pull request most recent head 63225be. Consider uploading reports for the commit 63225be to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1705      +/-   ##
==========================================
- Coverage   98.59%   98.41%   -0.18%     
==========================================
  Files         136      136              
  Lines        7808     7830      +22     
==========================================
+ Hits         7698     7706       +8     
- Misses        110      124      +14     
Impacted Files Coverage Δ
sentry-rails/spec/sentry/rails_spec.rb 90.69% <25.00%> (-9.31%) ⬇️
sentry-rails/lib/sentry/rails/railtie.rb 97.26% <50.00%> (-2.74%) ⬇️
sentry-rails/spec/support/test_rails_app/app.rb 97.76% <100.00%> (+0.01%) ⬆️
sentry-ruby/lib/sentry/hub.rb 100.00% <100.00%> (ø)

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 4217838...63225be. Read the comment docs.

@st0012 st0012 force-pushed the implement-#1630 branch 2 times, most recently from f136ad6 to c9d107a Compare January 30, 2022 18:08
@st0012
Copy link
Collaborator Author

st0012 commented Jan 30, 2022

@georgeclaghorn I'd like to hear your opinion on the gotcha and the workaround I took 🙂

@georgeclaghorn
Copy link

I don't feel familiar enough with this gem to give a thorough review but this all seems reasonable to me. 👍

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.

cool that we support this now!

@st0012 st0012 merged commit 9889bbe into master Feb 1, 2022
5.x automation moved this from In progress to Done Feb 1, 2022
@st0012 st0012 deleted the implement-#1630 branch February 1, 2022 10:19
lewispb added a commit to lewispb/sentry-ruby that referenced this pull request Feb 1, 2022
* master:
  feat(performance): Sync activerecord and net-http span names (getsentry#1681)
  Register Sentry's ErrorSubscriber for Rails 7.0+ apps (getsentry#1705)
  Support serializing ActiveRecord job arguments in global id form (getsentry#1688)
  release: 5.0.2
  Fix report_after_job_retries's decision logic (getsentry#1704)
  Followup of getsentry#1701 (getsentry#1703)
  Capture transaction tags (getsentry#1701)
@casperisfine
Copy link
Contributor

(Dropping the original integration isn't an option because the contextual data may not be accessible from the error reporter)

Could you expand on that? I'm not sure I follow.

@ariccio
Copy link

ariccio commented Sep 12, 2023

Interesting. I think this means I can turn on config.active_record.db_warnings_action, setting it to :report, and it will automatically send them through to sentry? Awesome! I'll have to try it!

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

Successfully merging this pull request may close these issues.

Support the new Rails standard error reporting interface
6 participants