From 3d643fda796a376f0d0e5a364e8b976bbbb5efb7 Mon Sep 17 00:00:00 2001 From: Joel Oliveira Date: Thu, 2 Nov 2023 13:34:49 -0400 Subject: [PATCH] Fix: Accomodate apps that do not use Sidekiq Enterprise (#42) * Fix: Accomodate apps that do not use Sidekiq Enterprise Why? ==== Apps that do not use Sidekiq Enterprise will not have a `Sidekiq::Limiter` module defined. As a result, if an error is encountered while in the `#call` method of `SidekiqPrometheus::JobMetrics` then an additional problem will be encountered, causing a large backlog of errors re-enqueueing into Sidekiq. What? ===== 1. Wrap the specs in following proper contexts: * That Sidekiq Enterprise error class exists. And... * That error class does NOT exist. 2. Test that the correct behavior occurs in those two contexts. 3. Lastly, in the implementation code, make sure we ask that we check for presence of `Sidekiq::Limiter::OverLimit` * rake standard:fix --------- Co-authored-by: Lukas Eklund --- lib/sidekiq_prometheus/job_metrics.rb | 2 +- spec/sidekiq_prometheus/job_metrics_spec.rb | 147 ++++++++++++-------- 2 files changed, 92 insertions(+), 57 deletions(-) diff --git a/lib/sidekiq_prometheus/job_metrics.rb b/lib/sidekiq_prometheus/job_metrics.rb index 7b73d0b..2e2de56 100644 --- a/lib/sidekiq_prometheus/job_metrics.rb +++ b/lib/sidekiq_prometheus/job_metrics.rb @@ -30,7 +30,7 @@ def call(worker, job, queue) result rescue => e - if e.instance_of?(::Sidekiq::Limiter::OverLimit) + if defined?(::Sidekiq::Limiter::OverLimit) && e.instance_of?(::Sidekiq::Limiter::OverLimit) registry[:sidekiq_job_over_limit].increment(labels: labels) else err_label = {error_class: e.class.to_s} diff --git a/spec/sidekiq_prometheus/job_metrics_spec.rb b/spec/sidekiq_prometheus/job_metrics_spec.rb index bb4cc60..92af421 100644 --- a/spec/sidekiq_prometheus/job_metrics_spec.rb +++ b/spec/sidekiq_prometheus/job_metrics_spec.rb @@ -8,11 +8,6 @@ def prometheus_labels end end -module Sidekiq::Limiter - class OverLimit < StandardError - end -end - RSpec.describe SidekiqPrometheus::JobMetrics do let(:middleware) { described_class.new } let(:registry) { instance_double Prometheus::Client::Registry } @@ -28,81 +23,121 @@ class OverLimit < StandardError end describe "#call" do - it "records the expected metrics" do - SidekiqPrometheus.registry = registry - allow(registry).to receive(:get).and_return(metric) + context "when Sidekiq::Limiter module is not defined" do + before do + # Ensure that if this module _is_ defined (depending on random spec + # execution order) that we remove it so we can test the fallback + # behavior. + if defined?(Sidekiq::Limiter) + Sidekiq.send(:remove_const, :Limiter) + end + end - expect { |b| middleware.call(worker, job, queue, &b) }.to yield_control + it "handles other errors with the fallback sidekiq_job_failed metric" do + SidekiqPrometheus.registry = registry + allow(registry).to receive(:get).and_return(metric) - expect(registry).to have_received(:get).with(:sidekiq_job_count) - expect(registry).to have_received(:get).with(:sidekiq_job_duration) - expect(registry).to have_received(:get).with(:sidekiq_job_success) - expect(registry).to have_received(:get).with(:sidekiq_job_allocated_objects) + expect { middleware.call(worker, job, queue) { raise StandardError } }.to raise_error(StandardError) - expect(metric).to have_received(:increment).twice.with(labels: labels) - expect(metric).to have_received(:observe).twice.with(kind_of(Numeric), labels: labels) + expect(registry).not_to have_received(:get).with(:sidekiq_job_over_limit) + expect(registry).to have_received(:get).with(:sidekiq_job_count) + expect(registry).to have_received(:get).with(:sidekiq_job_failed) + + expect(registry).not_to have_received(:get).with(:sidekiq_job_duration) + expect(registry).not_to have_received(:get).with(:sidekiq_job_success) + + expect(metric).to have_received(:increment).once.with(labels: labels) + expect(metric).to have_received(:increment).once.with(labels: labels.merge(error_class: "StandardError")) + expect(metric).not_to have_received(:observe) + end end - it "returns the result from the yielded block" do - SidekiqPrometheus.registry = registry - allow(registry).to receive(:get).and_return(metric) - expected = "Zoot Boot" + context "when Sidekiq::Limiter::OverLimit class is defined" do + before do + unless defined?(Sidekiq::Limiter::OverLimit) + module Sidekiq::Limiter + class OverLimit < StandardError + end + end + end + end - result = middleware.call(worker, job, queue) { expected } + it "records the expected metrics" do + SidekiqPrometheus.registry = registry + allow(registry).to receive(:get).and_return(metric) - expect(result).to eq(expected) - end + expect { |b| middleware.call(worker, job, queue, &b) }.to yield_control - it "increments the sidekiq_job_failed metric on error and raises" do - SidekiqPrometheus.registry = registry - allow(registry).to receive(:get).and_return(metric) + expect(registry).to have_received(:get).with(:sidekiq_job_count) + expect(registry).to have_received(:get).with(:sidekiq_job_duration) + expect(registry).to have_received(:get).with(:sidekiq_job_success) + expect(registry).to have_received(:get).with(:sidekiq_job_allocated_objects) - expect { middleware.call(worker, job, queue) { raise "no way!" } }.to raise_error(StandardError) + expect(metric).to have_received(:increment).twice.with(labels: labels) + expect(metric).to have_received(:observe).twice.with(kind_of(Numeric), labels: labels) + end - expect(registry).to have_received(:get).with(:sidekiq_job_count) - expect(registry).to have_received(:get).with(:sidekiq_job_failed) - expect(registry).not_to have_received(:get).with(:sidekiq_job_duration) - expect(registry).not_to have_received(:get).with(:sidekiq_job_success) + it "returns the result from the yielded block" do + SidekiqPrometheus.registry = registry + allow(registry).to receive(:get).and_return(metric) + expected = "Zoot Boot" - expect(metric).to have_received(:increment).once.with(labels: failed_labels) - expect(metric).to have_received(:increment).once.with(labels: labels) - expect(metric).not_to have_received(:observe) - end + result = middleware.call(worker, job, queue) { expected } - it "handles sidekiq ent Sidekiq::Limiter::OverLimit errors independently of failures" do - SidekiqPrometheus.registry = registry - allow(registry).to receive(:get).and_return(metric) + expect(result).to eq(expected) + end - expect { middleware.call(worker, job, queue) { raise Sidekiq::Limiter::OverLimit } }.to raise_error(Sidekiq::Limiter::OverLimit) + it "increments the sidekiq_job_failed metric on error and raises" do + SidekiqPrometheus.registry = registry + allow(registry).to receive(:get).and_return(metric) - expect(registry).to have_received(:get).with(:sidekiq_job_count) - expect(registry).to have_received(:get).with(:sidekiq_job_over_limit) - expect(registry).not_to have_received(:get).with(:sidekiq_job_failed) - expect(registry).not_to have_received(:get).with(:sidekiq_job_duration) - expect(registry).not_to have_received(:get).with(:sidekiq_job_success) + expect { middleware.call(worker, job, queue) { raise "no way!" } }.to raise_error(StandardError) - expect(metric).to have_received(:increment).twice.with(labels: labels) - expect(metric).not_to have_received(:observe) - end + expect(registry).to have_received(:get).with(:sidekiq_job_count) + expect(registry).to have_received(:get).with(:sidekiq_job_failed) + expect(registry).not_to have_received(:get).with(:sidekiq_job_duration) + expect(registry).not_to have_received(:get).with(:sidekiq_job_success) - context "when worker class is ActiveJob::QueueAdapters::SidekiqAdapter::JobWrapper" do - let(:wrapped_class) { "WrappedActiveJobClass" } - let(:job) { {"wrapped" => wrapped_class} } - let(:labels) { {class: wrapped_class, queue: queue, foo: "bar"} } + expect(metric).to have_received(:increment).once.with(labels: failed_labels) + expect(metric).to have_received(:increment).once.with(labels: labels) + expect(metric).not_to have_received(:observe) + end - it "sets the metric class attribute to the wrapped class" do + it "handles sidekiq ent Sidekiq::Limiter::OverLimit errors independently of failures" do SidekiqPrometheus.registry = registry allow(registry).to receive(:get).and_return(metric) - expect { |b| middleware.call(worker, job, queue, &b) }.to yield_control + expect { middleware.call(worker, job, queue) { raise Sidekiq::Limiter::OverLimit } }.to raise_error(Sidekiq::Limiter::OverLimit) expect(registry).to have_received(:get).with(:sidekiq_job_count) - expect(registry).to have_received(:get).with(:sidekiq_job_duration) - expect(registry).to have_received(:get).with(:sidekiq_job_success) - expect(registry).to have_received(:get).with(:sidekiq_job_allocated_objects) + expect(registry).to have_received(:get).with(:sidekiq_job_over_limit) + expect(registry).not_to have_received(:get).with(:sidekiq_job_failed) + expect(registry).not_to have_received(:get).with(:sidekiq_job_duration) + expect(registry).not_to have_received(:get).with(:sidekiq_job_success) expect(metric).to have_received(:increment).twice.with(labels: labels) - expect(metric).to have_received(:observe).twice.with(kind_of(Numeric), labels: labels) + expect(metric).not_to have_received(:observe) + end + + context "when worker class is ActiveJob::QueueAdapters::SidekiqAdapter::JobWrapper" do + let(:wrapped_class) { "WrappedActiveJobClass" } + let(:job) { {"wrapped" => wrapped_class} } + let(:labels) { {class: wrapped_class, queue: queue, foo: "bar"} } + + it "sets the metric class attribute to the wrapped class" do + SidekiqPrometheus.registry = registry + allow(registry).to receive(:get).and_return(metric) + + expect { |b| middleware.call(worker, job, queue, &b) }.to yield_control + + expect(registry).to have_received(:get).with(:sidekiq_job_count) + expect(registry).to have_received(:get).with(:sidekiq_job_duration) + expect(registry).to have_received(:get).with(:sidekiq_job_success) + expect(registry).to have_received(:get).with(:sidekiq_job_allocated_objects) + + expect(metric).to have_received(:increment).twice.with(labels: labels) + expect(metric).to have_received(:observe).twice.with(kind_of(Numeric), labels: labels) + end end end end