Merge pull request #79 from bensheldon/error_tracking

Capture errors via instrumentation from retry_on and discard_on

gemfiles/rails_5.2.gemfile.lock

@@ -202,6 +202,7 @@ GEM
     websocket-driver (0.7.3)
       websocket-extensions (>= 0.1.0)
     websocket-extensions (0.1.5)
+    yard (0.9.25)
 
 PLATFORMS
   ruby
@@ -222,6 +223,7 @@ DEPENDENCIES
   rubocop-performance
   rubocop-rails
   rubocop-rspec
+  yard
 
 BUNDLED WITH
    2.1.4

gemfiles/rails_6.0.gemfile.lock

@@ -217,6 +217,7 @@ GEM
     websocket-driver (0.7.3)
       websocket-extensions (>= 0.1.0)
     websocket-extensions (0.1.5)
+    yard (0.9.25)
     zeitwerk (2.3.1)
 
 PLATFORMS
@@ -238,6 +239,7 @@ DEPENDENCIES
   rubocop-performance
   rubocop-rails
   rubocop-rspec
+  yard
 
 BUNDLED WITH
    2.1.4

lib/good_job.rb

@@ -10,6 +10,7 @@
 require 'good_job/adapter'
 require 'good_job/pg_locks'
 require 'good_job/performer'
+require 'good_job/current_execution'
 
 require 'active_job/queue_adapters/good_job_adapter'
 

lib/good_job/current_execution.rb

@@ -0,0 +1,25 @@
+module GoodJob
+  # Thread-local attributes for passing values from Instrumentation.
+  # (Cannot use ActiveSupport::CurrentAttributes because ActiveJob resets it)
+
+  module CurrentExecution
+    # @!attribute [rw] error_on_retry
+    #   @!scope class
+    #   Error captured by retry_on
+    #   @return [Exception, nil]
+    thread_mattr_accessor :error_on_retry
+
+    # @!attribute [rw] error_on_discard
+    #   @!scope class
+    #   Error captured by discard_on
+    #   @return [Exception, nil]
+    thread_mattr_accessor :error_on_discard
+
+    # Resets attributes
+    # @return [void]
+    def self.reset
+      self.error_on_retry = nil
+      self.error_on_discard = nil
+    end
+  end
+end

lib/good_job/job.rb

