Merge pull request #1104 from appsignal/sidekiq-error-on-discard

Report Sidekiq errors when the job is discarded

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