diff --git a/lib/cc/service.rb b/lib/cc/service.rb index 2b196f8..5caec30 100644 --- a/lib/cc/service.rb +++ b/lib/cc/service.rb @@ -32,15 +32,6 @@ def self.load_services ALL_EVENTS = %w[test unit coverage quality vulnerability] - def self.receive(config, payload, invocation_class = Invocation) - statsd = config.delete(:statsd) - logger = config.delete(:logger) - service = new(config, payload) - - invocation = invocation_class.new(service, statsd, logger) - invocation.invoke - end - # Tracks the defined services. def self.services @services ||= [] diff --git a/lib/cc/service/http.rb b/lib/cc/service/http.rb index f7cfb23..887a176 100644 --- a/lib/cc/service/http.rb +++ b/lib/cc/service/http.rb @@ -53,6 +53,7 @@ def http(options = {}) Faraday.new(options) do |b| b.request(:url_encoded) + b.response(:raise_error) b.adapter(*Array(options[:adapter] || config[:adapter])) end end diff --git a/lib/cc/service/invocation.rb b/lib/cc/service/invocation.rb index d8f26a3..7c724bc 100644 --- a/lib/cc/service/invocation.rb +++ b/lib/cc/service/invocation.rb @@ -1,57 +1,48 @@ -class CC::Service::Invocation - RETRIES = 3 - - def initialize(service, statsd = nil, logger = nil) - @service = service - @statsd = statsd || NullObject.new - @logger = logger || NullObject.new - end - - def invoke - safely { service.receive } - - statsd.increment(success_stat) - end - - private - - attr_reader :service, :statsd, :logger - - def safely(&block) - with_retries(RETRIES, &block) - rescue => ex - statsd.increment(error_stat(ex)) - logger.error(error_message(ex)) - end - - def with_retries(retries, &block) - yield - - rescue => ex - raise ex if retries.zero? +require 'cc/service/invocation/invocation_chain' +require 'cc/service/invocation/with_retries' +require 'cc/service/invocation/with_metrics' +require 'cc/service/invocation/with_error_handling' - retries -= 1 - retry - end - - def success_stat - "services.invocations.#{slug}" - end - - def error_stat(ex) - "services.errors.#{slug}.#{ex.class.name.underscore}" - end - - def error_message(ex) - "Exception invoking #{slug} service: (#{ex.class}) #{ex.message}" - end - - def slug - service.class.slug +class CC::Service::Invocation + MIDDLEWARE = { + retries: WithRetries, + metrics: WithMetrics, + error_handling: WithErrorHandling, + } + + # Build a chain of invocation wrappers which eventually calls receive + # on the given service, then execute that chain. + # + # Order is important. Each call to #with, wraps the last. + # + # Usage: + # + # CC::Service::Invocation.new(service) do |i| + # i.with :retries, 3 + # i.with :metrics, $statsd + # i.with :error_handling, Rails.logger + # end + # + # In the above example, service.receive could happen 4 times (once, + # then three retries) before an exception is re-raised up to the + # metrics collector, then up again to the error handling. If the order + # were reversed, the error handling middleware would prevent the other + # middleware from seeing any exceptions at all. + def initialize(service) + @chain = InvocationChain.new { service.receive } + + yield(self) if block_given? + + @chain.call + end + + def with(middleware, *args) + if klass = MIDDLEWARE[middleware] + wrap(klass, *args) + end end - class NullObject - def method_missing(*) - end + def wrap(klass, *args) + @chain.wrap(klass, *args) end end diff --git a/lib/cc/service/invocation/invocation_chain.rb b/lib/cc/service/invocation/invocation_chain.rb new file mode 100644 index 0000000..b89a7f3 --- /dev/null +++ b/lib/cc/service/invocation/invocation_chain.rb @@ -0,0 +1,15 @@ +class CC::Service::Invocation + class InvocationChain + def initialize(&block) + @invocation = block + end + + def wrap(klass, *args) + @invocation = klass.new(@invocation, *args) + end + + def call + @invocation.call + end + end +end diff --git a/lib/cc/service/invocation/with_error_handling.rb b/lib/cc/service/invocation/with_error_handling.rb new file mode 100644 index 0000000..dd05546 --- /dev/null +++ b/lib/cc/service/invocation/with_error_handling.rb @@ -0,0 +1,23 @@ +class CC::Service::Invocation + class WithErrorHandling + def initialize(invocation, logger, prefix = nil) + @invocation = invocation + @logger = logger + @prefix = prefix + end + + def call + @invocation.call + rescue => ex + @logger.error(error_message(ex)) + end + + private + + def error_message(ex) + message = "Exception invoking service:" + message << " [#{@prefix}]" if @prefix + message << " (#{ex.class}) #{ex.message}" + end + end +end diff --git a/lib/cc/service/invocation/with_metrics.rb b/lib/cc/service/invocation/with_metrics.rb new file mode 100644 index 0000000..f2a4f6b --- /dev/null +++ b/lib/cc/service/invocation/with_metrics.rb @@ -0,0 +1,25 @@ +class CC::Service::Invocation + class WithMetrics + def initialize(invocation, statsd, prefix = nil) + @invocation = invocation + @statsd = statsd + @prefix = prefix + end + + def call + @invocation.call + @statsd.increment(success_key) + rescue => ex + @statsd.increment(error_key(ex)) + raise ex + end + + def success_key + ["services.invocations", @prefix].compact.join('.') + end + + def error_key(ex) + ["services.errors", @prefix, "#{ex.class.name.underscore}"].compact.join('.') + end + end +end diff --git a/lib/cc/service/invocation/with_retries.rb b/lib/cc/service/invocation/with_retries.rb new file mode 100644 index 0000000..1627c77 --- /dev/null +++ b/lib/cc/service/invocation/with_retries.rb @@ -0,0 +1,17 @@ +class CC::Service::Invocation + class WithRetries + def initialize(invocation, retries) + @invocation = invocation + @retries = retries + end + + def call + @invocation.call + rescue => ex + raise ex if @retries.zero? + + @retries -= 1 + retry + end + end +end diff --git a/service_test.rb b/service_test.rb index 5ed75a4..d3bc955 100644 --- a/service_test.rb +++ b/service_test.rb @@ -20,10 +20,24 @@ require 'cc/services' CC::Service.load_services +class WithResponseLogging + def initialize(invocation) + @invocation = invocation + end + + def call + @invocation.call.tap { |r| p r } + end +end + def test_service(klass, config) repo_name = ENV["REPO_NAME"] || "App" - klass.receive(config, name: :test, repo_name: repo_name) + service = klass.new(config, name: :test, repo_name: repo_name) + + CC::Service::Invocation.new(service) do |i| + i.wrap(WithResponseLogging) + end end if webhook_url = ENV["SLACK_WEBHOOK_URL"] diff --git a/test/invocation_test.rb b/test/invocation_test.rb index 6cf59a1..1d89bc7 100644 --- a/test/invocation_test.rb +++ b/test/invocation_test.rb @@ -4,45 +4,83 @@ class TestInvocation < Test::Unit::TestCase def test_success service = FakeService.new - CC::Service::Invocation.new(service).invoke + CC::Service::Invocation.new(service) assert_equal 1, service.receive_count end - def test_failure + def test_retries service = FakeService.new service.raise_on_receive = true + error_occurred = false - CC::Service::Invocation.new(service).invoke + begin + CC::Service::Invocation.new(service) do |i| + i.with :retries, 3 + end + rescue + error_occurred = true + end - # First call + N retries - expected_count = 1 + CC::Service::Invocation::RETRIES - assert_equal expected_count, service.receive_count + assert error_occurred + assert_equal 1 + 3, service.receive_count end - def test_success_metrics + def test_metrics statsd = FakeStatsd.new - logger = FakeLogger.new - service = FakeService.new - CC::Service::Invocation.new(service, statsd, logger).invoke + CC::Service::Invocation.new(FakeService.new) do |i| + i.with :metrics, statsd, "a_prefix" + end assert_equal 1, statsd.incremented_keys.length - assert_match /services\.invocations/, statsd.incremented_keys.first - assert_empty logger.logged_errors + assert_equal "services.invocations.a_prefix", statsd.incremented_keys.first end - def test_failure_metrics + def test_metrics_on_errors statsd = FakeStatsd.new + service = FakeService.new + service.raise_on_receive = true + error_occurred = false + + begin + CC::Service::Invocation.new(service) do |i| + i.with :metrics, statsd, "a_prefix" + end + rescue + error_occurred = true + end + + assert error_occurred + assert_equal 1, statsd.incremented_keys.length + assert_match /^services\.errors\.a_prefix/, statsd.incremented_keys.first + end + + def test_error_handling logger = FakeLogger.new service = FakeService.new service.raise_on_receive = true - CC::Service::Invocation.new(service, statsd, logger).invoke + CC::Service::Invocation.new(service) do |i| + i.with :error_handling, logger, "a_prefix" + end + + assert_equal 1, logger.logged_errors.length + assert_match /^Exception invoking service: \[a_prefix\]/, logger.logged_errors.first + end + + def test_multiple_middleware + service = FakeService.new + service.raise_on_receive = true + logger = FakeLogger.new + + CC::Service::Invocation.new(service) do |i| + i.with :retries, 3 + i.with :error_handling, logger + end - refute_empty statsd.incremented_keys - refute_empty logger.logged_errors - assert_match /services\.errors/, statsd.incremented_keys.first + assert_equal 1 + 3, service.receive_count + assert_equal 1, logger.logged_errors.length end private @@ -51,10 +89,6 @@ class FakeService attr_reader :receive_count attr_accessor :raise_on_receive - def self.slug - "fake-service" - end - def initialize @receive_count = 0 end diff --git a/test/service_test.rb b/test/service_test.rb index a0ebb04..6f1a44f 100644 --- a/test/service_test.rb +++ b/test/service_test.rb @@ -6,30 +6,4 @@ def test_validates_events CC::Service.new(:foo, {}, {}) end end - - def test_receive - ret = CC::Service.receive( - { statsd: "statsd", logger: "logger" }, - { name: :test, foo: "bar" }, - FakeInvocation - ) - - assert ret[:invoked] - assert ret[:service].is_a?(CC::Service) - assert_equal "test", ret[:service].event - assert_equal "bar", ret[:service].payload["foo"] - assert_equal "statsd", ret[:statsd] - assert_equal "logger", ret[:logger] - end - - FakeInvocation = Struct.new(:service, :statsd, :logger) do - def invoke - { - invoked: true, - service: service, - statsd: statsd, - logger: logger - } - end - end end