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
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
- Use `Sentry.with_child_span` in redis and net/http instead of `span.start_child` [#1920](https://github.com/getsentry/sentry-ruby/pull/1920)
- This might change the nesting of some spans and make it more accurate

- Use `Exception#detailed_message` when generating exception message if applicable [#1924](https://github.com/getsentry/sentry-ruby/pull/1924)

### Bug Fixes

- `Sentry::BackgroundWorker` will release `ActiveRecord` connection pool only when the `ActiveRecord` connection is established
Expand Down
4 changes: 2 additions & 2 deletions sentry-rails/spec/sentry/rails_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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.

end
end

Expand Down Expand Up @@ -159,7 +159,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!")
expect(event["sdk"]).to eq("name" => "sentry.ruby.rails", "version" => Sentry::Rails::VERSION)
end

Expand Down
10 changes: 9 additions & 1 deletion sentry-ruby/lib/sentry/interfaces/single_exception.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,15 @@ class SingleExceptionInterface < Interface

def initialize(exception:, stacktrace: nil)
@type = exception.class.to_s
@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)".

else
exception.message || ""
end

@value = exception_message.byteslice(0..Event::MAX_MESSAGE_SIZE_IN_BYTES)

@module = exception.class.to_s.split('::')[0...-1].join('::')
@thread_id = Thread.current.object_id
@stacktrace = stacktrace
Expand Down
30 changes: 28 additions & 2 deletions sentry-ruby/spec/sentry/client_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,33 @@ def sentry_context

it "sets the message to the exception's value and type" do
expect(hash[:exception][:values][0][:type]).to eq("Exception")
expect(hash[:exception][:values][0][:value]).to eq(message)
expect(hash[:exception][:values][0][:value]).to match(message)
end

context "with special error messages" do
let(:exception) do
begin
{}[:foo][:bar]
rescue => e
e
end
end

it "sets correct exception message based on Ruby version" do
version = Gem::Version.new(RUBY_VERSION)

if version >= Gem::Version.new("3.2.0-dev")
expect(hash[:exception][:values][0][:value]).to eq(
"undefined method `[]' for nil:NilClass (NoMethodError)\n\n {}[:foo][:bar]\n ^^^^^^"
)
elsif version >= Gem::Version.new("3.1")
expect(hash[:exception][:values][0][:value]).to eq(
"undefined method `[]' for nil:NilClass\n\n {}[:foo][:bar]\n ^^^^^^"
)
else
expect(hash[:exception][:values][0][:value]).to eq("undefined method `[]' for nil:NilClass")
end
end
end

it "sets threads interface without stacktrace" do
Expand Down Expand Up @@ -195,7 +221,7 @@ def sentry_context
it 'returns an event' do
event = subject.event_from_exception(ZeroDivisionError.new("divided by 0"))
expect(event).to be_a(Sentry::ErrorEvent)
expect(Sentry::Event.get_message_from_exception(event.to_hash)).to eq("ZeroDivisionError: divided by 0")
expect(Sentry::Event.get_message_from_exception(event.to_hash)).to match("ZeroDivisionError: divided by 0")
end

context 'for a nested exception type' do
Expand Down
4 changes: 2 additions & 2 deletions sentry-ruby/spec/sentry/event_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -224,8 +224,8 @@
end

it "returns the exception's message" do
expect(described_class.get_log_message(subject.to_hash)).to eq("Exception: error!")
expect(described_class.get_log_message(subject.to_json_compatible)).to eq("Exception: error!")
expect(described_class.get_log_message(subject.to_hash)).to include("Exception: error!")
expect(described_class.get_log_message(subject.to_json_compatible)).to match("Exception: error!")
end
end
context "with message event" do
Expand Down
2 changes: 1 addition & 1 deletion sentry-sidekiq/spec/sentry/sidekiq/error_handler_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
processor.fire_event(:startup)

event = transport.events.last.to_hash
expect(Sentry::Event.get_message_from_exception(event)).to eq("RuntimeError: Uhoh!")
expect(Sentry::Event.get_message_from_exception(event)).to match("RuntimeError: Uhoh!")
expect(event[:transaction]).to eq "Sidekiq/startup"
end

Expand Down
2 changes: 1 addition & 1 deletion sentry-sidekiq/spec/sentry/sidekiq_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@

event = transport.events.last.to_hash
expect(event[:sdk]).to eq({ name: "sentry.ruby.sidekiq", version: described_class::VERSION })
expect(Sentry::Event.get_message_from_exception(event)).to eq("RuntimeError: I'm sad!")
expect(Sentry::Event.get_message_from_exception(event)).to match("RuntimeError: I'm sad!")
end

describe "context cleanup" do
Expand Down