From 952e9c9a835a7b999c7abb793d9a4e2fb19939ad Mon Sep 17 00:00:00 2001 From: Stan Lo Date: Mon, 16 Aug 2021 11:50:14 +0800 Subject: [PATCH] Support after-retry reporting to sentry-sidekiq (#1532) * Improve test setup * Rename process_job to execute_worker * Add config.sidekiq.report_after_job_retries When set to true, `sentry-sidekiq` only reports exceptions when the job has been fully retried. * Update changelog --- CHANGELOG.md | 4 ++ sentry-sidekiq/lib/sentry-sidekiq.rb | 1 + .../lib/sentry/sidekiq/configuration.rb | 21 ++++++ .../lib/sentry/sidekiq/error_handler.rb | 9 +++ .../spec/sentry/sidekiq/configuration_spec.rb | 15 +++++ .../sidekiq/sentry_context_middleware_spec.rb | 6 +- sentry-sidekiq/spec/sentry/sidekiq_spec.rb | 66 +++++++++++++++---- sentry-sidekiq/spec/spec_helper.rb | 31 +++++++-- 8 files changed, 132 insertions(+), 21 deletions(-) create mode 100644 sentry-sidekiq/lib/sentry/sidekiq/configuration.rb create mode 100644 sentry-sidekiq/spec/sentry/sidekiq/configuration_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 895ee8580..d2849a917 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +## Unreleased + +- Support after-retry reporting to `sentry-sidekiq` [#1532](https://github.com/getsentry/sentry-ruby/pull/1532) + ## 4.6.5 - SDK should drop the event when any event processor returns nil [#1523](https://github.com/getsentry/sentry-ruby/pull/1523) diff --git a/sentry-sidekiq/lib/sentry-sidekiq.rb b/sentry-sidekiq/lib/sentry-sidekiq.rb index 5b67ea8dc..4f80df52d 100644 --- a/sentry-sidekiq/lib/sentry-sidekiq.rb +++ b/sentry-sidekiq/lib/sentry-sidekiq.rb @@ -2,6 +2,7 @@ require "sentry-ruby" require "sentry/integrable" require "sentry/sidekiq/version" +require "sentry/sidekiq/configuration" require "sentry/sidekiq/error_handler" require "sentry/sidekiq/sentry_context_middleware" diff --git a/sentry-sidekiq/lib/sentry/sidekiq/configuration.rb b/sentry-sidekiq/lib/sentry/sidekiq/configuration.rb new file mode 100644 index 000000000..c346371a1 --- /dev/null +++ b/sentry-sidekiq/lib/sentry/sidekiq/configuration.rb @@ -0,0 +1,21 @@ +module Sentry + class Configuration + attr_reader :sidekiq + + add_post_initialization_callback do + @sidekiq = Sentry::Sidekiq::Configuration.new + end + end + + module Sidekiq + class Configuration + # Set this option to true if you want Sentry to only capture the last job + # retry if it fails. + attr_accessor :report_after_job_retries + + def initialize + @report_after_job_retries = false + end + end + end +end diff --git a/sentry-sidekiq/lib/sentry/sidekiq/error_handler.rb b/sentry-sidekiq/lib/sentry/sidekiq/error_handler.rb index da0393a39..00f362c97 100644 --- a/sentry-sidekiq/lib/sentry/sidekiq/error_handler.rb +++ b/sentry-sidekiq/lib/sentry/sidekiq/error_handler.rb @@ -11,6 +11,15 @@ def call(ex, context) scope = Sentry.get_current_scope scope.set_transaction_name(context_filter.transaction_name) unless scope.transaction_name + retry_option = context.dig(:job, "retry") + + if Sentry.configuration.sidekiq.report_after_job_retries && retry_option.is_a?(Integer) + retry_count = context.dig(:job, "retry_count") + if retry_count.nil? || retry_count < retry_option - 1 + return + end + end + Sentry::Sidekiq.capture_exception( ex, contexts: { sidekiq: context_filter.filtered }, diff --git a/sentry-sidekiq/spec/sentry/sidekiq/configuration_spec.rb b/sentry-sidekiq/spec/sentry/sidekiq/configuration_spec.rb new file mode 100644 index 000000000..b2ef53f13 --- /dev/null +++ b/sentry-sidekiq/spec/sentry/sidekiq/configuration_spec.rb @@ -0,0 +1,15 @@ +require "spec_helper" + +RSpec.describe Sentry::Sidekiq::Configuration do + it "adds #delayed_job option to Sentry::Configuration" do + config = Sentry::Configuration.new + + expect(config.sidekiq).to be_a(described_class) + end + + describe "#report_after_job_retries" do + it "has correct default value" do + expect(subject.report_after_job_retries).to eq(false) + end + end +end diff --git a/sentry-sidekiq/spec/sentry/sidekiq/sentry_context_middleware_spec.rb b/sentry-sidekiq/spec/sentry/sidekiq/sentry_context_middleware_spec.rb index 7cebdf8ea..5c2cdbc88 100644 --- a/sentry-sidekiq/spec/sentry/sidekiq/sentry_context_middleware_spec.rb +++ b/sentry-sidekiq/spec/sentry/sidekiq/sentry_context_middleware_spec.rb @@ -20,7 +20,7 @@ perform_basic_setup { |config| config.traces_sample_rate = 0 } Sentry.set_user(user) - process_job(processor, "SadWorker") + execute_worker(processor, SadWorker) expect(transport.events.count).to eq(1) event = transport.events.first @@ -34,7 +34,7 @@ end it "sets user to the transaction" do - process_job(processor, "HappyWorker") + execute_worker(processor, HappyWorker) expect(transport.events.count).to eq(1) transaction = transport.events.first @@ -43,7 +43,7 @@ end it "sets user to both the event and transaction" do - process_job(processor, "SadWorker") + execute_worker(processor, SadWorker) expect(transport.events.count).to eq(2) transaction = transport.events.first diff --git a/sentry-sidekiq/spec/sentry/sidekiq_spec.rb b/sentry-sidekiq/spec/sentry/sidekiq_spec.rb index b5a280c61..1387854f5 100644 --- a/sentry-sidekiq/spec/sentry/sidekiq_spec.rb +++ b/sentry-sidekiq/spec/sentry/sidekiq_spec.rb @@ -6,10 +6,24 @@ perform_basic_setup end + let(:queue) do + Sidekiq::Queue.new("default") + end + + let(:retry_set) do + Sidekiq::RetrySet.new + end + + before do + retry_set.clear + queue.clear + end + after do # those test jobs will go into the real Redis and be visiable to other sidekiq processes # this can affect local testing and development, so we should clear them after each test - Sidekiq::RetrySet.new.clear + retry_set.clear + queue.clear end let(:processor) do @@ -28,7 +42,7 @@ end it "captues exception raised in the worker" do - expect { process_job(processor, "SadWorker") }.to change { transport.events.size }.by(1) + expect { execute_worker(processor, SadWorker) }.to change { transport.events.size }.by(1) event = transport.events.last.to_hash expect(event[:sdk]).to eq({ name: "sentry.ruby.sidekiq", version: described_class::VERSION }) @@ -37,8 +51,8 @@ describe "context cleanup" do it "cleans up context from processed jobs" do - process_job(processor, "HappyWorker") - process_job(processor, "SadWorker") + execute_worker(processor, HappyWorker) + execute_worker(processor, SadWorker) expect(transport.events.count).to eq(1) event = transport.events.last.to_json_compatible @@ -49,8 +63,8 @@ end it "cleans up context from failed jobs" do - process_job(processor, "SadWorker") - process_job(processor, "VerySadWorker") + execute_worker(processor, SadWorker) + execute_worker(processor, VerySadWorker) expect(transport.events.count).to eq(2) event = transport.events.last.to_json_compatible @@ -61,19 +75,45 @@ end it "has some context when capturing, even if no exception raised" do - process_job(processor, "ReportingWorker") + execute_worker(processor, ReportingWorker) event = transport.events.last.to_json_compatible expect(event["message"]).to eq "I have something to say!" - expect(event["contexts"]["sidekiq"]).to eq("class" => "ReportingWorker", "jid" => "123123", "queue" => "default") + expect(event["contexts"]["sidekiq"]).to eq("args" => [], "class" => "ReportingWorker", "jid" => "123123", "queue" => "default") end it "adds the failed job to the retry queue" do - process_job(processor, "SadWorker") + execute_worker(processor, SadWorker) + + expect(retry_set.count).to eq(1) + end - retries = Sidekiq::RetrySet.new - expect(retries.count).to eq(1) + context "with config.report_after_job_retries = true" do + before do + Sentry.configuration.sidekiq.report_after_job_retries = true + end + + it "doesn't report the error until retries are exhuasted" do + execute_worker(processor, RetryWorker) + + expect(transport.events.count).to eq(0) + + expect(retry_set.count).to eq(1) + + retry_set.first.add_to_queue + job = queue.first + work = Sidekiq::BasicFetch::UnitOfWork.new('queue:default', job.value) + process_work(processor, work) + expect(transport.events.count).to eq(1) + end + + it "doesn't affect no-retry jobs" do + execute_worker(processor, SadWorker) + + expect(transport.events.count).to eq(1) + expect(retry_set.count).to eq(1) + end end context "when tracing is enabled" do @@ -84,7 +124,7 @@ end it "records transaction" do - process_job(processor, "HappyWorker") + execute_worker(processor, HappyWorker) expect(transport.events.count).to eq(1) transaction = transport.events.first @@ -96,7 +136,7 @@ end it "records transaction with exception" do - process_job(processor, "SadWorker") + execute_worker(processor, SadWorker) expect(transport.events.count).to eq(2) transaction = transport.events.first diff --git a/sentry-sidekiq/spec/spec_helper.rb b/sentry-sidekiq/spec/spec_helper.rb index dc4e20f45..f90f4a2b7 100644 --- a/sentry-sidekiq/spec/spec_helper.rb +++ b/sentry-sidekiq/spec/spec_helper.rb @@ -1,5 +1,6 @@ require "bundler/setup" require "pry" +require "debug" if RUBY_VERSION.to_f >= 2.6 # this enables sidekiq's server mode require "sidekiq/cli" @@ -132,12 +133,32 @@ def perform end end -def process_job(processor, klass) - msg = Sidekiq.dump_json(jid: "123123", class: klass) - job = Sidekiq::BasicFetch::UnitOfWork.new('queue:default', msg) - processor.instance_variable_set(:'@job', job) +class RetryWorker + include Sidekiq::Worker + + sidekiq_options retry: 1 + + def perform + 1/0 + end +end + +def execute_worker(processor, klass) + klass_options = klass.sidekiq_options_hash || {} + options = {} + + # for Ruby < 2.6 + klass_options.each do |k, v| + options[k.to_sym] = v + end + + msg = Sidekiq.dump_json(jid: "123123", class: klass, args: [], **options) + work = Sidekiq::BasicFetch::UnitOfWork.new('queue:default', msg) + process_work(processor, work) +end - processor.send(:process, job) +def process_work(processor, work) + processor.send(:process, work) rescue StandardError # do nothing end