diff --git a/CHANGELOG.md b/CHANGELOG.md index cd18e7413..73201fc77 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ ### Bug fixes - Default to `internal_error` error type for OpenTelemetry spans [#2473](https://github.com/getsentry/sentry-ruby/pull/2473) +- Improve `before_send` and `before_send_transaction`'s return value handling ([#2504](https://github.com/getsentry/sentry-ruby/pull/2504)) ## 5.22.1 diff --git a/sentry-ruby/lib/sentry/client.rb b/sentry-ruby/lib/sentry/client.rb index 9d48ee301..f1db4a295 100644 --- a/sentry-ruby/lib/sentry/client.rb +++ b/sentry-ruby/lib/sentry/client.rb @@ -183,8 +183,11 @@ def send_event(event, hint = nil) if event_type != TransactionEvent::TYPE && configuration.before_send event = configuration.before_send.call(event, hint) - if event.nil? - log_debug("Discarded event because before_send returned nil") + unless event.is_a?(ErrorEvent) + # Avoid serializing the event object in this case because we aren't sure what it is and what it contains + log_debug(<<~MSG) + Discarded event because before_send didn't return a Sentry::ErrorEvent object but an instance of #{event.class} + MSG transport.record_lost_event(:before_send, data_category) return end @@ -193,15 +196,18 @@ def send_event(event, hint = nil) if event_type == TransactionEvent::TYPE && configuration.before_send_transaction event = configuration.before_send_transaction.call(event, hint) - if event.nil? - log_debug("Discarded event because before_send_transaction returned nil") - transport.record_lost_event(:before_send, "transaction") - transport.record_lost_event(:before_send, "span", num: spans_before + 1) - return - else + if event.is_a?(TransactionEvent) spans_after = event.is_a?(TransactionEvent) ? event.spans.size : 0 spans_delta = spans_before - spans_after transport.record_lost_event(:before_send, "span", num: spans_delta) if spans_delta > 0 + else + # Avoid serializing the event object in this case because we aren't sure what it is and what it contains + log_debug(<<~MSG) + Discarded event because before_send_transaction didn't return a Sentry::TransactionEvent object but an instance of #{event.class} + MSG + transport.record_lost_event(:before_send, "transaction") + transport.record_lost_event(:before_send, "span", num: spans_before + 1) + return end end diff --git a/sentry-ruby/spec/sentry/client/event_sending_spec.rb b/sentry-ruby/spec/sentry/client/event_sending_spec.rb index e66926758..0fc9105c1 100644 --- a/sentry-ruby/spec/sentry/client/event_sending_spec.rb +++ b/sentry-ruby/spec/sentry/client/event_sending_spec.rb @@ -248,6 +248,30 @@ expect(dbl).not_to receive(:call) client.send_event(event) end + + it "warns if before_send returns nil" do + string_io = StringIO.new + logger = Logger.new(string_io, level: :debug) + configuration.logger = logger + configuration.before_send = lambda do |_event, _hint| + nil + end + + client.send_event(event) + expect(string_io.string).to include("Discarded event because before_send didn't return a Sentry::ErrorEvent object but an instance of NilClass") + end + + it "warns if before_send returns non-Event objects" do + string_io = StringIO.new + logger = Logger.new(string_io, level: :debug) + configuration.logger = logger + configuration.before_send = lambda do |_event, _hint| + 123 + end + + client.send_event(event) + expect(string_io.string).to include("Discarded event because before_send didn't return a Sentry::ErrorEvent object but an instance of Integer") + end end it_behaves_like "Event in send_event" do @@ -290,6 +314,18 @@ expect(event["tags"]["called"]).to eq(true) end end + + it "warns if before_send_transaction returns nil" do + string_io = StringIO.new + logger = Logger.new(string_io, level: :debug) + configuration.logger = logger + configuration.before_send_transaction = lambda do |_event, _hint| + nil + end + + client.send_event(event) + expect(string_io.string).to include("Discarded event because before_send_transaction didn't return a Sentry::TransactionEvent object but an instance of NilClass") + end end it_behaves_like "TransactionEvent in send_event" do diff --git a/sentry-ruby/spec/spec_helper.rb b/sentry-ruby/spec/spec_helper.rb index e0d1c9a1e..3821b3929 100644 --- a/sentry-ruby/spec/spec_helper.rb +++ b/sentry-ruby/spec/spec_helper.rb @@ -26,7 +26,8 @@ require "sentry-ruby" require "sentry/test_helper" -Dir[Pathname(__dir__).join("support/**/*.rb")].sort.each { |f| require f } +require "support/profiler" +require "support/stacktrace_test_fixture" RSpec.configure do |config| # Enable flags like --only-failures and --next-failure