Uh oh!
There was an error while loading. Please reload this page.
OCTO-951: Add ability to escalate to Sentry - #130
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Pull request overview
Adds optional Sentry escalation to the exception_handling gem so applications that initialize Sentry can receive error notifications alongside (or during migration from) Honeybadger.
Changes:
- Send Sentry notifications from
send_external_notificationswhen theSentryconstant is defined, with filter-based allow/deny behavior. - Introduce
send_to_sentryas a new exception filter flag (defaultfalse) and addExceptionInfo#send_to_sentry?migration logic (send to Sentry whensend_to_honeybadger: true). - Update documentation, changelog, version bump, and add unit tests covering the new Sentry behavior.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| spec/unit/exception_handling/exception_info_spec.rb | Adds unit tests for ExceptionInfo#send_to_sentry? behavior and filter interactions. |
| spec/unit/exception_handling/exception_description_spec.rb | Adds unit test coverage for the new send_to_sentry filter flag defaulting to false. |
| spec/unit/exception_handling_spec.rb | Adds integration-style specs validating when Sentry notifications are (and aren’t) sent and failure behavior. |
| README.md | Documents Honeybadger/Sentry behavior and filter flag semantics. |
| lib/exception_handling/version.rb | Bumps gem version to 3.2.0. |
| lib/exception_handling/exception_info.rb | Adds send_to_sentry? decision logic. |
| lib/exception_handling/exception_description.rb | Adds send_to_sentry configuration and reader. |
| lib/exception_handling.rb | Implements Sentry notification send path and sentry_defined?. |
| Gemfile.lock | Updates locked gem version to 3.2.0. |
| CHANGELOG.md | Adds 3.2.0 release entry describing Sentry support and filter behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| rescue Exception => ex | ||
| warn("ExceptionHandling.send_exception_to_sentry rescued exception while logging #{exception_info.exception_context}:\n#{exception.class}: #{exception.message}:\n#{ex.class}: #{ex.message}\n#{ex.backtrace.join("\n")}") | ||
| write_exception_to_log(ex, "ExceptionHandling.send_exception_to_sentry rescued exception while logging #{exception_info.exception_context}:\n#{exception.class}: #{exception.message}", exception_info.timestamp) |
| ExceptionHandling.sentry_defined? && ( | ||
| !exception_description || |
There was a problem hiding this comment.
Why would we send the exception to sentry if the exception_description is nil/false?
There was a problem hiding this comment.
A nil exception_description means that that the exception isn't in the exception filters, so we default to notifying sentry/honeybadger.
There was a problem hiding this comment.
Oh that is definitely strange...
There was a problem hiding this comment.
Yeah the naming is pretty weird but I don't want to boil the ocean. For the sake of this PR, I think it does fine to show that "the sentry integration works how the honeybadger integration works".
| # | ||
| # Check if Honeybadger defined. | ||
| # | ||
| def honeybadger_defined? | ||
| Object.const_defined?("Honeybadger") | ||
| end | ||
| # | ||
| # Check if Sentry defined. | ||
| # | ||
| def sentry_defined? | ||
| Object.const_defined?("Sentry") | ||
| end |
There was a problem hiding this comment.
I'm not sure if this is a good pattern we want to follow here. Honeybadger when required automatically setup a bunch of rails callbacks and things that didn't want to do. I wonder if Sentry doesn't do that and we can use some other level instead of if Sentry object is defined.
There was a problem hiding this comment.
Good point. I'll look into something better
There was a problem hiding this comment.
I definitely like this a lot better!
There was a problem hiding this comment.
Is the plan to have the the services that use sentry to initialize it themselves first then enable it here?
There was a problem hiding this comment.
Yeah. For example, to test it locally I added ExceptionHandling.enable_sentry to the bottom of the initializer in Web.
There was a problem hiding this comment.
1 issue found.
About Unblocked
Unblocked has been set up to automatically review your team's pull requests to identify genuine bugs and issues.
📖 Documentation — Learn more in our docs.
💬 Ask questions — Mention @unblocked to request a review or summary, or ask follow-up questions.
👍 Give feedback — React to comments with 👍 or 👎 to help us improve.
⚙️ Customize — Adjust settings in your preferences.
Uh oh!
There was an error while loading. Please reload this page.
Codecov Report❌ Patch coverage is Please upload reports for the commit 3953423 to get more accurate results.
Additional details and impacted files@@ Coverage Diff @@## master #130 +/- ##
==========================================
+ Coverage 93.58% 94.03% +0.45%
==========================================
Files 9 9 Lines 483 520 +37 ==========================================
+ Hits 452 489 +37
Misses 31 31
🚀 New features to boost your workflow:
|
Uh oh!
There was an error while loading. Please reload this page.
Ran
ExceptionHandling.log_error("exception handling integration test")from a local Web Rails console pointed at this version of the gem.See the result here: https://invoca-dev.sentry.io/issues/7652731279/?referrer=issue-stream