From 4df6e27b41f882ba88893b8894ae02d9de990a6f Mon Sep 17 00:00:00 2001 From: Justin Howard Date: Thu, 3 Feb 2022 16:51:42 -0800 Subject: [PATCH] Add granular errors for Elasticsearch patch With this change, the granular errors normally raised by Elasticsearch will be preserved. Previously we were discarding the granular error information raised by the Elasticsearch client and simply raising an "Error" subclass. Now we raise a Faulty error which is always a subclass of whichever error was raised by Elasticsearch itself. To do this, we add support for procs in the new error_mapper option for circuits. This allows us to individually map the errors returned by Elasticsearch. Add Faulty::Deprecation for deprecation notification DEPRECATIONS: - The error_module option is deprecated. Patches should use the error_module option instead. The option will be removed in 0.9 --- lib/faulty.rb | 1 + lib/faulty/circuit.rb | 40 ++++++++++++++++++++++++------ lib/faulty/deprecation.rb | 37 ++++++++++++++++++++++++++++ lib/faulty/error.rb | 3 +++ lib/faulty/patch.rb | 11 +++++---- lib/faulty/patch/elasticsearch.rb | 30 ++++++++++++++++++++-- lib/faulty/patch/mysql2.rb | 2 +- lib/faulty/patch/redis.rb | 2 +- spec/circuit_spec.rb | 36 +++++++++++++++++++++++++++ spec/deprecation_spec.rb | 41 +++++++++++++++++++++++++++++++ spec/patch/elasticsearch_spec.rb | 22 ++++++++++++++++- spec/patch_spec.rb | 22 ++++++++--------- 12 files changed, 218 insertions(+), 29 deletions(-) create mode 100644 lib/faulty/deprecation.rb create mode 100644 spec/deprecation_spec.rb diff --git a/lib/faulty.rb b/lib/faulty.rb index 3e5244a..a1dbf5f 100644 --- a/lib/faulty.rb +++ b/lib/faulty.rb @@ -4,6 +4,7 @@ require 'forwardable' require 'concurrent' +require 'faulty/deprecation' require 'faulty/immutable_options' require 'faulty/cache' require 'faulty/circuit' diff --git a/lib/faulty/circuit.rb b/lib/faulty/circuit.rb index f9e8cdb..a50fd71 100644 --- a/lib/faulty/circuit.rb +++ b/lib/faulty/circuit.rb @@ -49,9 +49,13 @@ class Circuit # rubocop:disable Metrics/ClassLength # @!attribute [r] cool_down # @return [Integer] The number of seconds the circuit will # stay open after it is tripped. Default 300. - # @!attribute [r] error_module - # @return [Module] Used by patches to set the namespace module for - # the faulty errors that will be raised. Default `Faulty` + # @!attribute [r] error_mapper + # @return [Module, #call] Used by patches to set the namespace module for + # the faulty errors that will be raised. Should be a module or a callable. + # If given a module, the circuit assumes the module has error classes + # in that module. If given an object that responds to `#call` (a proc + # or lambda), the return value of the callable will be used. The callable + # is called with (`error_name`, `cause_error`, `circuit`). Default `Faulty` # @!attribute [r] evaluation_window # @return [Integer] The number of seconds of history that # will be evaluated to determine the failure rate for a circuit. @@ -93,6 +97,7 @@ class Circuit # rubocop:disable Metrics/ClassLength :rate_threshold, :sample_threshold, :errors, + :error_mapper, :error_module, :exclude, :cache, @@ -120,7 +125,7 @@ def defaults cache_refreshes_after: 900, cool_down: 300, errors: [StandardError], - error_module: Faulty, + error_mapper: Faulty, exclude: [], evaluation_window: 60, rate_threshold: 0.5, @@ -133,7 +138,7 @@ def required cache cool_down errors - error_module + error_mapper exclude evaluation_window rate_threshold @@ -153,6 +158,17 @@ def finalize unless cache_refreshes_after.nil? self.cache_refresh_jitter = 0.2 * cache_refreshes_after end + + deprecated_error_module + end + + private + + def deprecated_error_module + return unless error_module + + Deprecation.method(self.class, :error_module, note: 'See :error_mapper', sunset: '0.9.0') + self.error_mapper = error_module end end @@ -379,7 +395,7 @@ def push_options # @return The result from cache if available def run_skipped(cached_value) skipped! - raise options.error_module::OpenCircuitError.new(nil, self) if cached_value.nil? + raise map_error(:OpenCircuitError) if cached_value.nil? cached_value end @@ -400,9 +416,9 @@ def run_exec(status, cached_value, cache_key) opened = failure!(status, e) if cached_value.nil? if opened - raise options.error_module::CircuitTrippedError.new(e.message, self) + raise map_error(:CircuitTrippedError, e) else - raise options.error_module::CircuitFailureError.new(e.message, self) + raise map_error(:CircuitFailureError, e) end else cached_value @@ -531,5 +547,13 @@ def storage @given_options.storage end + + def map_error(error_name, cause = nil) + if options.error_mapper.respond_to?(:call) + options.error_mapper.call(error_name, cause, self) + else + options.error_mapper.const_get(error_name).new(cause&.message, self) + end + end end end diff --git a/lib/faulty/deprecation.rb b/lib/faulty/deprecation.rb new file mode 100644 index 0000000..a737714 --- /dev/null +++ b/lib/faulty/deprecation.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +class Faulty + # Support deprecating Faulty features + module Deprecation + class << self + # Call to raise errors instead of logging warnings for Faulty deprecations + def raise_errors!(enabled = true) + @raise_errors = (enabled == true) + end + + def silenced + @silence = true + yield + ensure + @silence = false + end + + # @private + def method(klass, name, note: nil, sunset: nil) + deprecate("#{klass}##{name}", note: note, sunset: sunset) + end + + # @private + def deprecate(subject, note: nil, sunset: nil) + return if @silence + + message = "#{subject} is deprecated" + message += " and will be removed in #{sunset}" if sunset + message += " (#{note})" if note + raise DeprecationError, message if @raise_errors + + Kernel.warn("DEPRECATION: #{message}") + end + end + end +end diff --git a/lib/faulty/error.rb b/lib/faulty/error.rb index 684684c..211732e 100644 --- a/lib/faulty/error.rb +++ b/lib/faulty/error.rb @@ -28,6 +28,9 @@ def initialize(message = nil) end end + class DeprecationError < FaultyError + end + # Included in faulty circuit errors to provide common features for # native and patched errors module CircuitErrorBase diff --git a/lib/faulty/patch.rb b/lib/faulty/patch.rb index 831d843..74dcd36 100644 --- a/lib/faulty/patch.rb +++ b/lib/faulty/patch.rb @@ -64,14 +64,15 @@ class << self # option and these additional options # @option hash [String] :name The circuit name. Defaults to `default_name` # @option hash [Boolean] :patch_errors By default, circuit errors will be - # subclasses of `options[:patched_error_module]`. The user can disable + # subclasses of `options[:patched_error_mapper]`. The user can disable # this by setting this option to false. # @option hash [Faulty, String, Symbol, Hash{ constant: String }] :instance # A reference to a faulty instance. See examples. # @param options [Hash] Additional override options. Supports any circuit # option and these additional ones. - # @option options [Module] :patched_error_module The namespace module - # for patched errors + # @option options [Module] :patched_error_mapper The namespace module + # for patched errors or a mapping proc. See {Faulty::Circuit::Options} + # `:error_mapper` # @yield [Circuit::Options] For setting override options in a block # @return [Circuit, nil] The circuit if one was created def circuit_from_hash(default_name, hash, **options, &block) @@ -80,8 +81,8 @@ def circuit_from_hash(default_name, hash, **options, &block) hash = symbolize_keys(hash) name = hash.delete(:name) || default_name patch_errors = hash.delete(:patch_errors) != false - error_module = options.delete(:patched_error_module) - hash[:error_module] ||= error_module if error_module && patch_errors + error_mapper = options.delete(:patched_error_mapper) + hash[:error_mapper] ||= error_mapper if error_mapper && patch_errors faulty = resolve_instance(hash.delete(:instance)) faulty.circuit(name, **hash, **options, &block) end diff --git a/lib/faulty/patch/elasticsearch.rb b/lib/faulty/patch/elasticsearch.rb index 78abee0..c9d4d4d 100644 --- a/lib/faulty/patch/elasticsearch.rb +++ b/lib/faulty/patch/elasticsearch.rb @@ -36,7 +36,32 @@ module Patch module Elasticsearch include Base - Patch.define_circuit_errors(self, ::Elasticsearch::Transport::Transport::Error) + module Error; end + module SnifferTimeoutError; end + module ServerError; end + + # We will freeze this after adding the dynamic error classes + MAPPED_ERRORS = { # rubocop:disable Style/MutableConstant + ::Elasticsearch::Transport::Transport::Error => Error, + ::Elasticsearch::Transport::Transport::SnifferTimeoutError => SnifferTimeoutError, + ::Elasticsearch::Transport::Transport::ServerError => ServerError + } + + module Errors + ::Elasticsearch::Transport::Transport::ERRORS.each do |_code, klass| + MAPPED_ERRORS[klass] = const_set(klass.name.split('::').last, Module.new) + end + end + + MAPPED_ERRORS.freeze + MAPPED_ERRORS.each do |klass, mod| + Patch.define_circuit_errors(mod, klass) + end + + ERROR_MAPPER = lambda do |error_name, cause, circuit| + MAPPED_ERRORS.fetch(cause&.class, Error).const_get(error_name).new(cause&.message, circuit) + end + private_constant :ERROR_MAPPER, :MAPPED_ERRORS def initialize(arguments = {}, &block) super @@ -48,7 +73,8 @@ def initialize(arguments = {}, &block) 'elasticsearch', arguments[:faulty], errors: errors, - patched_error_module: Faulty::Patch::Elasticsearch + exclude: ::Elasticsearch::Transport::Transport::Errors::NotFound, + patched_error_mapper: ERROR_MAPPER ) end diff --git a/lib/faulty/patch/mysql2.rb b/lib/faulty/patch/mysql2.rb index fd1bdc3..0afd0c7 100644 --- a/lib/faulty/patch/mysql2.rb +++ b/lib/faulty/patch/mysql2.rb @@ -53,7 +53,7 @@ def initialize(opts = {}) ::Mysql2::Error::ConnectionError, ::Mysql2::Error::TimeoutError ], - patched_error_module: Faulty::Patch::Mysql2 + patched_error_mapper: Faulty::Patch::Mysql2 ) super diff --git a/lib/faulty/patch/redis.rb b/lib/faulty/patch/redis.rb index 7c2fa77..f375d06 100644 --- a/lib/faulty/patch/redis.rb +++ b/lib/faulty/patch/redis.rb @@ -43,7 +43,7 @@ def initialize(options = {}) ::Redis::BaseConnectionError, BusyError ], - patched_error_module: Faulty::Patch::Redis + patched_error_mapper: Faulty::Patch::Redis ) super diff --git a/spec/circuit_spec.rb b/spec/circuit_spec.rb index b14f463..e33cbfb 100644 --- a/spec/circuit_spec.rb +++ b/spec/circuit_spec.rb @@ -279,6 +279,22 @@ expect(circuit.options.cool_down).to eq(300) end + context 'with error_mapper module' do + let(:options) do + { + cache: cache, + error_mapper: custom_error_module, + storage: storage + } + end + + it 'raises custom errors' do + expect do + circuit.run { raise 'fail' } + end.to raise_error(custom_error_module::CircuitFailureError) + end + end + context 'with error_module' do let(:options) do { @@ -288,12 +304,32 @@ } end + around { |example| Faulty::Deprecation.silenced(&example) } + it 'raises custom errors' do expect do circuit.run { raise 'fail' } end.to raise_error(custom_error_module::CircuitFailureError) end end + + context 'with error_mapper lambda' do + let(:options) do + { + cache: cache, + error_mapper: lambda do |error_name, cause, circuit| + custom_error_module.const_get(error_name).new("mapped #{cause.message}", circuit) + end, + storage: storage + } + end + + it 'raises mapped error' do + expect do + circuit.run { raise 'fail' } + end.to raise_error(custom_error_module::CircuitFailureError, /.*mapped fail/) + end + end end context 'with memory storage' do diff --git a/spec/deprecation_spec.rb b/spec/deprecation_spec.rb new file mode 100644 index 0000000..759455d --- /dev/null +++ b/spec/deprecation_spec.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +RSpec.describe Faulty::Deprecation do + it 'prints note and sunset version' do + expect(Kernel).to receive(:warn) + .with('DEPRECATION: foo is deprecated and will be removed in 1.0 (Use bar)') + described_class.deprecate(:foo, note: 'Use bar', sunset: '1.0') + end + + it 'prints only subject' do + expect(Kernel).to receive(:warn) + .with('DEPRECATION: blah is deprecated') + described_class.deprecate('blah') + end + + it 'prints method deprecation' do + expect(Kernel).to receive(:warn) + .with('DEPRECATION: Faulty::Circuit#foo is deprecated and will be removed in 1.0 (Use bar)') + described_class.method(Faulty::Circuit, :foo, note: 'Use bar', sunset: '1.0') + end + + context 'with raise_errors!' do + before { described_class.raise_errors! } + + after { described_class.raise_errors!(false) } + + it 'raises DeprecationError' do + expect { described_class.deprecate('blah') } + .to raise_error(Faulty::DeprecationError, 'blah is deprecated') + end + end + + context 'when silenced' do + it 'does not surface deprecations' do + expect(Kernel).not_to receive(:warn) + described_class.silenced do + described_class.deprecate('blah') + end + end + end +end diff --git a/spec/patch/elasticsearch_spec.rb b/spec/patch/elasticsearch_spec.rb index 944f8b7..9d5e39a 100644 --- a/spec/patch/elasticsearch_spec.rb +++ b/spec/patch/elasticsearch_spec.rb @@ -21,6 +21,7 @@ def build_client(**options) expect { patched_bad_client.perform_request('GET', '_cluster/state') } .to raise_error do |error| expect(error).to be_a(Elasticsearch::Transport::Transport::Error) + expect(error.class).to eq(Faulty::Patch::Elasticsearch::Error::CircuitFailureError) expect(error).to be_a(Faulty::CircuitErrorBase) expect(error.cause).to be_a(Faraday::ConnectionFailed) end @@ -48,9 +49,28 @@ def build_client(**options) it 'raises unpatched errors if configured to' do expect { bad_client_unpatched_errors.perform_request('GET', '_cluster/state') } .to raise_error do |error| - expect(error).to be_a(Faulty::CircuitError) + expect(error.class).to eq(Faulty::CircuitFailureError) expect(error.cause).to be_a(Faraday::ConnectionFailed) end expect(faulty.circuit('elasticsearch').status.failure_rate).to eq(1) end + + it 'raises case-specific Elasticsearch errors' do + expect { patched_good_client.perform_request('PUT', '') } + .to raise_error do |error| + expect(error).to be_a(Elasticsearch::Transport::Transport::Errors::MethodNotAllowed) + expect(error.class).to eq(Faulty::Patch::Elasticsearch::Errors::MethodNotAllowed::CircuitFailureError) + expect(error).to be_a(Faulty::CircuitErrorBase) + expect(error.cause.class).to eq(Elasticsearch::Transport::Transport::Errors::MethodNotAllowed) + end + expect(faulty.circuit('elasticsearch').status.failure_rate).to eq(1) + end + + it 'ignores 404 errors' do + expect { patched_good_client.perform_request('GET', 'not_an_index') } + .to raise_error do |error| + expect(error.class).to eq(Elasticsearch::Transport::Transport::Errors::NotFound) + end + expect(faulty.circuit('elasticsearch').status.failure_rate).to eq(0) + end end diff --git a/spec/patch_spec.rb b/spec/patch_spec.rb index bbf51dd..d530b19 100644 --- a/spec/patch_spec.rb +++ b/spec/patch_spec.rb @@ -8,7 +8,7 @@ describe '.circuit_from_hash' do let(:faulty) { Faulty.new(listeners: []) } - let(:error_module) do + let(:error_mapper) do stub_const('TestErrors', Module.new) described_class.define_circuit_errors(TestErrors, error_base) TestErrors @@ -62,33 +62,33 @@ end context 'when patch_errors is enabled' do - it 'sets error_module' do + it 'sets error_mapper' do circuit = described_class.circuit_from_hash( 'test', { instance: faulty }, - patched_error_module: error_module + patched_error_mapper: error_mapper ) - expect(circuit.options.error_module).to eq(error_module) + expect(circuit.options.error_mapper).to eq(error_mapper) end end - context 'when patch_errors is enabled but patched_error_module is missing' do + context 'when patch_errors is enabled but patched_error_mapper is missing' do it 'uses Faulty error module' do circuit = described_class.circuit_from_hash( 'test', { instance: faulty, patch_errors: true } ) - expect(circuit.options.error_module).to eq(Faulty) + expect(circuit.options.error_mapper).to eq(Faulty) end end - context 'when user sets error_module manually' do - it 'overrides patched_error_module' do + context 'when user sets error_mapper manually' do + it 'overrides patched_error_mapper' do circuit = described_class.circuit_from_hash( 'test', - { instance: faulty, error_module: Faulty } + { instance: faulty, error_mapper: Faulty } ) - expect(circuit.options.error_module).to eq(Faulty) + expect(circuit.options.error_mapper).to eq(Faulty) end end @@ -98,7 +98,7 @@ 'test', { instance: faulty, patch_errors: false } ) - expect(circuit.options.error_module).to eq(Faulty) + expect(circuit.options.error_mapper).to eq(Faulty) end end