From 02e0f8c205b3ef301470c7634ad89c82a1a649f8 Mon Sep 17 00:00:00 2001 From: Stan Lo Date: Sun, 29 Dec 2024 21:47:28 +0000 Subject: [PATCH 1/4] 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. --- sentry-ruby/spec/spec_helper.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 From 0cdb62953cce0909b8c1ef6b1cac1437d69050b4 Mon Sep 17 00:00:00 2001 From: Stan Lo Date: Sun, 29 Dec 2024 21:49:44 +0000 Subject: [PATCH 2/4] 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. --- sentry-ruby/lib/sentry/client.rb | 22 +++++++----- .../spec/sentry/client/event_sending_spec.rb | 36 +++++++++++++++++++ 2 files changed, 50 insertions(+), 8 deletions(-) diff --git a/sentry-ruby/lib/sentry/client.rb b/sentry-ruby/lib/sentry/client.rb index 9d48ee301..3d81c175a 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 seraliazing 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 From bf6869501acef19714775b8e20ae0dc6d59eb871 Mon Sep 17 00:00:00 2001 From: Stan Lo Date: Sun, 29 Dec 2024 21:57:27 +0000 Subject: [PATCH 3/4] Update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) 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 From 2f8bd4e3c9e749369727c6e0068ce539ad8119b0 Mon Sep 17 00:00:00 2001 From: Stan Lo Date: Sun, 5 Jan 2025 13:10:38 +0000 Subject: [PATCH 4/4] Update sentry-ruby/lib/sentry/client.rb Co-authored-by: Peter Solnica --- sentry-ruby/lib/sentry/client.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sentry-ruby/lib/sentry/client.rb b/sentry-ruby/lib/sentry/client.rb index 3d81c175a..f1db4a295 100644 --- a/sentry-ruby/lib/sentry/client.rb +++ b/sentry-ruby/lib/sentry/client.rb @@ -201,7 +201,7 @@ def send_event(event, hint = nil) spans_delta = spans_before - spans_after transport.record_lost_event(:before_send, "span", num: spans_delta) if spans_delta > 0 else - # Avoid seraliazing the event object in this case because we aren't sure what it is and what it contains + # 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