Report Active Job errors on discard if configured

Since Active Job 7.1, there's an `after_discard` callback available. We can use that to finish our `activejob_report_errors` config option implementation, started in PR #1040. If the `activejob_report_errors` option is set to `discard`, our gem will only report the error of the last job retry, when it is discarded. Closes #1050
appsignal · May 8, 2024 · 28a36ba · 28a36ba
1 parent 8a8ad81
commit 28a36ba
Show file tree

Hide file tree

Showing 3 changed files with 98 additions and 7 deletions.
diff --git a/.changesets/only-report-error-on-active-job-discard.md b/.changesets/only-report-error-on-active-job-discard.md
@@ -0,0 +1,15 @@
+---
+bump: "patch"
+type: "add"
+---
+
+Add option to `activejob_report_errors` option to only report errors when a job is discard by Active Job. In the example below the job is retried twice. If it fails with an error twice the job is discarded. If `activejob_report_errors` is set to `discard`, you will only get an error reported when the job is discarded. This new `discard` value only works for Active Job 7.1 and newer.
+
+
+```ruby
+class ExampleJob < ActiveJob::Base
+  retry_on StandardError, :attempts => 2
+
+  # ...
+end
+```
diff --git a/lib/appsignal/hooks/active_job.rb b/lib/appsignal/hooks/active_job.rb
@@ -6,6 +6,12 @@ class Hooks
     class ActiveJobHook < Appsignal::Hooks::Hook
       register :active_job
 
+      def self.version_7_1_or_higher?
+        major = ::ActiveJob::VERSION::MAJOR
+        minor = ::ActiveJob::VERSION::MINOR
+        major > 7 || (major == 7 && minor >= 1)
+      end
+
       def dependencies_present?
         defined?(::ActiveJob)
       end
@@ -14,6 +20,14 @@ def install
         ActiveSupport.on_load(:active_job) do
           ::ActiveJob::Base
             .extend ::Appsignal::Hooks::ActiveJobHook::ActiveJobClassInstrumentation
+          return unless Appsignal::Hooks::ActiveJobHook.version_7_1_or_higher?
+
+          # Only works on ActiveJob 7.1 and newer
+          ::ActiveJob::Base.after_discard do |_job, exception|
+            next unless Appsignal.config[:activejob_report_errors] == "discard"
+
+            Appsignal::Transaction.current.set_error(exception)
+          end
         end
       end
 
@@ -86,7 +100,9 @@ def execute(job)
         private
 
         def transaction_set_error(transaction, exception)
-          return if Appsignal.config[:activejob_report_errors] == "none"
+          # Only report errors when the config option is set to "all".
+          # To report errors on discard, see the `after_discard` callback.
+          return unless Appsignal.config[:activejob_report_errors] == "all"
 
           transaction.set_error(exception)
         end

diff --git a/spec/lib/appsignal/hooks/activejob_spec.rb b/spec/lib/appsignal/hooks/activejob_spec.rb
@@ -88,6 +88,14 @@ def perform
         end
       end
 
+      class ActiveJobErrorWithRetryTestJob < ActiveJob::Base
+        retry_on StandardError, :wait => 0.seconds, :attempts => 2
+
+        def perform
+          raise "uh oh"
+        end
+      end
+
       class ActiveJobCustomQueueTestJob < ActiveJob::Base
         queue_as :custom_queue
 
@@ -99,6 +107,7 @@ def perform(*_args)
     after do
       Object.send(:remove_const, :ActiveJobTestJob)
       Object.send(:remove_const, :ActiveJobErrorTestJob)
+      Object.send(:remove_const, :ActiveJobErrorWithRetryTestJob)
       Object.send(:remove_const, :ActiveJobCustomQueueTestJob)
     end
 
@@ -227,23 +236,64 @@ def perform(*_args)
           Appsignal.config = project_fixture_config("production")
           Appsignal.config[:activejob_report_errors] = "none"
 
-          # Other calls we're testing in another test
           allow(Appsignal).to receive(:increment_counter)
           tags = { :queue => queue }
           expect(Appsignal).to receive(:increment_counter)
             .with("active_job_queue_job_count", 1, tags.merge(:status => :failed))
-          expect(Appsignal).to receive(:increment_counter)
-            .with("active_job_queue_job_count", 1, tags.merge(:status => :processed))
 
           expect do
             queue_job(ActiveJobErrorTestJob)
           end.to raise_error(RuntimeError, "uh oh")
 
           transaction = last_transaction
           transaction_hash = transaction.to_h
-          expect(transaction_hash).to include(
-            "error" => nil
-          )
+          expect(transaction_hash).to include("error" => nil)
+        end
+      end
+
+      if DependencyHelper.rails_version >= Gem::Version.new("7.1.0")
+        context "with activejob_report_errors set to discard" do
+          before do
+            Appsignal.config = project_fixture_config("production")
+            Appsignal.config[:activejob_report_errors] = "discard"
+          end
+
+          it "does not report error on first failure" do
+            with_test_adapter do
+              # Prevent the job from being instantly retried so we can test
+              # what happens before it's retried
+              allow_any_instance_of(ActiveJobErrorWithRetryTestJob).to receive(:retry_job)
+
+              queue_job(ActiveJobErrorWithRetryTestJob)
+            end
+
+            transaction = last_transaction
+            transaction_hash = transaction.to_h
+            expect(transaction_hash).to include("error" => nil)
+          end
+
+          it "reports error when discarding the job" do
+            allow(Appsignal).to receive(:increment_counter)
+            tags = { :queue => queue }
+            expect(Appsignal).to receive(:increment_counter)
+              .with("active_job_queue_job_count", 1, tags.merge(:status => :failed))
+
+            with_test_adapter do
+              expect do
+                queue_job(ActiveJobErrorWithRetryTestJob)
+              end.to raise_error(RuntimeError, "uh oh")
+            end
+
+            transaction = last_transaction
+            transaction_hash = transaction.to_h
+            expect(transaction_hash).to include(
+              "error" => {
+                "name" => "RuntimeError",
+                "message" => "uh oh",
+                "backtrace" => kind_of(String)
+              }
+            )
+          end
         end
       end
 
@@ -607,6 +657,16 @@ def welcome(*_args)
       end
     end
 
+    def with_test_adapter
+      ActiveJob::Base.queue_adapter = :test
+      ActiveJob::Base.queue_adapter.performed_jobs.clear
+      ActiveJob::Base.queue_adapter.perform_enqueued_jobs = true
+      ActiveJob::Base.queue_adapter.perform_enqueued_at_jobs = true
+      yield
+    ensure
+      ActiveJob::Base.queue_adapter = :inline # Restore to default
+    end
+
     def perform_active_job(&block)
       Timecop.freeze(time, &block)
     end