From 0b48f6653b1dd735242ff7a4bbfce7d91fd682d6 Mon Sep 17 00:00:00 2001
From: Madelein van Niekerk <mvanniekerk@gitlab.com>
Date: Fri, 24 Nov 2023 15:28:34 +0000
Subject: [PATCH] Introduce Gitlab::CircuitBreaker
Changelog: added
---
.rubocop_todo/rspec/named_subject.yml | 1 -
.../style/inline_disable_annotation.yml | 1 -
config/initializers/circuitbox.rb | 8 +-
ee/lib/gitlab/llm/concerns/circuit_breaker.rb | 36 ------
.../llm/concerns/exponential_backoff.rb | 9 +-
.../llm/concerns/circuit_breaker_spec.rb | 103 ---------------
.../llm/circuit_breaker_shared_examples.rb | 2 +-
lib/gitlab/circuit_breaker.rb | 36 ++++++
.../gitlab/circuit_breaker/notifier.rb | 2 +-
.../gitlab/circuit_breaker/store.rb | 6 -
rubocop/rubocop-code_reuse.yml | 1 +
spec/initializers/circuitbox_spec.rb | 7 +-
.../gitlab/circuit_breaker/notifier_spec.rb | 12 +-
.../lib/gitlab/circuit_breaker/store_spec.rb | 0
spec/lib/gitlab/circuit_breaker_spec.rb | 120 ++++++++++++++++++
15 files changed, 175 insertions(+), 169 deletions(-)
delete mode 100644 ee/lib/gitlab/llm/concerns/circuit_breaker.rb
delete mode 100644 ee/spec/lib/gitlab/llm/concerns/circuit_breaker_spec.rb
create mode 100644 lib/gitlab/circuit_breaker.rb
rename {ee/lib => lib}/gitlab/circuit_breaker/notifier.rb (93%)
rename {ee/lib => lib}/gitlab/circuit_breaker/store.rb (79%)
rename {ee/spec => spec}/lib/gitlab/circuit_breaker/notifier_spec.rb (64%)
rename {ee/spec => spec}/lib/gitlab/circuit_breaker/store_spec.rb (100%)
create mode 100644 spec/lib/gitlab/circuit_breaker_spec.rb
diff --git a/.rubocop_todo/rspec/named_subject.yml b/.rubocop_todo/rspec/named_subject.yml
index 960b5bb5a05a8..75b566464c281 100644
--- a/.rubocop_todo/rspec/named_subject.yml
+++ b/.rubocop_todo/rspec/named_subject.yml
@@ -473,7 +473,6 @@ RSpec/NamedSubject:
- 'ee/spec/lib/gitlab/llm/chat_message_spec.rb'
- 'ee/spec/lib/gitlab/llm/chat_storage_spec.rb'
- 'ee/spec/lib/gitlab/llm/completions/chat_spec.rb'
- - 'ee/spec/lib/gitlab/llm/concerns/circuit_breaker_spec.rb'
- 'ee/spec/lib/gitlab/llm/concerns/exponential_backoff_spec.rb'
- 'ee/spec/lib/gitlab/llm/graphql_subscription_response_service_spec.rb'
- 'ee/spec/lib/gitlab/llm/templates/categorize_question_spec.rb'
diff --git a/.rubocop_todo/style/inline_disable_annotation.yml b/.rubocop_todo/style/inline_disable_annotation.yml
index 37c6c6b93b422..aeefb9ceda364 100644
--- a/.rubocop_todo/style/inline_disable_annotation.yml
+++ b/.rubocop_todo/style/inline_disable_annotation.yml
@@ -2085,7 +2085,6 @@ Style/InlineDisableAnnotation:
- 'ee/spec/lib/gitlab/import_export/project/relation_factory_spec.rb'
- 'ee/spec/lib/gitlab/llm/chain/agents/zero_shot/executor_real_requests_spec.rb'
- 'ee/spec/lib/gitlab/llm/chain/tools/epic_identifier/executor_spec.rb'
- - 'ee/spec/lib/gitlab/llm/concerns/circuit_breaker_spec.rb'
- 'ee/spec/lib/gitlab/mirror_spec.rb'
- 'ee/spec/lib/gitlab/patch/database_config_spec.rb'
- 'ee/spec/lib/gitlab/sitemaps/sitemap_file_spec.rb'
diff --git a/config/initializers/circuitbox.rb b/config/initializers/circuitbox.rb
index 966320265aa5e..55c06cf868b91 100644
--- a/config/initializers/circuitbox.rb
+++ b/config/initializers/circuitbox.rb
@@ -1,8 +1,6 @@
# frozen_string_literal: true
-Gitlab.ee do
- Circuitbox.configure do |config|
- config.default_circuit_store = Gitlab::CircuitBreaker::Store.new
- config.default_notifier = Gitlab::CircuitBreaker::Notifier.new
- end
+Circuitbox.configure do |config|
+ config.default_circuit_store = Gitlab::CircuitBreaker::Store.new
+ config.default_notifier = Gitlab::CircuitBreaker::Notifier.new
end
diff --git a/ee/lib/gitlab/llm/concerns/circuit_breaker.rb b/ee/lib/gitlab/llm/concerns/circuit_breaker.rb
deleted file mode 100644
index d72514eeaff27..0000000000000
--- a/ee/lib/gitlab/llm/concerns/circuit_breaker.rb
+++ /dev/null
@@ -1,36 +0,0 @@
-# frozen_string_literal: true
-
-module Gitlab
- module Llm
- module Concerns
- module CircuitBreaker
- extend ActiveSupport::Concern
-
- InternalServerError = Class.new(StandardError)
-
- ERROR_THRESHOLD = 50
- VOLUME_THRESHOLD = 10
-
- included do
- def circuit
- @circuit ||= Circuitbox.circuit(service_name, {
- exceptions: [InternalServerError],
- error_threshold: ERROR_THRESHOLD,
- volume_threshold: VOLUME_THRESHOLD
- })
- end
- end
-
- def run_with_circuit(&block)
- circuit.run(exception: false, &block)
- end
-
- private
-
- def service_name
- self.class.name
- end
- end
- end
- end
-end
diff --git a/ee/lib/gitlab/llm/concerns/exponential_backoff.rb b/ee/lib/gitlab/llm/concerns/exponential_backoff.rb
index 72c86f8b1dfc0..8f9dc6c90d4b3 100644
--- a/ee/lib/gitlab/llm/concerns/exponential_backoff.rb
+++ b/ee/lib/gitlab/llm/concerns/exponential_backoff.rb
@@ -5,7 +5,6 @@ module Llm
module Concerns
module ExponentialBackoff
extend ActiveSupport::Concern
- include ::Gitlab::Llm::Concerns::CircuitBreaker
INITIAL_DELAY = 1.second
EXPONENTIAL_BASE = 2
@@ -18,7 +17,7 @@ def self.included(base)
end
def retry_with_exponential_backoff(&block)
- run_with_circuit do
+ Gitlab::CircuitBreaker.run_with_circuit(service_name) do
retry_with_monitored_exponential_backoff(&block)
end
end
@@ -46,7 +45,7 @@ def run_retry_with_exponential_backoff
http_response = response.response
return if http_response.nil? || http_response.body.blank?
- raise Gitlab::Llm::Concerns::CircuitBreaker::InternalServerError if response.server_error?
+ raise Gitlab::CircuitBreaker::InternalServerError if response.server_error?
return response unless response.too_many_requests? || retry_immediately?(response)
@@ -65,6 +64,10 @@ def run_retry_with_exponential_backoff
def retry_immediately?(_response)
false
end
+
+ def service_name
+ self.class.name
+ end
end
end
end
diff --git a/ee/spec/lib/gitlab/llm/concerns/circuit_breaker_spec.rb b/ee/spec/lib/gitlab/llm/concerns/circuit_breaker_spec.rb
deleted file mode 100644
index 90e8d243eace5..0000000000000
--- a/ee/spec/lib/gitlab/llm/concerns/circuit_breaker_spec.rb
+++ /dev/null
@@ -1,103 +0,0 @@
-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe Gitlab::Llm::Concerns::CircuitBreaker, :clean_gitlab_redis_rate_limiting, feature_category: :ai_abstraction_layer do
- let(:dummy_class) do
- Class.new do
- include ::Gitlab::Llm::Concerns::CircuitBreaker
-
- def dummy_method
- run_with_circuit do
- raise Gitlab::Llm::Concerns::CircuitBreaker::InternalServerError
- end
- end
-
- def another_dummy_method
- run_with_circuit do
- # Do nothing but successful.
- end
- end
- end
- end
-
- subject { dummy_class.new }
-
- before do
- stub_const('DummyService', dummy_class)
- end
-
- describe '#circuit' do
- it 'returns nil value' do
- expect(Circuitbox).to receive(:circuit).with('DummyService', anything).and_call_original
- expect(subject.dummy_method).to be_nil
- end
-
- it 'does not raise an error' do
- expect(Circuitbox).to receive(:circuit).with('DummyService', anything).and_call_original
- expect { subject.dummy_method }.not_to raise_error
- end
-
- context 'when failed multiple times below volume threshold' do
- it 'does not open the circuit' do
- (described_class::VOLUME_THRESHOLD - 1).times.each do
- subject.dummy_method
- end
-
- expect(subject.circuit).not_to be_open
- end
- end
-
- context 'when failed multiple times over volume threshold' do
- it 'opens the circuit' do
- (described_class::VOLUME_THRESHOLD + 1).times.each do
- subject.dummy_method
- end
-
- expect(subject.circuit).to be_open
- end
- end
-
- context 'when circuit is previously open' do
- before do
- # Opens the circuit
- (described_class::VOLUME_THRESHOLD + 1).times.each do
- subject.dummy_method
- end
-
- # Deletes the open key
- subject.circuit.try_close_next_time
- end
-
- context 'when does not fail again' do
- it 'closes the circuit' do
- subject.another_dummy_method
-
- expect(subject.circuit).not_to be_open
- end
- end
-
- context 'when fails again' do
- it 'opens the circuit' do
- subject.dummy_method
-
- expect(subject.circuit).to be_open
- end
- end
- end
- end
-
- describe '#run_with_circuit' do
- let(:circuit) { double('circuit') } # rubocop: disable RSpec/VerifiedDoubles
- let(:block) { proc {} }
-
- before do
- allow(subject).to receive(:circuit).and_return(circuit)
- end
-
- it 'runs the code block within the Circuitbox circuit' do
- expect(circuit).to receive(:run).with(exception: false, &block)
- subject.run_with_circuit(&block)
- end
- end
-end
diff --git a/ee/spec/support/shared_examples/lib/gitlab/llm/circuit_breaker_shared_examples.rb b/ee/spec/support/shared_examples/lib/gitlab/llm/circuit_breaker_shared_examples.rb
index f4fe78106b292..19b08c42b85a6 100644
--- a/ee/spec/support/shared_examples/lib/gitlab/llm/circuit_breaker_shared_examples.rb
+++ b/ee/spec/support/shared_examples/lib/gitlab/llm/circuit_breaker_shared_examples.rb
@@ -3,7 +3,7 @@
RSpec.shared_examples 'has circuit breaker' do
describe '#call_external_service' do
it 'runs the code block within the circuit breaker' do
- expect(service).to receive(:run_with_circuit)
+ expect(Gitlab::CircuitBreaker).to receive(:run_with_circuit)
subject
end
end
diff --git a/lib/gitlab/circuit_breaker.rb b/lib/gitlab/circuit_breaker.rb
new file mode 100644
index 0000000000000..2b3a6187f273b
--- /dev/null
+++ b/lib/gitlab/circuit_breaker.rb
@@ -0,0 +1,36 @@
+# frozen_string_literal: true
+
+# A configurable circuit breaker to protect the application from external service failures.
+# The circuit measures the amount of failures and if the threshold is exceeded, stops sending requests.
+module Gitlab
+ module CircuitBreaker
+ InternalServerError = Class.new(StandardError)
+
+ DEFAULT_ERROR_THRESHOLD = 50
+ DEFAULT_VOLUME_THRESHOLD = 10
+
+ class << self
+ include ::Gitlab::Utils::StrongMemoize
+
+ # @param [String] unique name for the circuit
+ # @param options [Hash] an options hash setting optional values per circuit
+ def run_with_circuit(service_name, options = {}, &block)
+ circuit(service_name, options).run(exception: false, &block)
+ end
+
+ private
+
+ def circuit(service_name, options)
+ strong_memoize_with(:circuit, service_name, options) do
+ circuit_options = {
+ exceptions: [InternalServerError],
+ error_threshold: DEFAULT_ERROR_THRESHOLD,
+ volume_threshold: DEFAULT_VOLUME_THRESHOLD
+ }.merge(options)
+
+ Circuitbox.circuit(service_name, circuit_options)
+ end
+ end
+ end
+ end
+end
diff --git a/ee/lib/gitlab/circuit_breaker/notifier.rb b/lib/gitlab/circuit_breaker/notifier.rb
similarity index 93%
rename from ee/lib/gitlab/circuit_breaker/notifier.rb
rename to lib/gitlab/circuit_breaker/notifier.rb
index aaa4616696328..b555158ee48b6 100644
--- a/ee/lib/gitlab/circuit_breaker/notifier.rb
+++ b/lib/gitlab/circuit_breaker/notifier.rb
@@ -14,7 +14,7 @@ def notify(service_name, event)
Gitlab::ErrorTracking.track_exception(exception)
end
- def notify_warning(service_name, message)
+ def notify_warning(_service_name, _message)
# no-op
end
diff --git a/ee/lib/gitlab/circuit_breaker/store.rb b/lib/gitlab/circuit_breaker/store.rb
similarity index 79%
rename from ee/lib/gitlab/circuit_breaker/store.rb
rename to lib/gitlab/circuit_breaker/store.rb
index 56e66624d3adf..0ba4f08d5e17a 100644
--- a/ee/lib/gitlab/circuit_breaker/store.rb
+++ b/lib/gitlab/circuit_breaker/store.rb
@@ -4,9 +4,7 @@ module Gitlab
module CircuitBreaker
class Store
def key?(key)
- # rubocop: disable CodeReuse/ActiveRecord
with { |redis| redis.exists?(key) }
- # rubocop: enable CodeReuse/ActiveRecord
end
def store(key, value, opts = {})
@@ -19,14 +17,12 @@ def store(key, value, opts = {})
def increment(key, amount = 1, opts = {})
expires = opts[:expires]
- # rubocop: disable CodeReuse/ActiveRecord
with do |redis|
redis.multi do |multi|
multi.incrby(key, amount)
multi.expire(key, expires) if expires
end
end
- # rubocop: enable CodeReuse/ActiveRecord
end
def load(key, _opts = {})
@@ -44,9 +40,7 @@ def delete(key)
private
def with(&block)
- # rubocop: disable CodeReuse/ActiveRecord
Gitlab::Redis::RateLimiting.with(&block)
- # rubocop: enable CodeReuse/ActiveRecord
rescue ::Redis::BaseConnectionError
# Do not raise an error if we cannot connect to Redis. If
# Redis::RateLimiting is unavailable it should not take the site down.
diff --git a/rubocop/rubocop-code_reuse.yml b/rubocop/rubocop-code_reuse.yml
index 6f9d7902dc681..cf81aa2db5e6d 100644
--- a/rubocop/rubocop-code_reuse.yml
+++ b/rubocop/rubocop-code_reuse.yml
@@ -26,6 +26,7 @@ CodeReuse/ActiveRecord:
- lib/banzai/**/*.rb
- lib/click_house/migration_support/**/*.rb
- lib/gitlab/background_migration/**/*.rb
+ - lib/gitlab/circuit_breaker/store.rb
- lib/gitlab/cycle_analytics/**/*.rb
- lib/gitlab/counters/**/*.rb
- lib/gitlab/database/**/*.rb
diff --git a/spec/initializers/circuitbox_spec.rb b/spec/initializers/circuitbox_spec.rb
index 64e384e4d2240..d0a6d6c9f9152 100644
--- a/spec/initializers/circuitbox_spec.rb
+++ b/spec/initializers/circuitbox_spec.rb
@@ -3,12 +3,7 @@
require 'spec_helper'
RSpec.describe 'circuitbox', feature_category: :shared do
- it 'does not configure Circuitbox', unless: Gitlab.ee? do
- expect(Circuitbox.default_circuit_store).to be_a(Circuitbox::MemoryStore)
- expect(Circuitbox.default_notifier).to be_a(Circuitbox::Notifier::ActiveSupport)
- end
-
- it 'configures Circuitbox', if: Gitlab.ee? do
+ it 'configures Circuitbox' do
expect(Circuitbox.default_circuit_store).to be_a(Gitlab::CircuitBreaker::Store)
expect(Circuitbox.default_notifier).to be_a(Gitlab::CircuitBreaker::Notifier)
end
diff --git a/ee/spec/lib/gitlab/circuit_breaker/notifier_spec.rb b/spec/lib/gitlab/circuit_breaker/notifier_spec.rb
similarity index 64%
rename from ee/spec/lib/gitlab/circuit_breaker/notifier_spec.rb
rename to spec/lib/gitlab/circuit_breaker/notifier_spec.rb
index 3d222c299ff6f..1640ebb99f967 100644
--- a/ee/spec/lib/gitlab/circuit_breaker/notifier_spec.rb
+++ b/spec/lib/gitlab/circuit_breaker/notifier_spec.rb
@@ -2,15 +2,15 @@
require 'spec_helper'
-RSpec.describe Gitlab::CircuitBreaker::Notifier, feature_category: :ai_abstraction_layer do
- subject { described_class.new }
+RSpec.describe Gitlab::CircuitBreaker::Notifier, feature_category: :shared do
+ subject(:instance) { described_class.new }
describe '#notify' do
context 'when event is failure' do
it 'sends an exception to Gitlab::ErrorTracking' do
expect(Gitlab::ErrorTracking).to receive(:track_exception)
- subject.notify('test_service', 'failure')
+ instance.notify('test_service', 'failure')
end
end
@@ -18,20 +18,20 @@
it 'does not send an exception to Gitlab::ErrorTracking' do
expect(Gitlab::ErrorTracking).not_to receive(:track_exception)
- subject.notify('test_service', 'test_event')
+ instance.notify('test_service', 'test_event')
end
end
end
describe '#notify_warning' do
it do
- expect { subject.notify_warning('test_service', 'test_message') }.not_to raise_error
+ expect { instance.notify_warning('test_service', 'test_message') }.not_to raise_error
end
end
describe '#notify_run' do
it do
- expect { subject.notify_run('test_service') { puts 'test block' } }.not_to raise_error
+ expect { instance.notify_run('test_service') { puts 'test block' } }.not_to raise_error
end
end
end
diff --git a/ee/spec/lib/gitlab/circuit_breaker/store_spec.rb b/spec/lib/gitlab/circuit_breaker/store_spec.rb
similarity index 100%
rename from ee/spec/lib/gitlab/circuit_breaker/store_spec.rb
rename to spec/lib/gitlab/circuit_breaker/store_spec.rb
diff --git a/spec/lib/gitlab/circuit_breaker_spec.rb b/spec/lib/gitlab/circuit_breaker_spec.rb
new file mode 100644
index 0000000000000..4cd2f41869eee
--- /dev/null
+++ b/spec/lib/gitlab/circuit_breaker_spec.rb
@@ -0,0 +1,120 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::CircuitBreaker, :clean_gitlab_redis_rate_limiting, feature_category: :shared do
+ let(:service_name) { 'DummyService' }
+ let(:volume_threshold) { 5 }
+ let(:circuit) do
+ Circuitbox.circuit(service_name,
+ { volume_threshold: volume_threshold, exceptions: [Gitlab::CircuitBreaker::InternalServerError] })
+ end
+
+ let(:dummy_class) do
+ Class.new do
+ def dummy_method
+ Gitlab::CircuitBreaker.run_with_circuit('DummyService') do
+ raise Gitlab::CircuitBreaker::InternalServerError
+ end
+ end
+
+ def another_dummy_method
+ Gitlab::CircuitBreaker.run_with_circuit('DummyService') do
+ # Do nothing but successful.
+ end
+ end
+ end
+ end
+
+ subject(:instance) { dummy_class.new }
+
+ before do
+ stub_const(service_name, dummy_class)
+ allow(Circuitbox).to receive(:circuit).and_return(circuit)
+ end
+
+ # rubocop: disable RSpec/AnyInstanceOf -- the instance is defined by an initializer
+ describe '#circuit' do
+ it 'returns nil value' do
+ expect(instance.dummy_method).to be_nil
+ end
+
+ it 'does not raise an error' do
+ expect { instance.dummy_method }.not_to raise_error
+ end
+
+ context 'when failed multiple times below volume threshold' do
+ it 'does not open the circuit' do
+ expect_any_instance_of(Gitlab::CircuitBreaker::Notifier).to receive(:notify)
+ .with(anything, 'failure')
+ .exactly(4).times
+
+ 4.times do
+ instance.dummy_method
+ end
+
+ expect(circuit).not_to be_open
+ end
+ end
+
+ context 'when failed multiple times over volume threshold' do
+ it 'allows the call 5 times, then opens the circuit and skips subsequent calls' do
+ expect_any_instance_of(Gitlab::CircuitBreaker::Notifier).to receive(:notify)
+ .with(anything, 'failure')
+ .exactly(5).times
+
+ expect_any_instance_of(Gitlab::CircuitBreaker::Notifier).to receive(:notify)
+ .with(anything, 'open')
+ .once
+
+ expect_any_instance_of(Gitlab::CircuitBreaker::Notifier).to receive(:notify)
+ .with(anything, 'skipped')
+ .once
+
+ 6.times do
+ instance.dummy_method
+ end
+
+ expect(circuit).to be_open
+ end
+ end
+
+ context 'when circuit is previously open' do
+ before do
+ # Opens the circuit
+ 6.times do
+ instance.dummy_method
+ end
+
+ # Deletes the open key
+ circuit.try_close_next_time
+ end
+
+ context 'when does not fail again' do
+ it 'closes the circuit' do
+ instance.another_dummy_method
+
+ expect(circuit).not_to be_open
+ end
+ end
+
+ context 'when fails again' do
+ it 'opens the circuit' do
+ instance.dummy_method
+
+ expect(circuit).to be_open
+ end
+ end
+ end
+ end
+ # rubocop: enable RSpec/AnyInstanceOf
+
+ describe '#run_with_circuit' do
+ let(:block) { proc {} }
+
+ it 'runs the code block within the Circuitbox circuit' do
+ expect(circuit).to receive(:run).with(exception: false, &block)
+ described_class.run_with_circuit('service', &block)
+ end
+ end
+end
--
GitLab