Merge pull request #13 from codeclimate/pb-check-http-responses

Middleware pattern, tell Faraday to raise

Author: brynary

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 ||= []

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

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

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

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

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

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

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"]

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

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