From 328ec93aa27e28d1d32a82482cc9f28b5b05050c Mon Sep 17 00:00:00 2001 From: Peter Solnica Date: Fri, 2 May 2025 19:23:47 +0000 Subject: [PATCH 1/7] Set active_job_report_after_job_retries to true by default Refs #2598 Refs #2597 --- sentry-rails/lib/sentry/rails/configuration.rb | 2 +- .../spec/sentry/rails/activejob_spec.rb | 18 ++++++++++++++++-- .../spec/sentry/rails/configuration_spec.rb | 2 +- 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/sentry-rails/lib/sentry/rails/configuration.rb b/sentry-rails/lib/sentry/rails/configuration.rb index 1b2fa93db..2b9bff571 100644 --- a/sentry-rails/lib/sentry/rails/configuration.rb +++ b/sentry-rails/lib/sentry/rails/configuration.rb @@ -176,7 +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 + @active_job_report_after_job_retries = true end end end diff --git a/sentry-rails/spec/sentry/rails/activejob_spec.rb b/sentry-rails/spec/sentry/rails/activejob_spec.rb index 85b58df9b..9dd4de5b9 100644 --- a/sentry-rails/spec/sentry/rails/activejob_spec.rb +++ b/sentry-rails/spec/sentry/rails/activejob_spec.rb @@ -85,7 +85,9 @@ class FailedJobWithRetryOn < FailedJob RSpec.describe "ActiveJob integration", type: :job do before do - make_basic_app + make_basic_app do |config| + config.rails.active_job_report_after_job_retries = false + end end let(:event) do @@ -218,6 +220,7 @@ def post.to_global_id before do make_basic_app do |config| config.traces_sample_rate = 1.0 + config.rails.active_job_report_after_job_retries = false end end @@ -261,7 +264,9 @@ def post.to_global_id context "when DeserializationError happens in user's jobs" do before do - make_basic_app + make_basic_app do |config| + config.rails.active_job_report_after_job_retries = false + end end class DeserializationErrorJob < ActiveJob::Base @@ -441,7 +446,16 @@ def perform(event, hint) end end + context "when active_job_report_after_job_retries is false" do + before do + Sentry.configuration.rails.active_job_report_after_job_retries = false + end + + after do + Sentry.configuration.rails.active_job_report_after_job_retries = true + end + it "reports 3 exceptions" do allow(Sentry::Rails::ActiveJobExtensions::SentryReporter) .to receive(:capture_exception).and_call_original diff --git a/sentry-rails/spec/sentry/rails/configuration_spec.rb b/sentry-rails/spec/sentry/rails/configuration_spec.rb index b7961f894..fbee96120 100644 --- a/sentry-rails/spec/sentry/rails/configuration_spec.rb +++ b/sentry-rails/spec/sentry/rails/configuration_spec.rb @@ -62,7 +62,7 @@ class MySubscriber; 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) + expect(subject.active_job_report_after_job_retries).to be(true) end end end From 16e761d01caa6eacace1f40118cede0c8695bbb5 Mon Sep 17 00:00:00 2001 From: Peter Solnica Date: Mon, 5 May 2025 09:56:59 +0000 Subject: [PATCH 2/7] Use test jobs defined in support file --- .../spec/sentry/rails/activejob_spec.rb | 73 +------------------ sentry-rails/spec/support/test_jobs.rb | 6 ++ 2 files changed, 7 insertions(+), 72 deletions(-) diff --git a/sentry-rails/spec/sentry/rails/activejob_spec.rb b/sentry-rails/spec/sentry/rails/activejob_spec.rb index 9dd4de5b9..08a0ef88b 100644 --- a/sentry-rails/spec/sentry/rails/activejob_spec.rb +++ b/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 diff --git a/sentry-rails/spec/support/test_jobs.rb b/sentry-rails/spec/support/test_jobs.rb index 8f2957c70..4923d9a49 100644 --- a/sentry-rails/spec/support/test_jobs.rb +++ b/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 From 556301da6e7f6977db17ee7345b0155cf867d8da Mon Sep 17 00:00:00 2001 From: Peter Solnica Date: Mon, 5 May 2025 11:11:30 +0000 Subject: [PATCH 3/7] Introduce active_job_report_on_retry_error --- sentry-rails/lib/sentry/rails/active_job.rb | 28 ++-------- .../lib/sentry/rails/configuration.rb | 7 ++- .../spec/sentry/rails/activejob_spec.rb | 54 ++++++++++--------- .../spec/sentry/rails/configuration_spec.rb | 4 +- 4 files changed, 38 insertions(+), 55 deletions(-) diff --git a/sentry-rails/lib/sentry/rails/active_job.rb b/sentry-rails/lib/sentry/rails/active_job.rb index 9b5bc7176..2f5a134d0 100644 --- a/sentry-rails/lib/sentry/rails/active_job.rb +++ b/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 diff --git a/sentry-rails/lib/sentry/rails/configuration.rb b/sentry-rails/lib/sentry/rails/configuration.rb index 2b9bff571..a82a11359 100644 --- a/sentry-rails/lib/sentry/rails/configuration.rb +++ b/sentry-rails/lib/sentry/rails/configuration.rb @@ -156,9 +156,8 @@ 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 + # 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 = true + @active_job_report_on_retry_error = false end end end diff --git a/sentry-rails/spec/sentry/rails/activejob_spec.rb b/sentry-rails/spec/sentry/rails/activejob_spec.rb index 08a0ef88b..28334f674 100644 --- a/sentry-rails/spec/sentry/rails/activejob_spec.rb +++ b/sentry-rails/spec/sentry/rails/activejob_spec.rb @@ -14,9 +14,7 @@ RSpec.describe "ActiveJob integration", type: :job do before do - make_basic_app do |config| - config.rails.active_job_report_after_job_retries = false - end + make_basic_app end let(:event) do @@ -40,7 +38,10 @@ 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", @@ -65,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( [ { @@ -83,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", @@ -105,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", @@ -123,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 @@ -138,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") @@ -149,14 +164,14 @@ def post.to_global_id before do make_basic_app do |config| config.traces_sample_rate = 1.0 - config.rails.active_job_report_after_job_retries = false end end 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 }) @@ -174,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") @@ -193,9 +208,7 @@ def post.to_global_id context "when DeserializationError happens in user's jobs" do before do - make_basic_app do |config| - config.rails.active_job_report_after_job_retries = false - end + make_basic_app end class DeserializationErrorJob < ActiveJob::Base @@ -211,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") @@ -368,21 +381,20 @@ def perform(event, hint) end end - describe "active_job_report_after_job_retries", skip: RAILS_VERSION < 7.0 do + describe "Reporting on retry errors" do before do if defined?(JRUBY_VERSION) && JRUBY_VERSION == "9.4.12.0" && RAILS_VERSION <= 7.1 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_after_job_retries = false + Sentry.configuration.rails.active_job_report_on_retry_error = true end after do - Sentry.configuration.rails.active_job_report_after_job_retries = true + Sentry.configuration.rails.active_job_report_on_retry_error = false end it "reports 3 exceptions" do @@ -401,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 diff --git a/sentry-rails/spec/sentry/rails/configuration_spec.rb b/sentry-rails/spec/sentry/rails/configuration_spec.rb index fbee96120..d148f4f08 100644 --- a/sentry-rails/spec/sentry/rails/configuration_spec.rb +++ b/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 be(true) + expect(subject.active_job_report_on_retry_error).to be(true) end end end From 4060dc142d19baa8d53f6df86a04e28ee97cbb8e Mon Sep 17 00:00:00 2001 From: Peter Solnica Date: Mon, 5 May 2025 11:17:17 +0000 Subject: [PATCH 4/7] Fix spec --- sentry-rails/spec/sentry/rails/configuration_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sentry-rails/spec/sentry/rails/configuration_spec.rb b/sentry-rails/spec/sentry/rails/configuration_spec.rb index d148f4f08..d008af92f 100644 --- a/sentry-rails/spec/sentry/rails/configuration_spec.rb +++ b/sentry-rails/spec/sentry/rails/configuration_spec.rb @@ -62,7 +62,7 @@ class MySubscriber; end describe "#active_job_report_on_retry_error" do it "has correct default value" do - expect(subject.active_job_report_on_retry_error).to be(true) + expect(subject.active_job_report_on_retry_error).to be(false) end end end From 4ee05747bfb289d45176cf6b216503d6ed9e12a8 Mon Sep 17 00:00:00 2001 From: Peter Solnica Date: Mon, 5 May 2025 11:37:43 +0000 Subject: [PATCH 5/7] Adjust spec skipping logic --- sentry-rails/spec/sentry/rails/activejob_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sentry-rails/spec/sentry/rails/activejob_spec.rb b/sentry-rails/spec/sentry/rails/activejob_spec.rb index 28334f674..8431bffff 100644 --- a/sentry-rails/spec/sentry/rails/activejob_spec.rb +++ b/sentry-rails/spec/sentry/rails/activejob_spec.rb @@ -381,9 +381,9 @@ def perform(event, hint) end end - describe "Reporting on retry errors" 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 From 7ab289b90be7833c2cded283fbc9518372841236 Mon Sep 17 00:00:00 2001 From: Peter Solnica Date: Mon, 5 May 2025 11:39:31 +0000 Subject: [PATCH 6/7] Update CHANGELOG.md --- CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index aa44825a0..b7bd4f55d 100644 --- a/CHANGELOG.md +++ b/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 ([#2500](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 From cf2e4a0c5f2d2037e87f5151077c90bb00d1977c Mon Sep 17 00:00:00 2001 From: Peter Solnica Date: Mon, 5 May 2025 11:40:40 +0000 Subject: [PATCH 7/7] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b7bd4f55d..d7befc7d7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,7 +14,7 @@ ``` - 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/2617)) -- New configuration option called `active_job_report_on_retry_error` which enables reporting errors on each retry error ([#2500](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