Introduce active_job_report_on_retry_error (#2617)

* Set active_job_report_after_job_retries to true by default

Refs #2598
Refs #2597

* Use test jobs defined in support file

* Introduce active_job_report_on_retry_error

* Fix spec

* Adjust spec skipping logic

* Update CHANGELOG.md

* Update CHANGELOG.md

Author: solnic

CHANGELOG.md

@@ -13,7 +13,8 @@
   config.sidekiq.propagate_traces = false unless Rails.const_defined?('Server')
   ```
 - Only expose `active_storage` keys on span data if `send_default_pii` is on ([#2589](https://github.com/getsentry/sentry-ruby/pull/2589))
-- Add `Sentry.capture_log` ([#2606](https://github.com/getsentry/sentry-ruby/pull/2606))
+- Add `Sentry.capture_log` ([#2606](https://github.com/getsentry/sentry-ruby/pull/2617))
+- New configuration option called `active_job_report_on_retry_error` which enables reporting errors on each retry error ([#2617](https://github.com/getsentry/sentry-ruby/pull/2617))
 
 ### Bug Fixes
 
@@ -28,7 +29,6 @@
 ### Internal
 
 - Remove `user_segment` from DSC ([#2586](https://github.com/getsentry/sentry-ruby/pull/2586))
-- New configuration option called `report_after_job_retries` for ActiveJob which makes reporting exceptions only happen when the last retry attempt failed ([#2500](https://github.com/getsentry/sentry-ruby/pull/2500))
 
 ## 5.23.0
 

sentry-rails/lib/sentry/rails/active_job.rb

@@ -24,8 +24,7 @@ class SentryReporter
         SPAN_ORIGIN = "auto.queue.active_job"
 
         EVENT_HANDLERS = {
-          "enqueue_retry.active_job" => :retry_handler,
-          "retry_stopped.active_job" => :retry_stopped_handler
+          "enqueue_retry.active_job" => :retry_handler
         }
 
         class << self
@@ -53,19 +52,13 @@ def record(job, &block)
               rescue Exception => e # rubocop:disable Lint/RescueException
                 finish_sentry_transaction(transaction, 500)
 
-                maybe_capture_exception(job, e)
+                capture_exception(job, e)
 
                 raise
               end
             end
           end
 
-          def maybe_capture_exception(job, e)
-            unless Sentry.configuration.rails.active_job_report_after_job_retries
-              capture_exception(job, e)
-            end
-          end
-
           def capture_exception(job, e)
             Sentry::Rails.capture_exception(
               e,
@@ -92,24 +85,11 @@ def detach_event_handlers
             subscribers.clear
           end
 
-          # This handler does not capture error when `active_job_report_after_job_retries` is true
-          # because in such case only the last retry that failed will capture exception
+          # This handler does not capture error unless `active_job_report_on_retry_error` is true
           def retry_handler(*args)
             handle_error_event(*args) do |job, error|
               return if !Sentry.initialized? || job.already_supported_by_sentry_integration?
-              return if Sentry.configuration.rails.active_job_report_after_job_retries
-
-              capture_exception(job, error)
-            end
-          end
-
-          # This handler does not capture error when `active_job_report_after_job_retries` is false
-          # because in such cases regular execution flow that failed will capture it in `record`
-          # method
-          def retry_stopped_handler(*args)
-            handle_error_event(*args) do |job, error|
-              return if !Sentry.initialized? || job.already_supported_by_sentry_integration?
-              return unless Sentry.configuration.rails.active_job_report_after_job_retries
+              return unless Sentry.configuration.rails.active_job_report_on_retry_error
 
               capture_exception(job, error)
             end

sentry-rails/lib/sentry/rails/configuration.rb

@@ -156,9 +156,8 @@ class Configuration
       # @return [Hash<String, Array<Symbol>>]
       attr_accessor :active_support_logger_subscription_items
 
-      # Set this option to true if you want Sentry to only capture the last job
-      # retry if it fails.
-      attr_accessor :active_job_report_after_job_retries
+      # Set this option to true if you want Sentry to capture each retry failure
+      attr_accessor :active_job_report_on_retry_error
 
       def initialize
         @register_error_subscriber = false
@@ -176,7 +175,7 @@ def initialize
         @enable_db_query_source = true
         @db_query_source_threshold_ms = 100
         @active_support_logger_subscription_items = Sentry::Rails::ACTIVE_SUPPORT_LOGGER_SUBSCRIPTION_ITEMS_DEFAULT.dup
-        @active_job_report_after_job_retries = false
+        @active_job_report_on_retry_error = false
       end
     end
   end

sentry-rails/spec/sentry/rails/activejob_spec.rb

@@ -1,77 +1,6 @@
 # frozen_string_literal: true
 
-require "spec_helper"
-require "active_job/railtie"
-
-class NormalJob < ActiveJob::Base
-  def perform
-    "foo"
-  end
-end
-
-class FailedJob < ActiveJob::Base
-  self.logger = nil
-
-  class TestError < RuntimeError
-  end
-
-  def perform
-    a = 1
-    b = 0
-    raise TestError, "Boom!"
-  end
-end
-
-class FailedWithExtraJob < FailedJob
-  def perform
-    Sentry.get_current_scope.set_extras(foo: :bar)
-    super
-  end
-end
-
-class JobWithArgument < ActiveJob::Base
-  def perform(*args, integer:, post:, **options)
-    raise "foo"
-  end
-end
-
-class QueryPostJob < ActiveJob::Base
-  self.logger = nil
-
-  def perform
-    Post.all.to_a
-  end
-end
-
-class RescuedActiveJob < FailedWithExtraJob
-  rescue_from TestError, with: :rescue_callback
-
-  def rescue_callback(error); end
-end
-
-class ProblematicRescuedActiveJob < FailedWithExtraJob
-  rescue_from TestError, with: :rescue_callback
-
-  def rescue_callback(error)
-    raise "foo"
-  end
-end
-
-class NormalJobWithCron < NormalJob
-  include Sentry::Cron::MonitorCheckIns
-  sentry_monitor_check_ins
-end
-
-class FailedJobWithCron < FailedJob
-  include Sentry::Cron::MonitorCheckIns
-  sentry_monitor_check_ins slug: "failed_job", monitor_config: Sentry::Cron::MonitorConfig.from_crontab("5 * * * *")
-end
-
-class FailedJobWithRetryOn < FailedJob
-  if respond_to? :retry_on
-    retry_on StandardError, attempts: 3, wait: 0
-  end
-end
+require_relative "../../support/test_jobs"
 
 RSpec.describe "without Sentry initialized", type: :job do
   it "runs job" do
@@ -109,7 +38,10 @@ class FailedJobWithRetryOn < FailedJob
         JobWithArgument.perform_now("foo", { bar: Sentry }, integer: 1, post: post, nested: { another_level: { post: post2 } })
       end.to raise_error(RuntimeError)
 
+      expect(transport.events.size).to be(1)
+
       event = transport.events.last.to_json_compatible
+
       expect(event.dig("extra", "arguments")).to eq(
         [
           "foo",
@@ -134,7 +66,10 @@ def post.to_global_id
         JobWithArgument.perform_now(integer: 1, post: post)
       end.to raise_error(RuntimeError)
 
+      expect(transport.events.size).to be(1)
+
       event = transport.events.last.to_json_compatible
+
       expect(event.dig("extra", "arguments")).to eq(
         [
           {
@@ -152,7 +87,10 @@ def post.to_global_id
         JobWithArgument.perform_now("foo", { bar: Sentry }, integer: 1, post: post, range: 1..3)
       end.to raise_error(RuntimeError)
 
+      expect(transport.events.size).to be(1)
+
       event = transport.events.last.to_json_compatible
+
       expect(event.dig("extra", "arguments")).to eq(
         [
           "foo",
@@ -174,7 +112,10 @@ def post.to_global_id
         JobWithArgument.perform_now("foo", { bar: Sentry }, integer: 1, post: post, range: range)
       end.to raise_error(RuntimeError)
 
+      expect(transport.events.size).to be(1)
+
       event = transport.events.last.to_json_compatible
+
       expect(event.dig("extra", "arguments")).to eq(
         [
           "foo",
@@ -192,7 +133,10 @@ def post.to_global_id
   it "adds useful context to extra" do
     expect { FailedJob.perform_now }.to raise_error(FailedJob::TestError)
 
+    expect(transport.events.size).to be(1)
+
     event = transport.events.last.to_json_compatible
+
     expect(event.dig("extra", "active_job")).to eq("FailedJob")
     expect(event.dig("extra", "job_id")).to be_a(String)
     expect(event.dig("extra", "provider_job_id")).to be_nil
@@ -207,6 +151,8 @@ def post.to_global_id
   it "clears context" do
     expect { FailedWithExtraJob.perform_now }.to raise_error(FailedWithExtraJob::TestError)
 
+    expect(transport.events.size).to be(1)
+
     event = transport.events.last.to_json_compatible
 
     expect(event["extra"]["foo"]).to eq("bar")
@@ -224,7 +170,8 @@ def post.to_global_id
     it "sends transaction" do
       QueryPostJob.perform_now
 
-      expect(transport.events.count).to eq(1)
+      expect(transport.events.size).to be(1)
+
       transaction = transport.events.last
       expect(transaction.transaction).to eq("QueryPostJob")
       expect(transaction.transaction_info).to eq({ source: :task })
@@ -242,7 +189,7 @@ def post.to_global_id
       it "sends transaction and associates it with the event" do
         expect { FailedWithExtraJob.perform_now }.to raise_error(FailedWithExtraJob::TestError)
 
-        expect(transport.events.count).to eq(2)
+        expect(transport.events.size).to be(2)
 
         transaction = transport.events.first
         expect(transaction.transaction).to eq("FailedWithExtraJob")
@@ -277,7 +224,7 @@ def perform
         DeserializationErrorJob.perform_now
       end.to raise_error(ActiveJob::DeserializationError, /divided by 0/)
 
-      expect(transport.events.size).to eq(1)
+      expect(transport.events.size).to be(1)
 
       event = transport.events.last.to_json_compatible
       expect(event.dig("exception", "values", 0, "type")).to eq("ZeroDivisionError")
@@ -434,14 +381,22 @@ def perform(event, hint)
     end
   end
 
-  describe "active_job_report_after_job_retries", skip: RAILS_VERSION < 7.0 do
+  describe "Reporting on retry errors", skip: RAILS_VERSION < 7.0 do
     before do
-      if defined?(JRUBY_VERSION) && JRUBY_VERSION == "9.4.12.0" && RAILS_VERSION <= 7.1
+      if defined?(JRUBY_VERSION) && JRUBY_VERSION == "9.4.12.0"
         skip "This crashes on jruby + rails 7.0.0.x. See https://github.com/getsentry/sentry-ruby/issues/2612"
       end
     end
 
-    context "when active_job_report_after_job_retries is false" do
+    context "when active_job_report_on_retry_error is true" do
+      before do
+        Sentry.configuration.rails.active_job_report_on_retry_error = true
+      end
+
+      after do
+        Sentry.configuration.rails.active_job_report_on_retry_error = false
+      end
+
       it "reports 3 exceptions" do
         allow(Sentry::Rails::ActiveJobExtensions::SentryReporter)
           .to receive(:capture_exception).and_call_original
@@ -458,16 +413,8 @@ def perform(event, hint)
       end
     end
 
-    context "when active_job_report_after_job_retries is true" do
-      before do
-        Sentry.configuration.rails.active_job_report_after_job_retries = true
-      end
-
-      after do
-        Sentry.configuration.rails.active_job_report_after_job_retries = false
-      end
-
-      it "reports 1 exception" do
+    context "when active_job_report_on_retry_error is false" do
+      it "reports 1 exception on final attempt failure" do
         allow(Sentry::Rails::ActiveJobExtensions::SentryReporter)
           .to receive(:capture_exception).and_call_original
 

sentry-rails/spec/sentry/rails/configuration_spec.rb

@@ -60,9 +60,9 @@ class MySubscriber; end
     end
   end
 
-  describe "#active_job_report_after_job_retries" do
+  describe "#active_job_report_on_retry_error" do
     it "has correct default value" do
-      expect(subject.active_job_report_after_job_retries).to eq(false)
+      expect(subject.active_job_report_on_retry_error).to be(false)
     end
   end
 end

sentry-rails/spec/support/test_jobs.rb

@@ -65,3 +65,9 @@ class FailedJobWithCron < FailedJob
   include Sentry::Cron::MonitorCheckIns
   sentry_monitor_check_ins slug: "failed_job", monitor_config: Sentry::Cron::MonitorConfig.from_crontab("5 * * * *")
 end
+
+class FailedJobWithRetryOn < FailedJob
+  if respond_to? :retry_on
+    retry_on StandardError, attempts: 3, wait: 0
+  end
+end