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 <[email protected]>

---------

Co-authored-by: Peter Solnica <[email protected]>

Author: st0012

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
 

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
 

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

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