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

Author: st0012

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)

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"
 

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

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 },

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

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

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

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