From 984ed4a927f5d8756fe9cdf32c6fe056c6f3a4ec Mon Sep 17 00:00:00 2001
From: John Nunemaker <nunemaker@gmail.com>
Date: Tue, 5 Mar 2013 14:50:59 -0500
Subject: [PATCH] Added statsd instrumentation support.

---
 Gemfile                                       |   1 +
 .../instrumentation/metriks_subscriber.rb     |  84 ++-------------
 lib/flipper/instrumentation/statsd.rb         |   6 ++
 .../instrumentation/statsd_subscriber.rb      |  28 +++++
 lib/flipper/instrumentation/subscriber.rb     | 102 ++++++++++++++++++
 .../metriks_subscriber_spec.rb                |   7 --
 .../instrumentation/statsd_subscriber_spec.rb |  76 +++++++++++++
 spec/helper.rb                                |   2 +
 spec/support/fake_udp_socket.rb               |  27 +++++
 9 files changed, 248 insertions(+), 85 deletions(-)
 create mode 100644 lib/flipper/instrumentation/statsd.rb
 create mode 100644 lib/flipper/instrumentation/statsd_subscriber.rb
 create mode 100644 lib/flipper/instrumentation/subscriber.rb
 create mode 100644 spec/flipper/instrumentation/statsd_subscriber_spec.rb
 create mode 100644 spec/support/fake_udp_socket.rb

diff --git a/Gemfile b/Gemfile
index d95d742e7..8bbaba866 100644
--- a/Gemfile
+++ b/Gemfile
@@ -3,6 +3,7 @@ gemspec
 
 gem 'rake'
 gem 'metriks', :require => false
+gem 'statsd-ruby', :require => false
 gem 'rspec'
 gem 'rack-test'
 gem 'activesupport', :require => false
diff --git a/lib/flipper/instrumentation/metriks_subscriber.rb b/lib/flipper/instrumentation/metriks_subscriber.rb
index 75fda45d3..5f4826fb0 100644
--- a/lib/flipper/instrumentation/metriks_subscriber.rb
+++ b/lib/flipper/instrumentation/metriks_subscriber.rb
@@ -2,90 +2,18 @@
 # ActiveSupport::Notifications. Instead, you should require the metriks file
 # that lives in the same directory as this file. The benefit is that it
 # subscribes to the correct events and does everything for your.
+require 'flipper/instrumentation/subscriber'
 require 'metriks'
 
 module Flipper
   module Instrumentation
-    class MetriksSubscriber
-      # Public: Use this as the subscribed block.
-      def self.call(name, start, ending, transaction_id, payload)
-        new(name, start, ending, transaction_id, payload).update
+    class MetriksSubscriber < Subscriber
+      def update_timer(metric)
+        Metriks.timer(metric).update(@duration)
       end
 
-      # Private: Initializes a new event processing instance.
-      def initialize(name, start, ending, transaction_id, payload)
-        @name = name
-        @start = start
-        @ending = ending
-        @payload = payload
-        @duration = ending - start
-        @transaction_id = transaction_id
-      end
-
-      def update
-        operation_type = @name.split('.').first
-        method_name = "update_#{operation_type}_metrics"
-
-        if respond_to?(method_name)
-          send(method_name)
-        else
-          puts "Could not update #{operation_type} metrics as MetriksSubscriber did not respond to `#{method_name}`"
-        end
-      end
-
-      def update_feature_operation_metrics
-        feature_name = @payload[:feature_name]
-        gate_name = @payload[:gate_name]
-        operation = strip_trailing_question_mark(@payload[:operation])
-        result = @payload[:result]
-        thing = @payload[:thing]
-
-        Metriks.timer("flipper.feature_operation.#{operation}").update(@duration)
-
-        if @payload[:operation] == :enabled?
-          metric_name = if result
-            "flipper.feature.#{feature_name}.enabled"
-          else
-            "flipper.feature.#{feature_name}.disabled"
-          end
-
-          Metriks.meter(metric_name).mark
-        end
-      end
-
-      def update_adapter_operation_metrics
-        adapter_name = @payload[:adapter_name]
-        operation = @payload[:operation]
-        result = @payload[:result]
-        value = @payload[:value]
-        key = @payload[:key]
-
-        Metriks.timer("flipper.adapter.#{adapter_name}.#{operation}").update(@duration)
-      end
-
-      def update_gate_operation_metrics
-        feature_name = @payload[:feature_name]
-        gate_name = @payload[:gate_name]
-        operation = strip_trailing_question_mark(@payload[:operation])
-        result = @payload[:result]
-        thing = @payload[:thing]
-
-        Metriks.timer("flipper.gate_operation.#{gate_name}.#{operation}").update(@duration)
-        Metriks.timer("flipper.feature.#{feature_name}.gate_operation.#{gate_name}.#{operation}").update(@duration)
-
-        if @payload[:operation] == :open?
-          metric_name = if result
-            "flipper.feature.#{feature_name}.gate.#{gate_name}.open"
-          else
-            "flipper.feature.#{feature_name}.gate.#{gate_name}.closed"
-          end
-
-          Metriks.meter(metric_name).mark
-        end
-      end
-
-      def strip_trailing_question_mark(operation)
-        operation.to_s.gsub(/\?$/, '')
+      def update_counter(metric)
+        Metriks.meter(metric).mark
       end
     end
   end
