Report Sidekiq errors when the job is discarded

Add the `sidekiq_report_errors` config option. If set to `discard`, it
will only report the error if the job is discarded, or "dead" in Sidekiq
terminology.

This follows the same behavior as the `activejob_report_errors` config
option.

To make this behavior work, it uses a Sidekiq death handler to report
the error when the job is discarded. This death handler is run before
the error handler, so the error handler can stay responsible for
completing the transaction.

Death handlers were introduced in Sidekiq 5.1. Older versions will
report all errors for jobs.

Part of #1091

Author: tombruijn

.changesets/report-sidekiq-exceptions-on-discard.md

@@ -0,0 +1,6 @@
+---
+bump: minor
+type: add
+---
+
+Report Sidekiq errors when a job is dead/discarded. Configure the new `sidekiq_report_errors` config option to "discard" to only report errors when the job is not retried further.

lib/appsignal/config.rb

@@ -47,6 +47,7 @@ class Config
       ],
       :send_environment_metadata => true,
       :send_params => true,
+      :sidekiq_report_errors => "all",
       :transaction_debug_mode => false
     }.freeze
 
@@ -108,6 +109,7 @@ class Config
       "APPSIGNAL_SEND_ENVIRONMENT_METADATA" => :send_environment_metadata,
       "APPSIGNAL_SEND_PARAMS" => :send_params,
       "APPSIGNAL_SEND_SESSION_DATA" => :send_session_data,
+      "APPSIGNAL_SIDEKIQ_REPORT_ERRORS" => :sidekiq_report_errors,
       "APPSIGNAL_SKIP_SESSION_DATA" => :skip_session_data,
       "APPSIGNAL_STATSD_PORT" => :statsd_port,
       "APPSIGNAL_TRANSACTION_DEBUG_MODE" => :transaction_debug_mode,
@@ -130,6 +132,7 @@ class Config
       APPSIGNAL_LOGGING_ENDPOINT
       APPSIGNAL_PUSH_API_ENDPOINT
       APPSIGNAL_PUSH_API_KEY
+      APPSIGNAL_SIDEKIQ_REPORT_ERRORS
       APPSIGNAL_STATSD_PORT
       APPSIGNAL_WORKING_DIRECTORY_PATH
       APPSIGNAL_WORKING_DIR_PATH
@@ -563,6 +566,11 @@ def determine_overrides
         config[:activejob_report_errors] = "all"
       end
 
+      if config_hash[:sidekiq_report_errors] == "discard" &&
+          !Appsignal::Hooks::SidekiqHook.version_5_1_or_higher?
+        config[:sidekiq_report_errors] = "all"
+      end
+
       config
     end
 

lib/appsignal/hooks/sidekiq.rb

@@ -5,17 +5,34 @@ class Hooks
     class SidekiqHook < Appsignal::Hooks::Hook
       register :sidekiq
 
-      def dependencies_present?
+      def self.version_5_1_or_higher?
+        @version_5_1_or_higher ||=
+          if dependencies_present?
+            Gem::Version.new(::Sidekiq::VERSION) >= Gem::Version.new("5.1.0")
+          else
+            false
+          end
+      end
+
+      def self.dependencies_present?
         defined?(::Sidekiq)
       end
 
+      def dependencies_present?
+        self.class.dependencies_present?
+      end
+
       def install
         require "appsignal/integrations/sidekiq"
         Appsignal::Probes.register :sidekiq, Appsignal::Probes::SidekiqProbe
 
         ::Sidekiq.configure_server do |config|
           config.error_handlers <<
             Appsignal::Integrations::SidekiqErrorHandler.new
+          if config.respond_to? :death_handlers
+            config.death_handlers <<
+              Appsignal::Integrations::SidekiqDeathHandler.new
+          end
 
           config.server_middleware do |chain|
             if chain.respond_to? :prepend

lib/appsignal/integrations/sidekiq.rb

@@ -4,32 +4,51 @@
 
 module Appsignal
   module Integrations
