diff --git a/CHANGELOG.md b/CHANGELOG.md index aa44825a0..d7befc7d7 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 ([#2617](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 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 1b2fa93db..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 = false + @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 85b58df9b..8431bffff 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 @@ -109,7 +38,10 @@ class FailedJobWithRetryOn < FailedJob 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", @@ -134,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( [ { @@ -152,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", @@ -174,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", @@ -192,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 @@ -207,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") @@ -224,7 +170,8 @@ def post.to_global_id 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 }) @@ -242,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") @@ -277,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") @@ -434,14 +381,22 @@ def perform(event, hint) end end - describe "active_job_report_after_job_retries", skip: RAILS_VERSION < 7.0 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 - 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_on_retry_error = true + end + + after do + Sentry.configuration.rails.active_job_report_on_retry_error = false + end + it "reports 3 exceptions" do allow(Sentry::Rails::ActiveJobExtensions::SentryReporter) .to receive(:capture_exception).and_call_original @@ -458,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 b7961f894..d008af92f 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 eq(false) + expect(subject.active_job_report_on_retry_error).to be(false) end end end 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