diff --git a/lib/flipper/instrumentation/statsd.rb b/lib/flipper/instrumentation/statsd.rb
new file mode 100644
index 000000000..899933947
--- /dev/null
+++ b/lib/flipper/instrumentation/statsd.rb
@@ -0,0 +1,6 @@
+require 'securerandom'
+require 'active_support/notifications'
+require 'flipper/instrumentation/statsd_subscriber'
+
+ActiveSupport::Notifications.subscribe /\.flipper$/,
+  Flipper::Instrumentation::StatsdSubscriber
diff --git a/lib/flipper/instrumentation/statsd_subscriber.rb b/lib/flipper/instrumentation/statsd_subscriber.rb
new file mode 100644
index 000000000..bd2890a3c
--- /dev/null
+++ b/lib/flipper/instrumentation/statsd_subscriber.rb
@@ -0,0 +1,28 @@
+# Note: You should never need to require this file directly if you are using
+# ActiveSupport::Notifications. Instead, you should require the metriks file
+# that lives in the same directory as this file. The benefit is that it
+# subscribes to the correct events and does everything for your.
+require 'flipper/instrumentation/subscriber'
+require 'statsd'
+
+module Flipper
+  module Instrumentation
+    class StatsdSubscriber < Subscriber
+      class << self
+        attr_accessor :client
+      end
+
+      def update_timer(metric)
+        if self.class.client
+          self.class.client.timing metric, (@duration * 1_000).round
+        end
+      end
+
+      def update_counter(metric)
+        if self.class.client
+          self.class.client.increment metric
+        end
+      end
+    end
+  end
+end
diff --git a/lib/flipper/instrumentation/subscriber.rb b/lib/flipper/instrumentation/subscriber.rb
new file mode 100644
index 000000000..193f5c3d2
--- /dev/null
+++ b/lib/flipper/instrumentation/subscriber.rb
@@ -0,0 +1,102 @@
+module Flipper
+  module Instrumentation
+    class Subscriber
+      # Public: Use this as the subscribed block.
+      def self.call(name, start, ending, transaction_id, payload)
+        new(name, start, ending, transaction_id, payload).update
+      end
+
+      # Private: Initializes a new event processing instance.
+      def initialize(name, start, ending, transaction_id, payload)
+        @name = name
+        @start = start
+        @ending = ending
+        @payload = payload
+        @duration = ending - start
+        @transaction_id = transaction_id
+      end
+
+      # Internal: Override in subclass.
+      def update_timer(metric)
+        raise 'not implemented'
+      end
+
+      # Internal: Override in subclass.
+      def update_counter(metric)
+        raise 'not implemented'
+      end
+
+      # Private
+      def update
+        operation_type = @name.split('.').first
+        method_name = "update_#{operation_type}_metrics"
+
+        if respond_to?(method_name)
+          send(method_name)
+        else
+          puts "Could not update #{operation_type} metrics as #{self.class} did not respond to `#{method_name}`"
+        end
+      end
+
+      # Private
+      def update_feature_operation_metrics
+        feature_name = @payload[:feature_name]
+        gate_name = @payload[:gate_name]
+        operation = strip_trailing_question_mark(@payload[:operation])
+        result = @payload[:result]
+        thing = @payload[:thing]
+
+        update_timer "flipper.feature_operation.#{operation}"
+
+        if @payload[:operation] == :enabled?
+          metric_name = if result
+            "flipper.feature.#{feature_name}.enabled"
+          else
+            "flipper.feature.#{feature_name}.disabled"
+          end
+
+          update_counter metric_name
+        end
+      end
+
+      # Private
+      def update_adapter_operation_metrics
+        adapter_name = @payload[:adapter_name]
+        operation = @payload[:operation]
+        result = @payload[:result]
+        value = @payload[:value]
+        key = @payload[:key]
+
+
+        update_timer "flipper.adapter.#{adapter_name}.#{operation}"
+      end
+
+      # Private
+      def update_gate_operation_metrics
+        feature_name = @payload[:feature_name]
+        gate_name = @payload[:gate_name]
+        operation = strip_trailing_question_mark(@payload[:operation])
+        result = @payload[:result]
+        thing = @payload[:thing]
+
+        update_timer "flipper.gate_operation.#{gate_name}.#{operation}"
+        update_timer "flipper.feature.#{feature_name}.gate_operation.#{gate_name}.#{operation}"
+
+        if @payload[:operation] == :open?
+          metric_name = if result
+            "flipper.feature.#{feature_name}.gate.#{gate_name}.open"
+          else
+            "flipper.feature.#{feature_name}.gate.#{gate_name}.closed"
+          end
+
+          update_counter metric_name
+        end
+      end
+
+      # Private
+      def strip_trailing_question_mark(operation)
+        operation.to_s.gsub(/\?$/, '')
+      end
+    end
+  end
+end
diff --git a/spec/flipper/instrumentation/metriks_subscriber_spec.rb b/spec/flipper/instrumentation/metriks_subscriber_spec.rb
index af697174e..24937dff0 100644
--- a/spec/flipper/instrumentation/metriks_subscriber_spec.rb
+++ b/spec/flipper/instrumentation/metriks_subscriber_spec.rb
@@ -56,11 +56,4 @@
     Metriks.meter("flipper.feature.stats.gate.actor.open").count.should be(1)
     Metriks.meter("flipper.feature.stats.gate.boolean.closed").count.should be(1)
   end