+    # Handler for job death events. We get notified when a job has exhausted
+    # its retries.
+    #
+    # This is called before the SidekiqErrorHandler so it doesn't need to worry
+    # about completing the transaction.
+    #
+    # Introduced in Sidekiq 5.1.
+    class SidekiqDeathHandler
+      def call(_job_context, exception)
+        return unless Appsignal.config[:sidekiq_report_errors] == "discard"
+
+        transaction = Appsignal::Transaction.current
+        transaction.set_error(exception)
+      end
+    end
+
     # Error handler for Sidekiq to report errors from jobs and internal Sidekiq
     # errors.
     #
     # @api private
     class SidekiqErrorHandler
+      # Sidekiq 7.1.5 introduced the third sidekiq_config argument. It is not
+      # given on older Sidekiq versions.
       def call(exception, sidekiq_context, _sidekiq_config = nil)
-        transaction =
-          if Appsignal::Transaction.current?
-            Appsignal::Transaction.current
-          else
-            # Sidekiq error outside of the middleware scope.
-            # Can be a job JSON parse error or some other error happening in
-            # Sidekiq.
-            transaction =
-              Appsignal::Transaction.create(
-                SecureRandom.uuid, # Newly generated job id
-                Appsignal::Transaction::BACKGROUND_JOB,
-                Appsignal::Transaction::GenericRequest.new({})
-              )
-            transaction.set_action_if_nil("SidekiqInternal")
-            transaction.set_metadata("sidekiq_error", sidekiq_context[:context])
-            transaction.params = { :jobstr => sidekiq_context[:jobstr] }
-            transaction
+        if Appsignal::Transaction.current?
+          if Appsignal.config[:sidekiq_report_errors] == "all"
+            transaction = Appsignal::Transaction.current
+            transaction.set_error(exception)
           end
+        else
+          # Sidekiq error outside of the middleware scope.
+          # Can be a job JSON parse error or some other error happening in
+          # Sidekiq.
+          transaction =
+            Appsignal::Transaction.create(
+              SecureRandom.uuid, # Newly generated job id
+              Appsignal::Transaction::BACKGROUND_JOB,
+              Appsignal::Transaction::GenericRequest.new({})
+            )
+          transaction.set_action_if_nil("SidekiqInternal")
+          transaction.set_metadata("sidekiq_error", sidekiq_context[:context])
+          transaction.params = { :jobstr => sidekiq_context[:jobstr] }
+          transaction.set_error(exception)
+        end
 
-        transaction.set_error(exception)
         Appsignal::Transaction.complete_current!
       end
     end

spec/integration/diagnose

@@ -187,6 +187,7 @@
         :send_environment_metadata      => true,
         :send_params                    => true,
         :send_session_data              => true,
+        :sidekiq_report_errors          => "all",
         :transaction_debug_mode         => false
       )
     end
@@ -583,6 +584,33 @@
         end
       end
     end
+
+    context "sidekiq_report_errors" do
+      let(:config_options) { { :sidekiq_report_errors => "discard" } }
+      before do
+        if Appsignal::Hooks::SidekiqHook.instance_variable_defined?(:@version_5_1_or_higher)
+          Appsignal::Hooks::SidekiqHook.remove_instance_variable(:@version_5_1_or_higher)
+        end
+      end
+
+      context "when Sidekiq >= 5.1 and 'discard'" do
+        before { stub_const("Sidekiq::VERSION", "5.1.0") }
+
+        it "does not override the sidekiq_report_errors value" do
+          expect(config[:sidekiq_report_errors]).to eq("discard")
+          expect(config.override_config[:sidekiq_report_errors]).to be_nil
+        end
+      end
+
+      context "when Sidekiq < 5.1 and 'discard'" do
+        before { stub_const("Sidekiq::VERSION", "5.0.0") }
+
+        it "sets sidekiq_report_errors to 'all'" do
+          expect(config[:sidekiq_report_errors]).to eq("all")
+          expect(config.override_config[:sidekiq_report_errors]).to eq("all")
+        end
+      end
+    end
   end
 
   describe "config keys" do