diff --git a/README.md b/README.md index c61b91f..6cebcab 100644 --- a/README.md +++ b/README.md @@ -202,6 +202,7 @@ module Freno DEFAULT_WAIT_SECONDS = 0.5 DEFAULT_MAX_WAIT_SECONDS = 10 + DEFAULT_REQUIRED_CONSECUTIVE_SUCCESSES = 3 def initialize(client: nil, app: nil, @@ -209,7 +210,9 @@ module Freno instrumenter: Instrumenter::Noop, circuit_breaker: CircuitBreaker::Noop, wait_seconds: DEFAULT_WAIT_SECONDS, - max_wait_seconds: DEFAULT_MAX_WAIT_SECONDS) + max_wait_seconds: DEFAULT_MAX_WAIT_SECONDS, + required_consecutive_successes: DEFAULT_REQUIRED_CONSECUTIVE_SUCCESSES, + stable_recovery: false) @client = client @@ -219,6 +222,8 @@ module Freno @circuit_breaker = circuit_breaker @wait_seconds = wait_seconds @max_wait_seconds = max_wait_seconds + @required_consecutive_successes = required_consecutive_successes + @stable_recovery = stable_recovery yield self if block_given? @@ -238,6 +243,50 @@ You optionally provide the time you want the throttler to sleep in case the chec If replication lags badly, you can control until when you want to keep sleeping and retrying the check by setting `max_wait_seconds`. When that times out, the throttle will raise a `Freno::Throttler::WaitedTooLong` error. +Stable recovery is opt-in (`stable_recovery: false` by default), preserving +legacy recovery and timeout circuit-breaker behavior when upgrading the gem. +With `stable_recovery: true`, an initially healthy check proceeds immediately. After any failed check, +`required_consecutive_successes` passing checks are required before the block +runs. Passing samples must be consecutive; another rejection resets the count. +This prevents an oscillating metric from releasing work on a single healthy +trough. + +With stable recovery enabled, `WaitedTooLong` represents normal sustained +throttling and does not count as a circuit-breaker failure. Freno transport and +decision errors still fail the circuit breaker. + +#### Rolling out stable recovery + +Gate `stable_recovery` with the consuming application's feature switch and enable +it for one reviewed workload first: + +```ruby +Freno::Throttler.new( + client: freno_client, + app: :my_app, + stable_recovery: true, + required_consecutive_successes: 3, + max_wait_seconds: 45 +) +``` + +The example wait budget accommodates the proposed server-side 30-second +continuous recovery window plus client stabilization; it does not guarantee +recovery during sustained overload. Keep the default 10-second budget for +existing callers unless their owners explicitly approve a longer blocking +period. Callers with a shorter budget must safely defer/retry `WaitedTooLong` +without executing the protected work. + +Observe `throttler.waited`, `throttler.waited_too_long`, client errors, job +throughput and backlog. Recovery instrumentation includes `stable_recovery` and +consecutive-success progress. Switch `stable_recovery` back to false to restore +both legacy recovery and timeout circuit-breaker behavior. Setting only +`required_consecutive_successes: 1` does not restore the latter. + +Recovery progress is local to a single `throttle` call, not shared between +calls, workers, or processes. Deploy the compatible server before enabling its +primary dependencies; server enforcement and this client switch are independent. + #### Instrumenting the throttler You can also configure the throttler with an `instrumenter` collaborator to subscribe to events happening during the `throttle` call. @@ -287,7 +336,10 @@ The throttler can also receive a `circuit_breaker` object to implement resilienc With that information it receives, the circuit breaker determines whether or not to allow the next request. A circuit is said to be open when the next request is not allowed; and it's said to be closed when the next request is allowed -If the throttler waited too long, or an unexpected error happened; the circuit breaker will receive a `failure`. If in contrast it succeeded, the circuit breaker will receive a `success` message. +If an unexpected Freno transport or decision error happens, the circuit breaker +receives a `failure`. Sustained throttling that raises `WaitedTooLong` also fails +the circuit breaker unless `stable_recovery: true`. When a check succeeds, the +circuit breaker receives a `success` message. Once the circuit is open, the throttler will not try to throttle calls, an instead throw a `Freno::Throttler::CircuitOpen` diff --git a/lib/freno/client.rb b/lib/freno/client.rb index 36505e1..4674bb5 100644 --- a/lib/freno/client.rb +++ b/lib/freno/client.rb @@ -233,7 +233,7 @@ def decorated(request) outermost = to_decorate[0] current = outermost - (to_decorate[1..]).each do |decorator| + to_decorate[1..].each do |decorator| current.request = decorator current = current.request end diff --git a/lib/freno/client/errors.rb b/lib/freno/client/errors.rb index afbb591..fbbb7a1 100644 --- a/lib/freno/client/errors.rb +++ b/lib/freno/client/errors.rb @@ -1,5 +1,6 @@ # frozen_string_literal: true module Freno - Error = Class.new(StandardError) + class Error < StandardError + end end diff --git a/lib/freno/client/preconditions.rb b/lib/freno/client/preconditions.rb index a49174b..2c5ed55 100644 --- a/lib/freno/client/preconditions.rb +++ b/lib/freno/client/preconditions.rb @@ -5,7 +5,8 @@ class Client module Preconditions module_function - PreconditionNotMet = Class.new(ArgumentError) + class PreconditionNotMet < ArgumentError + end class Checker attr_reader :errors diff --git a/lib/freno/throttler.rb b/lib/freno/throttler.rb index aec22bb..0886ddd 100644 --- a/lib/freno/throttler.rb +++ b/lib/freno/throttler.rb @@ -35,6 +35,7 @@ module Freno class Throttler DEFAULT_WAIT_SECONDS = 0.5 DEFAULT_MAX_WAIT_SECONDS = 10 + DEFAULT_REQUIRED_CONSECUTIVE_SUCCESSES = 3 REQUIRED_ARGS = %i[ client app @@ -43,6 +44,7 @@ class Throttler circuit_breaker wait_seconds max_wait_seconds + required_consecutive_successes ].freeze attr_accessor :client, @@ -51,7 +53,9 @@ class Throttler :instrumenter, :circuit_breaker, :wait_seconds, - :max_wait_seconds + :max_wait_seconds, + :required_consecutive_successes, + :stable_recovery # Initializes a new instance of the throttler # @@ -99,6 +103,12 @@ class Throttler # seconds the throttler will wait in total for replicas to catch-up # before raising a `WaitedTooLong` error. # + # - `:required_consecutive_successes`: The number of consecutive passing + # checks required after a failed check when stable recovery is enabled. + # + # - `:stable_recovery`: Opt into consecutive-success recovery and treating + # WaitedTooLong as throttling rather than a circuit-breaker failure. + # def initialize( client: nil, app: nil, @@ -106,7 +116,9 @@ def initialize( instrumenter: Instrumenter::Noop, circuit_breaker: CircuitBreaker::Noop, wait_seconds: DEFAULT_WAIT_SECONDS, - max_wait_seconds: DEFAULT_MAX_WAIT_SECONDS + max_wait_seconds: DEFAULT_MAX_WAIT_SECONDS, + required_consecutive_successes: DEFAULT_REQUIRED_CONSECUTIVE_SUCCESSES, + stable_recovery: false ) @client = client @app = app @@ -115,6 +127,8 @@ def initialize( @circuit_breaker = circuit_breaker @wait_seconds = wait_seconds @max_wait_seconds = max_wait_seconds + @required_consecutive_successes = required_consecutive_successes + @stable_recovery = stable_recovery yield self if block_given? @@ -151,11 +165,11 @@ def initialize( # # - "throttler.called" each time this method is called # - "throttler.succeeded" when the stores were ok, before yielding the block - # - "throttler.waited" when the stores were not ok, after waiting - # `wait_seconds` - # - "throttler.waited_too_long" when the stores were not ok, but the - # thottler already waited at least `max_wait_seconds`, right before - # raising `WaitedTooLong` + # - "throttler.waited" after waiting `wait_seconds` because stores were not + # ok or recovery had not yet reached the consecutive-success threshold + # - "throttler.waited_too_long" when stores remain unavailable or recovery + # stabilization exceeds `max_wait_seconds`, right before raising + # `WaitedTooLong` # - "throttler.freno_errored" when there was an error with freno, before # raising `ClientError`. # - "throttler.circuit_open" when the circuit breaker does not allow the @@ -165,6 +179,8 @@ def throttle(context = nil, **options) store_names = mapper.call(context) instrument(:called, store_names: store_names) waited = 0 + throttled = false + consecutive_successes = 0 while true unless circuit_breaker.allow_request? @@ -173,19 +189,47 @@ def throttle(context = nil, **options) end if all_stores_ok?(store_names, **options) - instrument(:succeeded, store_names: store_names, waited: waited) - circuit_breaker.success - break + consecutive_successes += 1 + if !stable_recovery || !throttled || consecutive_successes >= required_consecutive_successes + instrument( + :succeeded, + store_names: store_names, + waited: waited, + consecutive_successes: consecutive_successes, + stable_recovery: stable_recovery + ) + circuit_breaker.success + break + end + else + throttled = true + consecutive_successes = 0 end if waited + wait_seconds > max_wait_seconds - instrument(:waited_too_long, store_names: store_names, waited: waited, max: max_wait_seconds) - circuit_breaker.failure + instrument( + :waited_too_long, + store_names: store_names, + waited: waited, + max: max_wait_seconds, + consecutive_successes: consecutive_successes, + required_consecutive_successes: required_consecutive_successes, + stable_recovery: stable_recovery + ) + circuit_breaker.failure unless stable_recovery raise WaitedTooLong.new(waited_seconds: waited, max_wait_seconds: max_wait_seconds) else wait waited += wait_seconds - instrument(:waited, store_names: store_names, waited: waited, max: max_wait_seconds) + instrument( + :waited, + store_names: store_names, + waited: waited, + max: max_wait_seconds, + consecutive_successes: consecutive_successes, + required_consecutive_successes: required_consecutive_successes, + stable_recovery: stable_recovery + ) end end @@ -204,6 +248,12 @@ def validate_args unless max_wait_seconds > wait_seconds errors << "max_wait_seconds (#{max_wait_seconds}) has to be greather than wait_seconds (#{wait_seconds})" end + unless required_consecutive_successes.is_a?(Integer) && required_consecutive_successes.positive? + errors << "required_consecutive_successes (#{required_consecutive_successes}) must be a positive integer" + end + unless [true, false].include?(stable_recovery) + errors << "stable_recovery (#{stable_recovery}) must be true or false" + end raise ArgumentError, errors.join("\n") if errors.any? end diff --git a/test/freno/throttler_test.rb b/test/freno/throttler_test.rb index 7053983..8ea4e1c 100644 --- a/test/freno/throttler_test.rb +++ b/test/freno/throttler_test.rb @@ -10,6 +10,20 @@ def test_validations assert_includes ex.message, "app must be provided" assert_includes ex.message, "client must be provided" assert_includes ex.message, "max_wait_seconds (0.5) has to be greather than wait_seconds (1)" + + ex = assert_raises(ArgumentError) do + Freno::Throttler.new( + client: sample_client, + app: :github, + required_consecutive_successes: 0 + ) + end + assert_includes ex.message, "required_consecutive_successes (0) must be a positive integer" + + ex = assert_raises(ArgumentError) do + Freno::Throttler.new(client: sample_client, app: :github, stable_recovery: "false") + end + assert_includes ex.message, "stable_recovery (false) must be true or false" end def test_using_the_default_identity_mapper @@ -80,17 +94,18 @@ def test_sleeps_when_a_check_fails_and_then_calls_the_block block_called = false stub = sample_client - stub.expects(:check?).times(2) + stub.expects(:check?).times(4) .with(app: :github, store_name: :mysqla, options: {}) - .returns(false).then.returns(true) + .returns(false).then.returns(true).then.returns(true).then.returns(true) throttler = Freno::Throttler.new do |t| t.client = stub t.app = :github t.mapper = ->(_context) { [:mysqla] } t.instrumenter = MemoryInstrumenter.new + t.stable_recovery = true end - throttler.expects(:wait).once + throttler.expects(:wait).times(3) throttler.throttle do block_called = true @@ -105,10 +120,12 @@ def test_sleeps_when_a_check_fails_and_then_calls_the_block waited_events = throttler.instrumenter.events_for("throttler.waited") - assert_equal 1, waited_events.count + assert_equal 3, waited_events.count assert_equal [:mysqla], waited_events.first[:store_names] assert_in_delta 0.5, waited_events.first[:waited], 0.01 assert_equal 10, waited_events.first[:max] + assert_equal 3, waited_events.last[:required_consecutive_successes] + assert_equal 2, waited_events.last[:consecutive_successes] assert_equal 0, throttler.instrumenter.count("throttler.waited_too_long") assert_equal 0, throttler.instrumenter.count("throttler.freno_errored") @@ -158,6 +175,125 @@ def test_raises_waited_too_long_if_freno_checks_failed_consistenly assert_equal 0, throttler.instrumenter.count("throttler.circuit_open") end + def test_recovery_requires_consecutive_passing_checks + block_called = false + + stub = sample_client + stub.expects(:check?).times(7) + .with(app: :github, store_name: :mysqla, options: {}) + .returns(false) + .then.returns(true) + .then.returns(false) + .then.returns(true) + .then.returns(true) + .then.returns(false) + .then.returns(true) + + throttler = Freno::Throttler.new do |t| + t.client = stub + t.app = :github + t.mapper = ->(_context) { [:mysqla] } + t.instrumenter = MemoryInstrumenter.new + t.wait_seconds = 1 + t.max_wait_seconds = 6 + t.stable_recovery = true + end + throttler.expects(:wait).times(6) + + assert_raises(Freno::Throttler::WaitedTooLong) do + throttler.throttle do + block_called = true + end + end + + refute block_called + + event = throttler.instrumenter.events_for("throttler.waited_too_long").first + + assert_equal 1, event[:consecutive_successes] + assert_equal 3, event[:required_consecutive_successes] + end + + def test_recovery_uses_configured_consecutive_success_threshold + block_called = false + client = sample_client + client.expects(:check?).times(3) + .with(app: :github, store_name: :mysqla, options: {}) + .returns(false).then.returns(true).then.returns(true) + + throttler = Freno::Throttler.new( + client: client, + app: :github, + instrumenter: MemoryInstrumenter.new, + stable_recovery: true, + required_consecutive_successes: 2 + ) + throttler.expects(:wait).times(2) + + throttler.throttle(:mysqla) do + block_called = true + end + + assert block_called + event = throttler.instrumenter.events_for("throttler.succeeded").first + + assert_equal 2, event[:consecutive_successes] + end + + def test_waited_too_long_does_not_fail_the_circuit_breaker + client = sample_client + client.stubs(:check?).returns(false) + circuit_breaker = mock + circuit_breaker.stubs(:allow_request?).returns(true) + circuit_breaker.expects(:failure).never + + throttler = Freno::Throttler.new( + client: client, + app: :github, + circuit_breaker: circuit_breaker, + wait_seconds: 1, + max_wait_seconds: 2, + stable_recovery: true + ) + throttler.expects(:wait).times(2) + + assert_raises(Freno::Throttler::WaitedTooLong) do + throttler.throttle(:mysqla) { flunk "throttled block should not run" } + end + end + + def test_default_recovery_releases_after_one_passing_check + client = sample_client + client.expects(:check?).twice.returns(false).then.returns(true) + throttler = Freno::Throttler.new(client: client, app: :github) + throttler.expects(:wait).once + block_called = false + + throttler.throttle(:mysqla) { block_called = true } + + assert block_called + end + + def test_default_timeout_still_fails_the_circuit_breaker + client = sample_client + client.stubs(:check?).returns(false) + circuit_breaker = mock + circuit_breaker.stubs(:allow_request?).returns(true) + circuit_breaker.expects(:failure).once + throttler = Freno::Throttler.new( + client: client, + app: :github, + circuit_breaker: circuit_breaker, + wait_seconds: 1, + max_wait_seconds: 2 + ) + throttler.expects(:wait).times(2) + + assert_raises(Freno::Throttler::WaitedTooLong) do + throttler.throttle(:mysqla) { flunk "throttled block should not run" } + end + end + def test_raises_a_specific_error_in_case_freno_itself_errored block_called = false