Skip to content

Use Exception#detailed_message when generating exception message if applicable #1924

Merged
st0012 merged 3 commits intomasterfrom
deal-with-detailed_message
Oct 25, 2022
Merged

Use Exception#detailed_message when generating exception message if applicable #1924
st0012 merged 3 commits intomasterfrom
deal-with-detailed_message

Conversation

@st0012
Copy link
Copy Markdown
Contributor

@st0012 st0012 commented Oct 23, 2022

Ruby 3.1 provided more detailed exception messages like

undefined method `[]' for nil:NilClass

          {}[:foo][:bar]
                  ^^^^^^

But the additional information was put directly input Exception#message, which caused some problems (as described in this ticket).

So in Ruby 3.2, the additional message will be put into the Exception#detailed_message (proposal), which as a error monitoring service is what we should use.

@st0012 st0012 added this to the 5.6.0 milestone Oct 23, 2022
@st0012 st0012 self-assigned this Oct 23, 2022
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Oct 23, 2022

Codecov Report

❌ Patch coverage is 86.95652% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 98.43%. Comparing base (a1fda66) to head (69e2174).
⚠️ Report is 486 commits behind head on master.

Files with missing lines Patch % Lines
sentry-ruby/spec/sentry/client_spec.rb 84.61% 2 Missing ⚠️
...try-ruby/lib/sentry/interfaces/single_exception.rb 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1924      +/-   ##
==========================================
- Coverage   98.45%   98.43%   -0.02%     
==========================================
  Files         151      151              
  Lines        9259     9273      +14     
==========================================
+ Hits         9116     9128      +12     
- Misses        143      145       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…pplicable

Ruby 3.1 provided more detailed exception messages like

```
undefined method `[]' for nil:NilClass

          {}[:foo][:bar]
                  ^^^^^^
```

But the additional information was put directly input `Exception#message`, which caused some problems
(as described in [this ticket](https://bugs.ruby-lang.org/issues/18438)).

So in Ruby 3.2, the additional message will be put into the `Exception#detailed_message`,
which as a error monitoring service is what we should use.
@st0012 st0012 force-pushed the deal-with-detailed_message branch from 64a96bf to 1c4888f Compare October 23, 2022 21:20

expect(event["exception"]["values"][0]["type"]).to eq("RuntimeError")
expect(event["exception"]["values"][0]["value"]).to eq("An unhandled exception!")
expect(event["exception"]["values"][0]["value"]).to match("An unhandled exception!")
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#detailed_message always includes (ExceptionClass), which I think is beneficial and should be kept.
But I also don't want to check Ruby version before every message check, so I just switch to match instead.

@st0012
Copy link
Copy Markdown
Contributor Author

st0012 commented Oct 23, 2022

cc @mame

@st0012 st0012 marked this pull request as ready for review October 23, 2022 21:31
@st0012 st0012 requested a review from sl0thentr0py October 23, 2022 21:31
@value = (exception.message || "").byteslice(0..Event::MAX_MESSAGE_SIZE_IN_BYTES)
exception_message =
if exception.respond_to?(:detailed_message)
exception.detailed_message(highlight: false)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • highlight: false removes ANSI escape code that's mainly for standard output.
  • #detailed_message always returns a string. Even if the exception's message is nil, it still returns "(Exception)".

Copy link
Copy Markdown
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.

nice! the only (negligible) concern I have is if this somehow affects grouping?
But I think it should be fine, let's merge it.

@st0012 st0012 merged commit 7d97914 into master Oct 25, 2022
@st0012 st0012 deleted the deal-with-detailed_message branch October 25, 2022 22:34
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.

3 participants