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

Allow non-string error message to be reported to sentry #2137

Merged
merged 1 commit into from
Nov 25, 2023

Conversation

the-spectator
Copy link
Contributor

Ref: #907 (comment)

Description

When the def message in the error class is overridden to return a non-string value. Sentry is not able to report the error due to the error NoMethodError: undefined method byteslice' for []:Array`. Ideally, we shouldn't override the message and yet it was done 😓 .

Describe your changes:
Convert non-string error message to string.

Copy link
Contributor

@natikgadzhi natikgadzhi left a comment

Choose a reason for hiding this comment

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

Not related to the core team at Sentry, but the change looks reasonable to me. It's better to send out a longer, slightly awkward error message than to error out.

Copy link

codecov bot commented Nov 24, 2023

Codecov Report

Merging #2137 (5617ca3) into master (c4e4797) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 5617ca3 differs from pull request most recent head a5fa2ce. Consider uploading reports for the commit a5fa2ce to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2137   +/-   ##
=======================================
  Coverage   97.34%   97.34%           
=======================================
  Files          99       99           
  Lines        3687     3688    +1     
=======================================
+ Hits         3589     3590    +1     
  Misses         98       98           
Components Coverage Δ
sentry-ruby 98.03% <100.00%> (+<0.01%) ⬆️
sentry-rails 94.98% <ø> (ø)
sentry-sidekiq 94.50% <ø> (ø)
sentry-resque 93.65% <ø> (ø)
sentry-delayed_job 94.44% <ø> (ø)
sentry-opentelemetry 100.00% <ø> (ø)
Files Coverage Δ
...try-ruby/lib/sentry/interfaces/single_exception.rb 100.00% <100.00%> (ø)

Copy link
Collaborator

@st0012 st0012 left a comment

Choose a reason for hiding this comment

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

Thank you for the fix. Can you also add a changelog entry for it?

@the-spectator
Copy link
Contributor Author

the-spectator commented Nov 25, 2023

Thank you for the review! 🙏
@st0012 Added the changelog.

@st0012 st0012 merged commit bebedc4 into getsentry:master Nov 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants