diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f735c7be..d2394b01d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,9 @@ +## Unreleased + +### Features + +- 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 ### Features diff --git a/sentry-rails/lib/sentry/rails/active_job.rb b/sentry-rails/lib/sentry/rails/active_job.rb index fde7153eb..8fa1f8716 100644 --- a/sentry-rails/lib/sentry/rails/active_job.rb +++ b/sentry-rails/lib/sentry/rails/active_job.rb @@ -20,6 +20,7 @@ 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" class << self def record(job, &block) @@ -46,19 +47,51 @@ def record(job, &block) rescue Exception => e # rubocop:disable Lint/RescueException finish_sentry_transaction(transaction, 500) - Sentry::Rails.capture_exception( - e, - extra: sentry_context(job), - tags: { - job_id: job.job_id, - provider_job_id: job.provider_job_id - } - ) + unless Sentry.configuration.rails.active_job_report_after_job_retries + capture_exception(job, e) + end + raise end end end + def capture_exception(job, e) + Sentry::Rails.capture_exception( + e, + extra: sentry_context(job), + tags: { + job_id: job.job_id, + provider_job_id: job.provider_job_id + } + ) + end + + def register_retry_stopped_subscriber + unless @retry_stopped_subscriber + @retry_stopped_subscriber = ActiveSupport::Notifications.subscribe(NOTIFICATION_NAME) do |*args| + retry_stopped_handler(*args) + end + end + end + + def detach_retry_stopped_subscriber + if @retry_stopped_subscriber + ActiveSupport::Notifications.unsubscribe(@retry_stopped_subscriber) + @retry_stopped_subscriber = nil + end + end + + def retry_stopped_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 unless Sentry.configuration.rails.active_job_report_after_job_retries + capture_exception(job, error) + end + def finish_sentry_transaction(transaction, status) return unless transaction diff --git a/sentry-rails/lib/sentry/rails/configuration.rb b/sentry-rails/lib/sentry/rails/configuration.rb index a28a07f69..1b2fa93db 100644 --- a/sentry-rails/lib/sentry/rails/configuration.rb +++ b/sentry-rails/lib/sentry/rails/configuration.rb @@ -156,6 +156,10 @@ class Configuration # @return [Hash>] 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 + def initialize @register_error_subscriber = false @report_rescued_exceptions = true @@ -172,6 +176,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 end end end diff --git a/sentry-rails/lib/sentry/rails/railtie.rb b/sentry-rails/lib/sentry/rails/railtie.rb index 0a0cdf213..2025c7c88 100644 --- a/sentry-rails/lib/sentry/rails/railtie.rb +++ b/sentry-rails/lib/sentry/rails/railtie.rb @@ -51,6 +51,9 @@ class Railtie < ::Rails::Railtie activate_tracing 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) end runner do @@ -137,5 +140,9 @@ 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 0758cc7d3..e9d65b31f 100644 --- a/sentry-rails/spec/sentry/rails/activejob_spec.rb +++ b/sentry-rails/spec/sentry/rails/activejob_spec.rb @@ -67,6 +67,11 @@ class FailedJobWithCron < FailedJob 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 RSpec.describe "without Sentry initialized", type: :job do it "runs job" do @@ -361,7 +366,6 @@ def perform(event, hint) it "does not trigger sentry and re-raises" do expect { FailedJob.perform_now }.to raise_error(FailedJob::TestError) - expect(transport.events.size).to eq(0) end end @@ -429,4 +433,44 @@ def perform(event, hint) end end end + + describe "active_job_report_after_job_retries", skip: RAILS_VERSION < 7.0 do + context "when active_job_report_after_job_retries is false" do + it "reports 3 exceptions" do + allow(Sentry::Rails::ActiveJobExtensions::SentryReporter) + .to receive(:capture_exception).and_call_original + + assert_performed_jobs 3 do + FailedJobWithRetryOn.perform_later rescue nil + end + + expect(Sentry::Rails::ActiveJobExtensions::SentryReporter) + .to have_received(:capture_exception) + .exactly(3).times + 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 + allow(Sentry::Rails::ActiveJobExtensions::SentryReporter) + .to receive(:capture_exception).and_call_original + + assert_performed_jobs 3 do + FailedJobWithRetryOn.perform_later rescue nil + end + + expect(Sentry::Rails::ActiveJobExtensions::SentryReporter) + .to have_received(:capture_exception) + .exactly(1).times + end + end + end end diff --git a/sentry-rails/spec/sentry/rails/configuration_spec.rb b/sentry-rails/spec/sentry/rails/configuration_spec.rb index 37c5a41d6..b7961f894 100644 --- a/sentry-rails/spec/sentry/rails/configuration_spec.rb +++ b/sentry-rails/spec/sentry/rails/configuration_spec.rb @@ -59,4 +59,10 @@ class MySubscriber; end expect(subject.active_support_logger_subscription_items["foo"]).to include(:bar) end end + + describe "#active_job_report_after_job_retries" do + it "has correct default value" do + expect(subject.active_job_report_after_job_retries).to eq(false) + end + end end diff --git a/sentry-rails/spec/spec_helper.rb b/sentry-rails/spec/spec_helper.rb index ee086813f..b64dd2c19 100644 --- a/sentry-rails/spec/spec_helper.rb +++ b/sentry-rails/spec/spec_helper.rb @@ -34,6 +34,8 @@ Dir["#{__dir__}/support/**/*.rb"].each { |file| require file } +RAILS_VERSION = Rails.version.to_f + RSpec.configure do |config| # Enable flags like --only-failures and --next-failure config.example_status_persistence_file_path = ".rspec_status" @@ -52,6 +54,10 @@ expect(Sentry::Rails::Tracing.subscribed_tracing_events).to be_empty Sentry::Rails::Tracing.remove_active_support_notifications_patch + if defined?(Sentry::Rails::ActiveJobExtensions) + Sentry::Rails::ActiveJobExtensions::SentryReporter.detach_retry_stopped_subscriber + end + reset_sentry_globals! end