From b4bd321dd77469419a124396581cbfa24e6b7c60 Mon Sep 17 00:00:00 2001 From: Peter Solnica Date: Mon, 14 Apr 2025 12:29:54 +0000 Subject: [PATCH 1/8] [active_job] patch retry_job to report all errors during retries --- sentry-rails/lib/sentry/rails/active_job.rb | 7 +++++++ sentry-rails/spec/sentry/rails/activejob_spec.rb | 16 ++++++++++------ 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/sentry-rails/lib/sentry/rails/active_job.rb b/sentry-rails/lib/sentry/rails/active_job.rb index 8fa1f8716..700d5f191 100644 --- a/sentry-rails/lib/sentry/rails/active_job.rb +++ b/sentry-rails/lib/sentry/rails/active_job.rb @@ -17,6 +17,13 @@ def already_supported_by_sentry_integration? Sentry.configuration.rails.skippable_job_adapters.include?(self.class.queue_adapter.class.to_s) end + def retry_job(error:, **opts) + unless Sentry.configuration.rails.active_job_report_after_job_retries + SentryReporter.capture_exception(self, error) + end + super + end + class SentryReporter OP_NAME = "queue.active_job" SPAN_ORIGIN = "auto.queue.active_job" 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) From 00e34d1fb2187ed08b0d70ab3ba5cf5fc4dc4ee5 Mon Sep 17 00:00:00 2001 From: Peter Solnica Date: Mon, 14 Apr 2025 12:47:05 +0000 Subject: [PATCH 2/8] [active_job] dry-up capturing logic a bit --- sentry-rails/lib/sentry/rails/active_job.rb | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/sentry-rails/lib/sentry/rails/active_job.rb b/sentry-rails/lib/sentry/rails/active_job.rb index 700d5f191..4a04c2977 100644 --- a/sentry-rails/lib/sentry/rails/active_job.rb +++ b/sentry-rails/lib/sentry/rails/active_job.rb @@ -18,9 +18,7 @@ def already_supported_by_sentry_integration? end def retry_job(error:, **opts) - unless Sentry.configuration.rails.active_job_report_after_job_retries - SentryReporter.capture_exception(self, error) - end + SentryReporter.maybe_capture_exception(self, error) super end @@ -54,15 +52,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, From 5d1cb2051c8591cd69d6e18d5fc968baa514a295 Mon Sep 17 00:00:00 2001 From: Peter Solnica Date: Mon, 14 Apr 2025 12:55:43 +0000 Subject: [PATCH 3/8] Update CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) 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 From 5930c153517348f04af71c45f1439060b7ebc70e Mon Sep 17 00:00:00 2001 From: Peter Solnica Date: Wed, 30 Apr 2025 11:08:03 +0000 Subject: [PATCH 4/8] Rework to use event --- sentry-rails/lib/sentry/rails/active_job.rb | 24 ++++++++++++++++----- sentry-rails/lib/sentry/rails/railtie.rb | 7 ++++-- 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/sentry-rails/lib/sentry/rails/active_job.rb b/sentry-rails/lib/sentry/rails/active_job.rb index 4a04c2977..bf43649f3 100644 --- a/sentry-rails/lib/sentry/rails/active_job.rb +++ b/sentry-rails/lib/sentry/rails/active_job.rb @@ -17,11 +17,6 @@ def already_supported_by_sentry_integration? Sentry.configuration.rails.skippable_job_adapters.include?(self.class.queue_adapter.class.to_s) end - def retry_job(error:, **opts) - SentryReporter.maybe_capture_exception(self, error) - super - end - class SentryReporter OP_NAME = "queue.active_job" SPAN_ORIGIN = "auto.queue.active_job" @@ -76,6 +71,14 @@ def capture_exception(job, e) ) end + def register_retry_subscriber + unless @retry_subscriber + @retry_subscriber = ActiveSupport::Notifications.subscribe("enqueue_retry.active_job") do |*args| + retry_handler(*args) + end + end + end + def register_retry_stopped_subscriber unless @retry_stopped_subscriber @retry_stopped_subscriber = ActiveSupport::Notifications.subscribe(NOTIFICATION_NAME) do |*args| @@ -91,6 +94,17 @@ def detach_retry_stopped_subscriber end end + def retry_handler(*args) + event = ActiveSupport::Notifications::Event.new(*args) + job = event.payload[:job] + error = event.payload[: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 + def retry_stopped_handler(*args) event = ActiveSupport::Notifications::Event.new(*args) job = event.payload[:job] diff --git a/sentry-rails/lib/sentry/rails/railtie.rb b/sentry-rails/lib/sentry/rails/railtie.rb index 2025c7c88..a31ae12df 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) + register_active_job_subscribers + end end runner do @@ -141,7 +143,8 @@ def register_error_subscriber(app) app.executor.error_reporter.subscribe(Sentry::Rails::ErrorSubscriber.new) end - def register_retry_stopped_subscriber + def register_active_job_subscribers + Sentry::Rails::ActiveJobExtensions::SentryReporter.register_retry_subscriber Sentry::Rails::ActiveJobExtensions::SentryReporter.register_retry_stopped_subscriber end end From 3db0e7f4c40f346c4b4f154dcc9695bfcba23f59 Mon Sep 17 00:00:00 2001 From: Peter Solnica Date: Wed, 30 Apr 2025 11:12:10 +0000 Subject: [PATCH 5/8] Detach retry_handler in specs --- sentry-rails/lib/sentry/rails/active_job.rb | 7 ++++++- sentry-rails/spec/spec_helper.rb | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/sentry-rails/lib/sentry/rails/active_job.rb b/sentry-rails/lib/sentry/rails/active_job.rb index bf43649f3..0dcc2d930 100644 --- a/sentry-rails/lib/sentry/rails/active_job.rb +++ b/sentry-rails/lib/sentry/rails/active_job.rb @@ -87,11 +87,16 @@ def register_retry_stopped_subscriber end end - def detach_retry_stopped_subscriber + def detach_handlers if @retry_stopped_subscriber ActiveSupport::Notifications.unsubscribe(@retry_stopped_subscriber) @retry_stopped_subscriber = nil end + + if @retry_handler + ActiveSupport::Notifications.unsubscribe(@retry_handler) + @retry_handler = nil + end end def retry_handler(*args) diff --git a/sentry-rails/spec/spec_helper.rb b/sentry-rails/spec/spec_helper.rb index b64dd2c19..d416d0532 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_handlers end reset_sentry_globals! From 5ad370da7c75afb61c5a300043649c2d275114ff Mon Sep 17 00:00:00 2001 From: Peter Solnica Date: Wed, 30 Apr 2025 11:19:22 +0000 Subject: [PATCH 6/8] DRY up event handling --- sentry-rails/lib/sentry/rails/active_job.rb | 44 ++++++++++----------- sentry-rails/lib/sentry/rails/railtie.rb | 7 +--- sentry-rails/spec/spec_helper.rb | 2 +- 3 files changed, 24 insertions(+), 29 deletions(-) diff --git a/sentry-rails/lib/sentry/rails/active_job.rb b/sentry-rails/lib/sentry/rails/active_job.rb index 0dcc2d930..9b41cba78 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) @@ -71,32 +77,19 @@ def capture_exception(job, e) ) end - def register_retry_subscriber - unless @retry_subscriber - @retry_subscriber = ActiveSupport::Notifications.subscribe("enqueue_retry.active_job") do |*args| - retry_handler(*args) - end - end - 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_handlers - if @retry_stopped_subscriber - ActiveSupport::Notifications.unsubscribe(@retry_stopped_subscriber) - @retry_stopped_subscriber = nil - end - - if @retry_handler - ActiveSupport::Notifications.unsubscribe(@retry_handler) - @retry_handler = nil + def detach_event_handlers + subscribers.each do |subscriber| + ActiveSupport::Notifications.unsubscribe(subscriber) end + subscribers.clear end def retry_handler(*args) @@ -117,6 +110,7 @@ def retry_stopped_handler(*args) 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) end @@ -156,6 +150,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 a31ae12df..d2cf03d57 100644 --- a/sentry-rails/lib/sentry/rails/railtie.rb +++ b/sentry-rails/lib/sentry/rails/railtie.rb @@ -54,7 +54,7 @@ class Railtie < ::Rails::Railtie # Presence of ActiveJob is no longer a reliable cue if defined?(Sentry::Rails::ActiveJobExtensions) - register_active_job_subscribers + Sentry::Rails::ActiveJobExtensions::SentryReporter.register_event_handlers end end @@ -142,10 +142,5 @@ def register_error_subscriber(app) require "sentry/rails/error_subscriber" app.executor.error_reporter.subscribe(Sentry::Rails::ErrorSubscriber.new) end - - def register_active_job_subscribers - Sentry::Rails::ActiveJobExtensions::SentryReporter.register_retry_subscriber - Sentry::Rails::ActiveJobExtensions::SentryReporter.register_retry_stopped_subscriber - end end end diff --git a/sentry-rails/spec/spec_helper.rb b/sentry-rails/spec/spec_helper.rb index d416d0532..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_handlers + Sentry::Rails::ActiveJobExtensions::SentryReporter.detach_event_handlers end reset_sentry_globals! From b3c213bd64c0778c2eefd02703349cc6083e419e Mon Sep 17 00:00:00 2001 From: Peter Solnica Date: Wed, 30 Apr 2025 11:23:25 +0000 Subject: [PATCH 7/8] Add a common handler_error_event --- sentry-rails/lib/sentry/rails/active_job.rb | 27 +++++++++++---------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/sentry-rails/lib/sentry/rails/active_job.rb b/sentry-rails/lib/sentry/rails/active_job.rb index 9b41cba78..9be504a6f 100644 --- a/sentry-rails/lib/sentry/rails/active_job.rb +++ b/sentry-rails/lib/sentry/rails/active_job.rb @@ -93,25 +93,26 @@ def detach_event_handlers end def retry_handler(*args) - event = ActiveSupport::Notifications::Event.new(*args) - job = event.payload[:job] - error = event.payload[:error] - - return if !Sentry.initialized? || job.already_supported_by_sentry_integration? - return if Sentry.configuration.rails.active_job_report_after_job_retries + 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) + capture_exception(job, error) + end end 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) + end + end - capture_exception(job, error) + 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) From 2b6b7f1cc55b436a6763d0cd0e08ec74e239afd3 Mon Sep 17 00:00:00 2001 From: Peter Solnica Date: Wed, 30 Apr 2025 11:29:26 +0000 Subject: [PATCH 8/8] Add some comments to explain this tricky logic --- sentry-rails/lib/sentry/rails/active_job.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/sentry-rails/lib/sentry/rails/active_job.rb b/sentry-rails/lib/sentry/rails/active_job.rb index 9be504a6f..9b5bc7176 100644 --- a/sentry-rails/lib/sentry/rails/active_job.rb +++ b/sentry-rails/lib/sentry/rails/active_job.rb @@ -92,6 +92,8 @@ 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 def retry_handler(*args) handle_error_event(*args) do |job, error| return if !Sentry.initialized? || job.already_supported_by_sentry_integration? @@ -101,6 +103,9 @@ def retry_handler(*args) 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?