@@ -76,11 +76,11 @@ def self.enqueue(active_job, scheduled_at: nil, create_with_advisory_lock: false
     def perform(destroy_after: !GoodJob.preserve_job_records, reperform_on_standard_error: GoodJob.reperform_jobs_on_standard_error)
       raise PreviouslyPerformedError, 'Cannot perform a job that has already been performed' if finished_at
 
+      GoodJob::CurrentExecution.reset
       result = nil
       rescued_error = nil
       error = nil
 
-      ActiveSupport::Notifications.instrument("before_perform_job.good_job", { good_job: self })
       self.performed_at = Time.current
       save! unless destroy_after
 
@@ -96,11 +96,16 @@ def perform(destroy_after: !GoodJob.preserve_job_records, reperform_on_standard_
         rescued_error = e
       end
 
+      retry_or_discard_error = GoodJob::CurrentExecution.error_on_retry ||
+                               GoodJob::CurrentExecution.error_on_discard
+
       if rescued_error
         error = rescued_error
       elsif result.is_a?(Exception)
         error = result
         result = nil
+      elsif retry_or_discard_error
+        error = retry_or_discard_error
       end
 
       error_message = "#{error.class}: #{error.message}" if error

lib/good_job/railtie.rb

@@ -4,5 +4,15 @@ class Railtie < ::Rails::Railtie
       ActiveSupport.on_load(:good_job) { self.logger = ::Rails.logger }
       GoodJob::LogSubscriber.attach_to :good_job
     end
+
+    initializer "good_job.active_job_notifications" do
+      ActiveSupport::Notifications.subscribe "enqueue_retry.active_job" do |event|
+        GoodJob::CurrentExecution.error_on_retry = event.payload[:error]
+      end
+
+      ActiveSupport::Notifications.subscribe "discard.active_job" do |event|
+        GoodJob::CurrentExecution.error_on_discard = event.payload[:error]
+      end
+    end
   end
 end

lib/good_job/scheduler.rb

@@ -28,10 +28,10 @@ class Scheduler
       fallback_policy: :discard,
     }.freeze
 
-    # All instantiated Schedulers in the current process.
-    # @!scope class
     # @!attribute [r] instances
-    # @return [array<GoodJob:Scheduler>]
+    #   @!scope class
+    #   All instantiated Schedulers in the current process.
+    #   @return [array<GoodJob:Scheduler>]
     cattr_reader :instances, default: [], instance_reader: false
 
     # Creates GoodJob::Scheduler(s) and Performers from a GoodJob::Configuration instance.

spec/lib/good_job/current_execution_spec.rb

@@ -0,0 +1,55 @@
+require 'rails_helper'
+
+RSpec.describe GoodJob::CurrentExecution do
+  describe '.error_on_discard' do
+    it 'maintains value across threads' do
+      described_class.error_on_discard = 'apple'
+
+      Thread.new do
+        described_class.error_on_discard = 'bear'
+      end.join
+
+      expect(described_class.error_on_discard).to eq 'apple'
+    end
+
+    it 'maintains value across Rails execution wrapper' do
+      Rails.application.executor.wrap do
+        described_class.error_on_discard = 'apple'
+      end
+
+      expect(described_class.error_on_discard).to eq 'apple'
+    end
+
+    it 'is resettable' do
+      described_class.error_on_discard = 'apple'
+      described_class.reset
+      expect(described_class.error_on_discard).to eq nil
+    end
+  end
+
+  describe '.error_on_retry' do
+    it 'maintains value across threads' do
+      described_class.error_on_retry = 'apple'
+
+      Thread.new do
+        described_class.error_on_retry = 'bear'
+      end.join
+
+      expect(described_class.error_on_retry).to eq 'apple'
+    end
+
+    it 'maintains value across Rails execution wrapper' do
+      Rails.application.executor.wrap do
+        described_class.error_on_retry = 'apple'
+      end
+
+      expect(described_class.error_on_retry).to eq 'apple'
+    end
+
+    it 'is resettable' do
+      described_class.error_on_retry = 'apple'
+      described_class.reset
+      expect(described_class.error_on_retry).to eq nil
+    end
+  end
+end

spec/lib/good_job/job_spec.rb

@@ -105,11 +105,78 @@ def perform(result_value = nil, raise_error: false)
     let(:active_job) { ExampleJob.new("a string") }
     let!(:good_job) { described_class.enqueue(active_job) }
 
-    it 'returns the results of the job' do
-      result, error = good_job.perform
+    describe 'return value' do
+      it 'returns the results of the job' do
+        result, error = good_job.perform
+
+        expect(result).to eq "a string"
+        expect(error).to be_nil
+      end
+
+      context 'when there is an error' do
+        let(:active_job) { ExampleJob.new("whoops", raise_error: true) }
+
+        it 'returns the error' do
+          result, error = good_job.perform
+
+          expect(result).to eq nil
+          expect(error).to be_an_instance_of ExpectedError
+        end
 
-      expect(result).to eq "a string"
-      expect(error).to be_nil
+        context 'when error monitoring service intercepts exception' do
+          before do
+            # Similar to Sentry monitor's implementation
+            # https://github.com/getsentry/raven-ruby/blob/20b260a6d04e0ca01d5cddbd9728e6fc8ae9a90c/lib/raven/integrations/rails/active_job.rb#L21-L31
+            ExampleJob.around_perform do |_job, block|
+              begin
+                block.call
+              rescue StandardError => e
+                next if rescue_with_handler(e)
+
+                raise e
+              ensure
+                nil
+              end
+            end
+          end
+
+          it 'returns the error' do
+            result, error = good_job.perform
+
+            expect(result).to eq nil
+            expect(error).to be_an_instance_of ExpectedError
+          end
+
+          if Gem::Version.new(Rails.version) > Gem::Version.new("6")
+            # Necessary instrumentation was added in Rails 6.0
+            context 'when retry_on is used' do
+              before do
+                ExampleJob.retry_on(StandardError, wait: 0, attempts: Float::INFINITY) { nil }
+              end
+
+              it 'returns the error' do
+                result, error = good_job.perform
+
+                expect(result).to eq nil
+                expect(error).to be_an_instance_of ExpectedError
+              end
+            end
+
+            context 'when discard_on is used' do
+              before do
+                ExampleJob.discard_on(StandardError) { nil }
+              end
+
+              it 'returns the error' do
+                result, error = good_job.perform
+
+                expect(result).to eq nil
+                expect(error).to be_an_instance_of ExpectedError
+              end
+            end
+          end
+        end
+      end
     end
 
     it 'destroys the job' do
@@ -134,7 +201,7 @@ def perform(result_value = nil, raise_error: false)
       let(:active_job) { ExampleJob.new("a string", raise_error: true) }
 
       before do
-        ExampleJob.retry_on StandardError, wait: 0, attempts: Float::INFINITY
+        ExampleJob.retry_on(StandardError, wait: 0, attempts: Float::INFINITY) { nil }
       end
 
       it 'returns the results of the job' do