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

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

Merged
merged 3 commits into from
Oct 25, 2022

Conversation

st0012
Copy link
Collaborator

@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

codecov-commenter commented Oct 23, 2022

Codecov Report

Base: 98.45% // Head: 98.41% // Decreases project coverage by -0.04% ⚠️

Coverage data is based on head (5c96ff6) compared to base (d3d3568).
Patch coverage: 86.95% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1924      +/-   ##
==========================================
- Coverage   98.45%   98.41%   -0.05%     
==========================================
  Files         151      151              
  Lines        9259     9273      +14     
==========================================
+ Hits         9116     9126      +10     
- Misses        143      147       +4     
Impacted Files Coverage Δ
...try-ruby/lib/sentry/interfaces/single_exception.rb 97.05% <75.00%> (-2.95%) ⬇️
sentry-ruby/spec/sentry/client_spec.rb 96.61% <84.61%> (-0.64%) ⬇️
sentry-rails/spec/sentry/rails_spec.rb 85.25% <100.00%> (ø)
sentry-ruby/spec/sentry/event_spec.rb 100.00% <100.00%> (ø)
...-sidekiq/spec/sentry/sidekiq/error_handler_spec.rb 100.00% <100.00%> (ø)
sentry-sidekiq/spec/sentry/sidekiq_spec.rb 100.00% <100.00%> (ø)
sentry-ruby/lib/sentry/breadcrumb.rb 96.29% <0.00%> (-3.71%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

…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.
@@ -103,7 +103,7 @@
expect(response.status).to eq(500)

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
Collaborator 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
Collaborator Author

st0012 commented Oct 23, 2022

cc @mame

@st0012 st0012 marked this pull request as ready for review 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
Collaborator 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
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