feat: Gracefully handle SDK callback exceptions

sentry-ruby/lib/sentry-ruby.rb

@@ -10,6 +10,7 @@
 require "sentry/utils/argument_checking_helper"
 require "sentry/utils/encoding_helper"
 require "sentry/utils/logging_helper"
+require "sentry/utils/callback_helper"
 require "sentry/utils/sample_rand"
 require "sentry/configuration"
 require "sentry/structured_logger"

sentry-ruby/lib/sentry/backtrace.rb

@@ -7,6 +7,8 @@
 module Sentry
   # @api private
   class Backtrace
+    extend CallbackHelper
+
     # holder for an Array of Backtrace::Line instances
     attr_reader :lines
 
@@ -15,7 +17,14 @@ class Backtrace
     def self.parse(backtrace, project_root, app_dirs_pattern, in_app_pattern: nil, &backtrace_cleanup_callback)
       ruby_lines = backtrace.is_a?(Array) ? backtrace : backtrace.split(/\n\s*/)
 
-      ruby_lines = backtrace_cleanup_callback.call(ruby_lines) if backtrace_cleanup_callback
+      if backtrace_cleanup_callback
+        ruby_lines = safe_dispatch_callback(
+          "backtrace_cleanup_callback",
+          backtrace_cleanup_callback,
+          [ruby_lines],
+          fallback: ruby_lines
+        )
+      end
 
       # in_app_pattern is now passed in from StacktraceBuilder, so this regex won't be triggered
       # only here for backwards compat and will be deleted

sentry-ruby/lib/sentry/client.rb

@@ -11,6 +11,7 @@
 module Sentry
   class Client
     include LoggingHelper
+    include CallbackHelper
 
     # The Transport object that'll send events for the client.
     # @return [Transport]
@@ -89,7 +90,7 @@ def capture_event(event, scope, hint = {})
           transport.record_lost_event(:queue_overflow, "span", num: spans_before + 1) if is_transaction
         end
       else
-        send_event(event, hint)
+        event = send_event(event, hint)
       end
 
       event
@@ -237,7 +238,11 @@ def send_event(event, hint = nil)
       spans_before = event.is_a?(TransactionEvent) ? event.spans.size : 0
 
       if event.is_a?(ErrorEvent) && configuration.before_send
-        event = configuration.before_send.call(event, hint)
+        event = safe_dispatch_callback(
+          "before_send",
+          configuration.before_send,
+          [event, hint]
+        )
 
         if !event.is_a?(ErrorEvent)
           # Avoid serializing the event object in this case because we aren't sure what it is and what it contains
@@ -250,7 +255,11 @@ def send_event(event, hint = nil)
       end
 
       if event.is_a?(TransactionEvent) && configuration.before_send_transaction
-        event = configuration.before_send_transaction.call(event, hint)
+        event = safe_dispatch_callback(
+          "before_send_transaction",
+          configuration.before_send_transaction,
+          [event, hint]
+        )
 
         if !event.is_a?(TransactionEvent)
           # Avoid serializing the event object in this case because we aren't sure what it is and what it contains
@@ -268,7 +277,11 @@ def send_event(event, hint = nil)
       end
 
       if event.is_a?(CheckInEvent) && configuration.before_send_check_in
-        event = configuration.before_send_check_in.call(event, hint)
+        event = safe_dispatch_callback(
+          "before_send_check_in",
+          configuration.before_send_check_in,
+          [event, hint]
+        )
 
         if !event.is_a?(CheckInEvent)
           # Avoid serializing the event object in this case because we aren't sure what it is and what it contains

sentry-ruby/lib/sentry/hub.rb

@@ -7,6 +7,7 @@
 module Sentry
   class Hub
     include ArgumentCheckingHelper
+    include CallbackHelper
 
     MUTEX = Mutex.new
 
@@ -296,7 +297,11 @@ def add_breadcrumb(breadcrumb, hint: {})
       return unless configuration.enabled_in_current_env?
 
       if before_breadcrumb = current_client.configuration.before_breadcrumb
-        breadcrumb = before_breadcrumb.call(breadcrumb, hint)
+        breadcrumb = safe_dispatch_callback(
+          "before_breadcrumb",
+          before_breadcrumb,
+          [breadcrumb, hint]
+        )
       end
 
       return unless breadcrumb

sentry-ruby/lib/sentry/log_event_buffer.rb

@@ -21,7 +21,7 @@ def initialize(configuration, client)
         max_items_before_drop: MAX_EVENTS_BEFORE_DROP,
         envelope_type: "log",
         envelope_content_type: "application/vnd.sentry.items.log+json",
-        before_send: configuration.before_send_log
+        before_send: :before_send_log
       )
     end
   end

sentry-ruby/lib/sentry/metric_event_buffer.rb

@@ -21,7 +21,7 @@ def initialize(configuration, client)
         max_items_before_drop: MAX_METRICS_BEFORE_DROP,
         envelope_type: "trace_metric",
         envelope_content_type: "application/vnd.sentry.items.trace-metric+json",
