Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[active_job] support exception reporting only after last retry failed #2573

Merged
merged 20 commits into from
Apr 4, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
### 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

Expand Down
49 changes: 41 additions & 8 deletions sentry-rails/lib/sentry/rails/active_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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

Expand Down
5 changes: 5 additions & 0 deletions sentry-rails/lib/sentry/rails/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,10 @@ 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

def initialize
@register_error_subscriber = false
@report_rescued_exceptions = true
Expand All @@ -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
Expand Down
7 changes: 7 additions & 0 deletions sentry-rails/lib/sentry/rails/railtie.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
46 changes: 45 additions & 1 deletion sentry-rails/spec/sentry/rails/activejob_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
6 changes: 6 additions & 0 deletions sentry-rails/spec/sentry/rails/configuration_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
6 changes: 6 additions & 0 deletions sentry-rails/spec/spec_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think with our current setup this will always be true?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@st0012 not really, it may or may not be defined, depending on the order of loading files which is pretty much random 🙃

Sentry::Rails::ActiveJobExtensions::SentryReporter.detach_retry_stopped_subscriber
end

reset_sentry_globals!
end

Expand Down
Loading