From 315310f10d995f4ebbfbd64b72e8bdc9bfd81074 Mon Sep 17 00:00:00 2001 From: Stan Lo Date: Tue, 7 Jan 2025 00:55:42 +0800 Subject: [PATCH] Improve `before_send` and `before_send_transaction`'s return value handling (#2504) * Require testing support files explicitly instead of Dir.glob - It's usually better to require files explicitly when the number of files is manageable. Given that we only need 2 at the moment, I don't think it's worth using Dir.glob. - Also, using Dir.glob causes it to load the Rakefile, which is for isolated testing and it actually initializes the SDK with the wrong configuration in other tests. * Warn when before_send and before_send_transaction callbacks return non-events Currently, if the callbacks return non-event and non-nil values, they'd cause exceptions during serialization, which are rescued and ignored, without explicit logging. This commit adds additional checks to the return values so the users get warnings and also avoids the unnecessary exceptions. * Update changelog * Update sentry-ruby/lib/sentry/client.rb Co-authored-by: Peter Solnica --------- Co-authored-by: Peter Solnica --- CHANGELOG.md | 1 + sentry-ruby/lib/sentry/client.rb | 22 +++++++----- .../spec/sentry/client/event_sending_spec.rb | 36 +++++++++++++++++++ sentry-ruby/spec/spec_helper.rb | 3 +- 4 files changed, 53 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 954b4ac33..080b12bac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,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)) ### Internal 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