-
-  # Helper for seeing what is in the metriks registry
-  def print_registry_names
-    Metriks::Registry.default.each do |name, metric|
-      puts name
-    end
-  end
 end
diff --git a/spec/flipper/instrumentation/statsd_subscriber_spec.rb b/spec/flipper/instrumentation/statsd_subscriber_spec.rb
new file mode 100644
index 000000000..83250b7fb
--- /dev/null
+++ b/spec/flipper/instrumentation/statsd_subscriber_spec.rb
@@ -0,0 +1,76 @@
+require 'helper'
+require 'flipper/adapters/memory'
+require 'flipper/instrumentation/statsd'
+
+describe Flipper::Instrumentation::StatsdSubscriber do
+  let(:statsd_client) { Statsd.new }
+  let(:socket) { FakeUDPSocket.new }
+  let(:adapter) { Flipper::Adapters::Memory.new }
+  let(:flipper) {
+    Flipper.new(adapter, :instrumenter => ActiveSupport::Notifications)
+  }
+
+  let(:user) { user = Struct.new(:flipper_id).new('1') }
+
+  before do
+    described_class.client = statsd_client
+    Thread.current[:statsd_socket] = socket
+  end
+
+  after do
+    described_class.client = nil
+    Thread.current[:statsd_socket] = nil
+  end
+
+  def assert_timer(metric)
+    regex = /#{Regexp.escape metric}\:\d+\|ms/
+    socket.buffer.detect { |op| op.first =~ regex }.should_not be_nil
+  end
+
+  def assert_counter(metric)
+    socket.buffer.detect { |op| op.first == "#{metric}:1|c" }.should_not be_nil
+  end
+
+  context "for enabled feature" do
+    it "updates feature metrics when calls happen" do
+      flipper[:stats].enable(user)
+      assert_timer 'flipper.feature_operation.enable'
+
+      flipper[:stats].enabled?(user)
+      assert_timer 'flipper.feature_operation.enabled'
+      assert_counter 'flipper.feature.stats.enabled'
+    end
+  end
+
+  context "for disabled feature" do
+    it "updates feature metrics when calls happen" do
+      flipper[:stats].disable(user)
+      assert_timer 'flipper.feature_operation.disable'
+
+      flipper[:stats].enabled?(user)
+      assert_timer 'flipper.feature_operation.enabled'
+      assert_counter 'flipper.feature.stats.disabled'
+    end
+  end
+
+  it "updates adapter metrics when calls happen" do
+    flipper[:stats].enable(user)
+    assert_timer 'flipper.adapter.memory.enable'
+
+    flipper[:stats].enabled?(user)
+    assert_timer 'flipper.adapter.memory.get'
+
+    flipper[:stats].disable(user)
+    assert_timer 'flipper.adapter.memory.disable'
+  end
+
+  it "updates gate metrics when calls happen" do
+    flipper[:stats].enable(user)
+    flipper[:stats].enabled?(user)
+
+    assert_timer 'flipper.gate_operation.boolean.open'
+    assert_timer 'flipper.feature.stats.gate_operation.boolean.open'
+    assert_counter 'flipper.feature.stats.gate.actor.open'
+    assert_counter 'flipper.feature.stats.gate.boolean.closed'
+  end
+end
diff --git a/spec/helper.rb b/spec/helper.rb
index 5a86c4163..87cd3e1ee 100644
--- a/spec/helper.rb
+++ b/spec/helper.rb
@@ -15,6 +15,8 @@
 
 require 'flipper'
 
+Dir[root_path.join("spec/support/**/*.rb")].each { |f| require f }
+
 RSpec.configure do |config|
   config.fail_fast = true
 
diff --git a/spec/support/fake_udp_socket.rb b/spec/support/fake_udp_socket.rb
new file mode 100644
index 000000000..ad06797f2
--- /dev/null
+++ b/spec/support/fake_udp_socket.rb
@@ -0,0 +1,27 @@
+class FakeUDPSocket
+  attr_reader :buffer
+
+  def initialize
+    @buffer = []
+  end
+
+  def send(message, *rest)
+    @buffer.push [message]
+  end
+
+  def recv
+    @buffer.shift
+  end
+
+  def clear
+    @buffer = []
+  end
+
+  def to_s
+    inspect
+  end
+
+  def inspect
+    "<FakeUDPSocket: #{@buffer.inspect}>"
+  end
+end