diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b0bcb716..72df9e0b2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,7 @@ - Fixes [#2478](https://github.com/getsentry/sentry-ruby/issues/2478) - Fix compatibility issues with sidekiq-cron 2.2.0 ([#2591](https://github.com/getsentry/sentry-ruby/pull/2591)) - Update sentry-sidekiq to work correctly with Sidekiq 8.0 and its new timestamp format ([#2570](https://github.com/getsentry/sentry-ruby/pull/2570)) +- Ensure we capture exceptions after each job retry ([#2597](https://github.com/getsentry/sentry-ruby/pull/2597)) ### Internal diff --git a/sentry-rails/lib/sentry/rails/active_job.rb b/sentry-rails/lib/sentry/rails/active_job.rb index 8fa1f8716..9b5bc7176 100644 --- a/sentry-rails/lib/sentry/rails/active_job.rb +++ b/sentry-rails/lib/sentry/rails/active_job.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +require "set" + module Sentry module Rails module ActiveJobExtensions @@ -20,7 +22,11 @@ def already_supported_by_sentry_integration? class SentryReporter OP_NAME = "queue.active_job" SPAN_ORIGIN = "auto.queue.active_job" - NOTIFICATION_NAME = "retry_stopped.active_job" + + EVENT_HANDLERS = { + "enqueue_retry.active_job" => :retry_handler, + "retry_stopped.active_job" => :retry_stopped_handler + } class << self def record(job, &block) @@ -47,15 +53,19 @@ def record(job, &block) rescue Exception => e # rubocop:disable Lint/RescueException finish_sentry_transaction(transaction, 500) - unless Sentry.configuration.rails.active_job_report_after_job_retries - capture_exception(job, e) - end + maybe_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, @@ -67,29 +77,47 @@ def capture_exception(job, e) ) end - def register_retry_stopped_subscriber - unless @retry_stopped_subscriber - @retry_stopped_subscriber = ActiveSupport::Notifications.subscribe(NOTIFICATION_NAME) do |*args| - retry_stopped_handler(*args) + def register_event_handlers + EVENT_HANDLERS.each do |name, handler| + subscribers << ActiveSupport::Notifications.subscribe(name) do |*args| + public_send(handler, *args) end end end - def detach_retry_stopped_subscriber - if @retry_stopped_subscriber - ActiveSupport::Notifications.unsubscribe(@retry_stopped_subscriber) - @retry_stopped_subscriber = nil + def detach_event_handlers + subscribers.each do |subscriber| + ActiveSupport::Notifications.unsubscribe(subscriber) + end + 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 + 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) - event = ActiveSupport::Notifications::Event.new(*args) - job = event.payload[:job] - error = event.payload[:error] + 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 if !Sentry.initialized? || job.already_supported_by_sentry_integration? - return unless Sentry.configuration.rails.active_job_report_after_job_retries - capture_exception(job, error) + capture_exception(job, error) + end + end + + def handle_error_event(*args) + event = ActiveSupport::Notifications::Event.new(*args) + yield(event.payload[:job], event.payload[:error]) end def finish_sentry_transaction(transaction, status) @@ -128,6 +156,12 @@ def sentry_serialize_arguments(argument) argument end end + + private + + def subscribers + @__subscribers__ ||= Set.new + end end end end diff --git a/sentry-rails/lib/sentry/rails/railtie.rb b/sentry-rails/lib/sentry/rails/railtie.rb index 2025c7c88..d2cf03d57 100644 --- a/sentry-rails/lib/sentry/rails/railtie.rb +++ b/sentry-rails/lib/sentry/rails/railtie.rb @@ -53,7 +53,9 @@ class Railtie < ::Rails::Railtie register_error_subscriber(app) if ::Rails.version.to_f >= 7.0 && Sentry.configuration.rails.register_error_subscriber # Presence of ActiveJob is no longer a reliable cue - register_retry_stopped_subscriber if defined?(Sentry::Rails::ActiveJobExtensions) + if defined?(Sentry::Rails::ActiveJobExtensions) + Sentry::Rails::ActiveJobExtensions::SentryReporter.register_event_handlers + end end runner do @@ -140,9 +142,5 @@ def register_error_subscriber(app) require "sentry/rails/error_subscriber" app.executor.error_reporter.subscribe(Sentry::Rails::ErrorSubscriber.new) end - - def register_retry_stopped_subscriber - Sentry::Rails::ActiveJobExtensions::SentryReporter.register_retry_stopped_subscriber - end end end diff --git a/sentry-rails/spec/sentry/rails/activejob_spec.rb b/sentry-rails/spec/sentry/rails/activejob_spec.rb index 551a0be27..85b58df9b 100644 --- a/sentry-rails/spec/sentry/rails/activejob_spec.rb +++ b/sentry-rails/spec/sentry/rails/activejob_spec.rb @@ -446,9 +446,11 @@ def perform(event, hint) allow(Sentry::Rails::ActiveJobExtensions::SentryReporter) .to receive(:capture_exception).and_call_original - assert_performed_jobs 3 do - FailedJobWithRetryOn.perform_later rescue nil - end + FailedJobWithRetryOn.perform_later rescue nil + + perform_enqueued_jobs + perform_enqueued_jobs + perform_enqueued_jobs rescue nil expect(Sentry::Rails::ActiveJobExtensions::SentryReporter) .to have_received(:capture_exception) @@ -469,9 +471,11 @@ def perform(event, hint) allow(Sentry::Rails::ActiveJobExtensions::SentryReporter) .to receive(:capture_exception).and_call_original - assert_performed_jobs 3 do - FailedJobWithRetryOn.perform_later rescue nil - end + FailedJobWithRetryOn.perform_later rescue nil + + perform_enqueued_jobs + perform_enqueued_jobs + perform_enqueued_jobs rescue nil expect(Sentry::Rails::ActiveJobExtensions::SentryReporter) .to have_received(:capture_exception) diff --git a/sentry-rails/spec/spec_helper.rb b/sentry-rails/spec/spec_helper.rb index b64dd2c19..e3accdf98 100644 --- a/sentry-rails/spec/spec_helper.rb +++ b/sentry-rails/spec/spec_helper.rb @@ -55,7 +55,7 @@ Sentry::Rails::Tracing.remove_active_support_notifications_patch if defined?(Sentry::Rails::ActiveJobExtensions) - Sentry::Rails::ActiveJobExtensions::SentryReporter.detach_retry_stopped_subscriber + Sentry::Rails::ActiveJobExtensions::SentryReporter.detach_event_handlers end reset_sentry_globals!