From d5fbf68d2d85d88640070607c7086325892c04a2 Mon Sep 17 00:00:00 2001 From: Shayna Oriold Date: Thu, 5 Sep 2024 10:31:11 -0500 Subject: [PATCH 1/3] fix max_retries method --- .gitignore | 1 + lib/sidekiq/instrument/mixin.rb | 19 +++- lib/sidekiq/instrument/version.rb | 2 +- .../server_middleware_spec.rb | 88 ++++++++++++++----- 4 files changed, 81 insertions(+), 29 deletions(-) diff --git a/.gitignore b/.gitignore index 0cb6eeb..616c060 100644 --- a/.gitignore +++ b/.gitignore @@ -7,3 +7,4 @@ /pkg/ /spec/reports/ /tmp/ +sidekiq-instrument-* \ No newline at end of file diff --git a/lib/sidekiq/instrument/mixin.rb b/lib/sidekiq/instrument/mixin.rb index 4305054..189839a 100644 --- a/lib/sidekiq/instrument/mixin.rb +++ b/lib/sidekiq/instrument/mixin.rb @@ -13,10 +13,17 @@ def worker_dog_options(worker) end def max_retries(worker) - retries = worker.class.get_sidekiq_options['retry'] || Sidekiq[:max_retries] - return Sidekiq[:max_retries] if retries.to_s.eql?("true") - return 0 if retries.eql?("false") - retries + retries = fetch_worker_retry(worker) + case retries.to_s + when "true" + Sidekiq[:max_retries] + when "false" + 0 + when "" + Sidekiq[:max_retries] + else + retries + end end private @@ -29,6 +36,10 @@ def class_name(worker) worker.class.name.gsub('::', '_') end + def fetch_worker_retry(worker) + worker.class.get_sidekiq_options['retry'] + end + def underscore(string) string.gsub(/::/, '/'). gsub(/([A-Z]+)([A-Z][a-z])/,'\1_\2'). diff --git a/lib/sidekiq/instrument/version.rb b/lib/sidekiq/instrument/version.rb index 818a3ba..9c7fa30 100644 --- a/lib/sidekiq/instrument/version.rb +++ b/lib/sidekiq/instrument/version.rb @@ -1,5 +1,5 @@ module Sidekiq module Instrument - VERSION = '0.7.0' + VERSION = '0.7.1' end end diff --git a/spec/sidekiq-instrument/server_middleware_spec.rb b/spec/sidekiq-instrument/server_middleware_spec.rb index 719d3f5..8e04d76 100644 --- a/spec/sidekiq-instrument/server_middleware_spec.rb +++ b/spec/sidekiq-instrument/server_middleware_spec.rb @@ -28,7 +28,6 @@ context 'when an initial job succeeds' do before do Sidekiq[:max_retries] = 0 - allow_any_instance_of(MyWorker).to receive(:get_sidekiq_options).and_return({ 'retry': 'true'}) end it 'increments StatsD dequeue counter' do @@ -85,7 +84,6 @@ before do Sidekiq[:max_retries] = 1 allow_any_instance_of(MyWorker).to receive(:perform).and_raise('foo') - allow_any_instance_of(MyWorker).to receive(:get_sidekiq_options).and_return({ 'retry': 'true'}) # This makes the job look like a retry since we can't access the job argument allow_any_instance_of(Sidekiq::Instrument::ServerMiddleware).to receive(:current_retries).and_return(0) @@ -114,7 +112,6 @@ before do Sidekiq[:max_retries] = 0 allow_any_instance_of(MyWorker).to receive(:perform).and_raise('foo') - allow_any_instance_of(MyWorker).to receive(:get_sidekiq_options).and_return({ 'retry': 'false'}) end it 'increments the StatsD error counter' do @@ -141,36 +138,79 @@ end end - context 'job has retries left' do - before do - Sidekiq[:max_retries] = 1 - allow_any_instance_of(MyWorker).to receive(:get_sidekiq_options).and_return({ 'retry': 'true'}) + context 'the worker has retries disabled' do + shared_examples 'it does not attempt to track retries' do |retry_value| + before do + Sidekiq[:max_retries] = 1 + allow(MyWorker).to receive(:get_sidekiq_options).and_return({ "retry" => retry_value, "queue" => 'default' }) + end + + it 'does not increments the DogStatsD enqueue.retry counter' do + expect( + Sidekiq::Instrument::Statter.dogstatsd + ).to receive(:increment).with('sidekiq.dequeue', expected_dog_options).once + expect( + Sidekiq::Instrument::Statter.dogstatsd + ).not_to receive(:increment).with('sidekiq.enqueue.retry', expected_dog_options) + expect(Sidekiq::Instrument::Statter.dogstatsd).not_to receive(:time) + expect( + Sidekiq::Instrument::Statter.dogstatsd + ).to receive(:increment).with('sidekiq.error', expected_dog_options).once + + begin + MyWorker.perform_async + rescue StandardError + nil + end + end end - it 'increments the DogStatsD enqueue.retry counter' do - expect( - Sidekiq::Instrument::Statter.dogstatsd - ).to receive(:increment).with('sidekiq.dequeue', expected_dog_options).once - expect( - Sidekiq::Instrument::Statter.dogstatsd - ).to receive(:increment).with('sidekiq.enqueue.retry', expected_dog_options).once - expect(Sidekiq::Instrument::Statter.dogstatsd).not_to receive(:time) - expect( - Sidekiq::Instrument::Statter.dogstatsd - ).to receive(:increment).with('sidekiq.error', expected_dog_options).once + it_behaves_like 'it does not attempt to track retries', false - begin - MyWorker.perform_async - rescue StandardError - nil + it_behaves_like 'it does not attempt to track retries', 'false' + + it_behaves_like 'it does not attempt to track retries', 0 + end + + context 'the current job has retries left to attempt' do + shared_examples 'it tracks the retries with DogStatsD' do |retry_value| + before do + Sidekiq[:max_retries] = 2 + allow(MyWorker).to receive(:get_sidekiq_options).and_return({ "retry" => retry_value, "queue" => 'default' }) + end + + it 'does not increments the DogStatsD enqueue.retry counter' do + expect( + Sidekiq::Instrument::Statter.dogstatsd + ).to receive(:increment).with('sidekiq.dequeue', expected_dog_options).once + expect( + Sidekiq::Instrument::Statter.dogstatsd + ).to receive(:increment).with('sidekiq.enqueue.retry', expected_dog_options).once + expect(Sidekiq::Instrument::Statter.dogstatsd).not_to receive(:time) + expect( + Sidekiq::Instrument::Statter.dogstatsd + ).to receive(:increment).with('sidekiq.error', expected_dog_options).once + + begin + MyWorker.perform_async + rescue StandardError + nil + end end end + + it_behaves_like 'it tracks the retries with DogStatsD', 'true' + + it_behaves_like 'it tracks the retries with DogStatsD', true + + it_behaves_like 'it tracks the retries with DogStatsD', 5 + + it_behaves_like 'it tracks the retries with DogStatsD', nil end - context 'on its last retry' do + context 'the job is on its last retry attempt' do before do Sidekiq[:max_retries] = 1 - allow_any_instance_of(MyWorker).to receive(:get_sidekiq_options).and_return({ 'retry': 'true'}) # This makes the job look like a retry since we can't access the job argument allow_any_instance_of(Sidekiq::Instrument::ServerMiddleware).to receive(:current_retries).and_return(1) From a8029e2c2a45183f0f1b132be4ede58e70aaab35 Mon Sep 17 00:00:00 2001 From: Shayna Oriold Date: Thu, 5 Sep 2024 11:02:38 -0500 Subject: [PATCH 2/3] cleanup case statement --- lib/sidekiq/instrument/mixin.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/sidekiq/instrument/mixin.rb b/lib/sidekiq/instrument/mixin.rb index 189839a..40d0518 100644 --- a/lib/sidekiq/instrument/mixin.rb +++ b/lib/sidekiq/instrument/mixin.rb @@ -15,12 +15,10 @@ def worker_dog_options(worker) def max_retries(worker) retries = fetch_worker_retry(worker) case retries.to_s - when "true" + when "true", "" Sidekiq[:max_retries] when "false" 0 - when "" - Sidekiq[:max_retries] else retries end From 183da69dd7367388d049d2cdfae693e0c34e9a4f Mon Sep 17 00:00:00 2001 From: Shayna Oriold Date: Thu, 5 Sep 2024 11:21:01 -0500 Subject: [PATCH 3/3] use rspec best practices --- spec/sidekiq-instrument/server_middleware_spec.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/spec/sidekiq-instrument/server_middleware_spec.rb b/spec/sidekiq-instrument/server_middleware_spec.rb index 8e04d76..ae64eb1 100644 --- a/spec/sidekiq-instrument/server_middleware_spec.rb +++ b/spec/sidekiq-instrument/server_middleware_spec.rb @@ -138,14 +138,14 @@ end end - context 'the worker has retries disabled' do + context 'when the worker has retries disabled' do shared_examples 'it does not attempt to track retries' do |retry_value| before do Sidekiq[:max_retries] = 1 allow(MyWorker).to receive(:get_sidekiq_options).and_return({ "retry" => retry_value, "queue" => 'default' }) end - it 'does not increments the DogStatsD enqueue.retry counter' do + it 'does not increment the DogStatsD enqueue.retry counter' do expect( Sidekiq::Instrument::Statter.dogstatsd ).to receive(:increment).with('sidekiq.dequeue', expected_dog_options).once @@ -172,14 +172,14 @@ it_behaves_like 'it does not attempt to track retries', 0 end - context 'the current job has retries left to attempt' do + context 'when the current job has retries left to attempt' do shared_examples 'it tracks the retries with DogStatsD' do |retry_value| before do Sidekiq[:max_retries] = 2 allow(MyWorker).to receive(:get_sidekiq_options).and_return({ "retry" => retry_value, "queue" => 'default' }) end - it 'does not increments the DogStatsD enqueue.retry counter' do + it 'increments the DogStatsD enqueue.retry counter' do expect( Sidekiq::Instrument::Statter.dogstatsd ).to receive(:increment).with('sidekiq.dequeue', expected_dog_options).once @@ -208,7 +208,7 @@ it_behaves_like 'it tracks the retries with DogStatsD', nil end - context 'the job is on its last retry attempt' do + context 'when the job is on its last retry attempt' do before do Sidekiq[:max_retries] = 1