From 266bdb8920478c69dfdbb0c15b1137b2a6a68af2 Mon Sep 17 00:00:00 2001
From: patrick brisbin <pbrisbin@gmail.com>
Date: Mon, 10 Mar 2014 17:41:37 -0400
Subject: [PATCH 1/4] Refactor retries, metrics, and error handling

Move to a Middelware pattern to ease the introduction of new handlers
later (i.e. for HTTP response checking, etc).

Service.receive was removed. Now, Service::Invocation is the main
interface to outside callers, they should build and pass the service
instance themselves.
---
 lib/cc/service.rb            |   9 ---
 lib/cc/service/invocation.rb | 127 +++++++++++++++++++++++++----------
 test/invocation_test.rb      |  76 +++++++++++++++------
 test/service_test.rb         |  26 -------
 4 files changed, 148 insertions(+), 90 deletions(-)

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/invocation.rb b/lib/cc/service/invocation.rb
index d8f26a3..2fd1b0d 100644
--- a/lib/cc/service/invocation.rb
+++ b/lib/cc/service/invocation.rb
@@ -1,57 +1,116 @@
+# 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.
+#
 class CC::Service::Invocation
-  RETRIES = 3
+  class InvocationChain
+    def initialize(&block)
+      @invocation = block
+    end
+
+    def wrap(klass, *args)
+      @invocation = klass.new(@invocation, *args)
+    end
 
-  def initialize(service, statsd = nil, logger = nil)
-    @service = service
-    @statsd = statsd || NullObject.new
-    @logger = logger || NullObject.new
+    def call
+      @invocation.call
+    end
   end
 
-  def invoke
-    safely { service.receive }
+  class WithRetries
+    def initialize(invocation, retries)
+      @invocation = invocation
+      @retries = retries
+    end
+
+    def call
+      @invocation.call
+    rescue => ex
+      raise ex if @retries.zero?
 
-    statsd.increment(success_stat)
+      @retries -= 1
+      retry
+    end
   end
 
-  private
+  class WithMetrics
+    def initialize(invocation, statsd, prefix = nil)
+      @invocation = invocation
+      @statsd = statsd
+      @prefix = prefix
+    end
 
-  attr_reader :service, :statsd, :logger
+    def call
+      @invocation.call
+      @statsd.increment(success_key)
+    rescue => ex
+      @statsd.increment(error_key(ex))
+      raise ex
+    end
 
-  def safely(&block)
-    with_retries(RETRIES, &block)
-  rescue => ex
-    statsd.increment(error_stat(ex))
-    logger.error(error_message(ex))
+    def success_key
+      ["services.invocations", @prefix].compact.join('.')
+    end
+
+    def error_key(ex)
+      ["services.errors", @prefix, "#{ex.class.name.underscore}"].compact.join('.')
+    end
   end
 
-  def with_retries(retries, &block)
-    yield
+  class WithErrorHandling
+    def initialize(invocation, logger, prefix = nil)
+      @invocation = invocation
+      @logger = logger
+      @prefix = prefix
+    end
 
-  rescue => ex
-    raise ex if retries.zero?
+    def call
+      @invocation.call
+    rescue => ex
+      @logger.error(error_message(ex))
+    end
 
-    retries -= 1
-    retry
-  end
+    private
 
-  def success_stat
-    "services.invocations.#{slug}"
+    def error_message(ex)
+      message  = "Exception invoking service:"
+      message << " [#{@prefix}]" if @prefix
+      message << " (#{ex.class}) #{ex.message}"
+    end
   end
 
-  def error_stat(ex)
-    "services.errors.#{slug}.#{ex.class.name.underscore}"
-  end
+  MIDDLEWARE = {
+    retries: WithRetries,
+    metrics: WithMetrics,
+    error_handling: WithErrorHandling,
+  }
 
-  def error_message(ex)
-    "Exception invoking #{slug} service: (#{ex.class}) #{ex.message}"
-  end
+  def initialize(service)
+    @chain = InvocationChain.new { service.receive }
+
+    yield(self) if block_given?
 
-  def slug
-    service.class.slug
+    @chain.call
   end
 
-  class NullObject
-    def method_missing(*)
+  def with(middleware, *args)
+    if klass = MIDDLEWARE[middleware]
+      @chain.wrap(klass, *args)
     end
   end
 end
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

From 8ecb08080335f157c950e93efecfbc4aca8a92ed Mon Sep 17 00:00:00 2001
From: patrick brisbin <pbrisbin@gmail.com>
Date: Mon, 10 Mar 2014 17:47:20 -0400
Subject: [PATCH 2/4] Move new invocation classes to separate files

---
 lib/cc/service/invocation.rb                  | 118 ++++--------------
 lib/cc/service/invocation/invocation_chain.rb |  15 +++
 .../service/invocation/with_error_handling.rb |  23 ++++
 lib/cc/service/invocation/with_metrics.rb     |  25 ++++
 lib/cc/service/invocation/with_retries.rb     |  17 +++
 5 files changed, 103 insertions(+), 95 deletions(-)
 create mode 100644 lib/cc/service/invocation/invocation_chain.rb
 create mode 100644 lib/cc/service/invocation/with_error_handling.rb
 create mode 100644 lib/cc/service/invocation/with_metrics.rb
 create mode 100644 lib/cc/service/invocation/with_retries.rb

diff --git a/lib/cc/service/invocation.rb b/lib/cc/service/invocation.rb
index 2fd1b0d..a482f03 100644
--- a/lib/cc/service/invocation.rb
+++ b/lib/cc/service/invocation.rb
@@ -1,105 +1,33 @@
-# 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.
-#
-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
-
-  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
-
-  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
-
-  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
+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'
 
+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 }
 
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

From 7595961568ceadd027a83ecf5231871b7a67910f Mon Sep 17 00:00:00 2001
From: patrick brisbin <pbrisbin@gmail.com>
Date: Mon, 10 Mar 2014 18:12:08 -0400
Subject: [PATCH 3/4] Leverage middleware pattern in test script

---
 lib/cc/service/invocation.rb |  6 +++++-
 service_test.rb              | 16 +++++++++++++++-
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/lib/cc/service/invocation.rb b/lib/cc/service/invocation.rb
index a482f03..7c724bc 100644
--- a/lib/cc/service/invocation.rb
+++ b/lib/cc/service/invocation.rb
@@ -38,7 +38,11 @@ def initialize(service)
 
   def with(middleware, *args)
     if klass = MIDDLEWARE[middleware]
-      @chain.wrap(klass, *args)
+      wrap(klass, *args)
     end
   end
+
+  def wrap(klass, *args)
+    @chain.wrap(klass, *args)
+  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"]

From 601017a970c0789e31d110c9fce7dcc010065317 Mon Sep 17 00:00:00 2001
From: patrick brisbin <pbrisbin@gmail.com>
Date: Mon, 10 Mar 2014 18:12:33 -0400
Subject: [PATCH 4/4] Tell Faraday to raise errors on non-200

---
 lib/cc/service/http.rb | 1 +
 1 file changed, 1 insertion(+)

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