-        before_send: configuration.before_send_metric
+        before_send: :before_send_metric
       )
     end
   end

sentry-ruby/lib/sentry/scope.rb

@@ -9,6 +9,7 @@
 module Sentry
   class Scope
     include ArgumentCheckingHelper
+    include CallbackHelper
 
     ATTRIBUTES = [
       :transaction_name,
@@ -71,7 +72,8 @@ def apply_to_event(event, hint = nil)
 
       unless all_event_processors.empty?
         all_event_processors.each do |processor_block|
-          event = processor_block.call(event, hint)
+          event = safe_dispatch_callback("event_processor", processor_block, [event, hint])
+          return unless event
         end
       end
 

sentry-ruby/lib/sentry/std_lib_logger.rb

@@ -4,6 +4,8 @@ module Sentry
   # Ruby Logger support Add commentMore actions
   # intercepts any logger instance and send the log to Sentry too.
   module StdLibLogger
+    include CallbackHelper
+
     SEVERITY_MAP = {
       0 => :debug,
       1 => :info,
@@ -37,8 +39,12 @@ def add(severity, message = nil, progname = nil, &block)
         message = message.to_s.strip
 
         if !message.nil? && message != Sentry::Logger::PROGNAME && method = SEVERITY_MAP[severity]
-          if (filter = Sentry.configuration.std_lib_logger_filter) && !filter.call(self, message, method)
-            return result
+          if filter = Sentry.configuration.std_lib_logger_filter
+            return result unless safe_dispatch_callback(
+              "std_lib_logger_filter",
+              filter,
+              [self, message, method]
+            )
           end
 
           Sentry.logger.send(method, message, origin: ORIGIN)

sentry-ruby/lib/sentry/telemetry_event_buffer.rb

@@ -12,6 +12,8 @@ module Sentry
   #
   # @!visibility private
   class TelemetryEventBuffer < ThreadedPeriodicWorker
+    include CallbackHelper
+
     FLUSH_INTERVAL = 5 # seconds
 
     # @!visibility private
@@ -21,6 +23,7 @@ def initialize(configuration, client, event_class:, max_items:, max_items_before
       super(configuration.sdk_logger, FLUSH_INTERVAL)
 
       @client = client
+      @configuration = configuration
       @dsn = configuration.dsn
       @debug = configuration.debug
       @event_class = event_class
@@ -95,9 +98,9 @@ def send_items
       discarded_bytes = 0
       envelope_items = []
 
-      if @before_send
+      if callback = @configuration.send(@before_send)
         @pending_items.each do |item|
-          processed_item = @before_send.call(item)
+          processed_item = safe_dispatch_callback(@before_send.to_s, callback, [item])
 
           if processed_item
             envelope_items << processed_item.to_h

sentry-ruby/lib/sentry/transaction.rb

@@ -14,6 +14,7 @@ class Transaction < Span
     SOURCES = %i[custom url route view component task]
 
     include LoggingHelper
+    include CallbackHelper
 
     # The name of the transaction.
     # @return [String]
@@ -142,7 +143,12 @@ def set_initial_sample_decision(sampling_context:)
 
       sample_rate =
         if configuration.traces_sampler.is_a?(Proc)
-          configuration.traces_sampler.call(sampling_context)
+          safe_dispatch_callback(
+            "traces_sampler",
+            configuration.traces_sampler,
+            [sampling_context],
+            fallback: configuration.traces_sample_rate
+          )
         elsif !sampling_context[:parent_sampled].nil?
           sampling_context[:parent_sampled]
         else

sentry-ruby/lib/sentry/utils/callback_helper.rb

@@ -0,0 +1,27 @@
+# frozen_string_literal: true
+
+module Sentry
+  # @private
+  module CallbackHelper
+    # @!visibility private
+    def safe_dispatch_callback(callback_name, callback, args, fallback: nil)
+      callback.call(*args)
+    rescue => e
+      log_callback_error(callback_name, e)
+      fallback
+    end
+
+    private
+
+    def log_callback_error(callback_name, exception)
+      return unless Sentry.initialized?
+
+      message = "Error in #{callback_name} callback: #{exception.message}"
+      message += "\n#{exception.backtrace.join("\n")}" if Sentry.configuration.debug && exception.backtrace
+
+      Sentry.sdk_logger.error(LOGGER_PROGNAME) { message }
+    rescue StandardError
+      # Callback error reporting must not propagate to the application.
+    end
+  end
+end

sentry-ruby/spec/sentry/client/event_sending_spec.rb

@@ -12,6 +12,11 @@
   before do
     stub_request(:post, Sentry::TestHelper::DUMMY_DSN)
     allow(Sentry).to receive(:configuration).and_return configuration
+    allow(Sentry).to receive(:initialized?).and_return(true)
+  end
+
+  after do
+    allow(Sentry).to receive(:initialized?).and_call_original
   end
 
   subject(:client) { Sentry::Client.new(configuration) }
@@ -373,7 +378,7 @@
         it "swallows the event and logs the failure" do
           expect(client.capture_event(event, scope)).to be_nil
 
-          expect(string_io.string).to match(/Event capturing failed: TypeError/)
+          expect(string_io.string).to match(/Error in event_processor callback: TypeError/)
           expect(string_io.string).not_to match(__FILE__)
         end
 
@@ -384,7 +389,7 @@
           it "logs the error with backtrace" do
             expect(client.capture_event(event, scope)).to be_nil
 
-            expect(string_io.string).to match(/Event capturing failed: TypeError/)
+            expect(string_io.string).to match(/Error in event_processor callback: TypeError/)
             expect(string_io.string).to match(__FILE__)
           end
         end
@@ -411,7 +416,9 @@
 
           expect(client.capture_event(event, scope)).to be_nil
 
-          expect(string_io.string).to match(/Event sending failed: TypeError/)
+          expect(string_io.string).to match(/Error in before_send callback: TypeError/)
+          expect(client.transport).to have_recorded_lost_event(:before_send, "error")
+          expect(client.transport).not_to have_recorded_lost_event(:network_error, "error")
         end
 
         it "captures client report for error event" do
@@ -447,7 +454,9 @@
           expect(client.capture_event(event, scope)).to be_a(Sentry::ErrorEvent)
           sleep(0.2)
 
-          expect(string_io.string).to match(/Event sending failed: TypeError/)
+          expect(string_io.string).to match(/Error in before_send callback: TypeError/)
+          expect(client.transport).to have_recorded_lost_event(:before_send, "error")
+          expect(client.transport).not_to have_recorded_lost_event(:network_error, "error")
         end
 
         it "captures client report for error event" do
@@ -486,12 +495,12 @@
           end
         end
 
-        it "raises the error" do
-          expect do
-            client.send_event(event)
-          end.to raise_error(TypeError)
+        it "swallows and logs the error" do
+          expect(client.send_event(event)).to be_nil
 
-          expect(string_io.string).to match(/Event sending failed: TypeError/)
+          expect(string_io.string).to match(/Error in before_send callback: TypeError/)
+          expect(client.transport).to have_recorded_lost_event(:before_send, "error")
+          expect(client.transport).not_to have_recorded_lost_event(:network_error, "error")
         end
 
         context "with config.debug = true" do
@@ -500,11 +509,9 @@
           end
 
           it "logs the error with backtrace" do
-            expect do
-              client.send_event(event)
-            end.to raise_error(TypeError)
+            expect(client.send_event(event)).to be_nil
 
-            expect(string_io.string).to match(/Event sending failed: TypeError/)
+            expect(string_io.string).to match(/Error in before_send callback: TypeError/)
             expect(string_io.string).to match(__FILE__)
           end
         end

sentry-ruby/spec/sentry/hub_spec.rb

@@ -481,6 +481,25 @@
           expect(peek_crumb).to eq(nil)
         end
       end
+
+      context "when before_breadcrumb raises" do
+        before do
+          allow(Sentry).to receive(:configuration).and_return(configuration)
+          allow(Sentry).to receive(:initialized?).and_return(true)
+          configuration.before_breadcrumb = ->(_breadcrumb, _hint) { raise TypeError }
+        end
+
+        after do
+          allow(Sentry).to receive(:initialized?).and_call_original
+        end
+
+        it "logs the error and doesn't add anything" do
+          expect { subject.add_breadcrumb(new_breadcrumb) }.not_to raise_error
+
+          expect(peek_crumb).to eq(nil)
+          expect(string_io.string).to include("Error in before_breadcrumb callback: TypeError")
+        end
+      end
     end
 
     context "when the SDK is not activated in the current environment" do
@@ -609,7 +628,7 @@
     let(:message) { "Test message" }
 
     it 'sends the result of Event.capture_type' do
-      expect(client).to receive(:send_event)
+      expect(client).to receive(:send_event).and_call_original
 
       event = subject.capture_message("Test message")
 

sentry-ruby/spec/sentry/metrics_spec.rb

@@ -316,6 +316,8 @@
     end
 
     context "with before_send_metric callback" do
+      let(:string_io) { StringIO.new }
+
       it "receives MetricEvent" do
         perform_basic_setup do |config|
           config.before_send_metric = lambda do |metric|
@@ -361,6 +363,20 @@
         expect(sentry_metrics.first[:name]).to eq("test.allowed")
         expect(Sentry.get_current_client.transport).to have_recorded_lost_event(:before_send, 'trace_metric', num: 2, num_bytes: a_value > 0)
       end
+
+      it "drops metrics when the callback raises" do
+        perform_basic_setup do |config|
+          config.sdk_logger = Logger.new(string_io)
+          config.before_send_metric = ->(_metric) { raise TypeError }
+        end
+
+        expect { Sentry.metrics.count("test.failed") }.not_to raise_error
+        Sentry.get_current_client.flush
+
+        expect(sentry_metrics).to be_empty
+        expect(Sentry.get_current_client.transport).to have_recorded_lost_event(:before_send, 'trace_metric', num: 1, num_bytes: a_value > 0)
+        expect(string_io.string).to include("Error in before_send_metric callback: TypeError")
+      end
     end
   end
 end