Skip to content

Commit

Permalink
Fix: Accomodate apps that do not use Sidekiq Enterprise (#42)
Browse files Browse the repository at this point in the history
* 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 <leklund@fastly.com>
  • Loading branch information
jayroh and leklund authored Nov 2, 2023
1 parent cd7c5ba commit 3d643fd
Show file tree
Hide file tree
Showing 2 changed files with 92 additions and 57 deletions.
2 changes: 1 addition & 1 deletion lib/sidekiq_prometheus/job_metrics.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand Down
147 changes: 91 additions & 56 deletions spec/sidekiq_prometheus/job_metrics_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand All @@ -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
Expand Down

0 comments on commit 3d643fd

Please sign in to comment.