[QENG-4075] Refactor statsd methods / classes

Prior to this we could easily run into situations where `statds_prefix` would
be `nil` (and possibly the `statsd` handle itself). There was some significant
complexity and brittleness in how statsd was set up.

Refactored so that:

 - `statsd_prefix` is no longer exposed to any callers of statsd methods
 - there is now a `Vmpooler::DummyStatsd` class which can be returned when we are not actually going to publish stats, but would like to keep the calling interface consistent
 - setup of the statsd handle is via just passing in `config[:statsd]`, if `nil`, this will result in a dummy handle being return
 - defaulting of `server` values was fixed -- this did not actually work in the previous implementation. `config[:statsd][:server]` is now required.
 - tests use a `DummyStatsd` instance instead of an rspec double.
 - calls to `statsd.increment` were taking incorrect arguments (some our fault, some part of the prior implementation), and were not collecting data on which pools were "invalid" or "empty". Fixed this and are now explicitly tracking the invalid/empty pool names.

lib/vmpooler.rb

@@ -92,12 +92,9 @@ module Vmpooler
     end
   end
 
-  def self.new_statsd(server, port)
+  def self.new_statsd(params)
-    if server.nil? || server.empty?
+    return DummyStatsd.new unless params
-      nil
+    Statsd.new params
-    else
-      Statsd.new server, port
-    end
   end
 
   def self.pools(conf)

lib/vmpooler/api/v1.rb

@@ -16,12 +16,6 @@ module Vmpooler
       Vmpooler::API.settings.statsd
     end
 
-    def statsd_prefix
-      if Vmpooler::API.settings.statsd
-        Vmpooler::API.settings.config[:statsd]['prefix'] ? Vmpooler::API.settings.config[:statsd]['prefix'] : 'vmpooler'
-      end
-    end
-
     def config
       Vmpooler::API.settings.config[:config]
     end
@@ -98,11 +92,11 @@ module Vmpooler
           vm, name = fetch_single_vm(requested)
           if !vm
             failed = true
-            statsd.increment(statsd_prefix + '.checkout.empty.' + requested, 1)
+            statsd.increment('checkout.empty.' + requested)
             break
           else
             vms << [ name, vm ]
-            statsd.increment(statsd_prefix + '.checkout.success.' + name, 1)
+            statsd.increment('checkout.success.' + name)
           end
         end
       end
@@ -388,7 +382,7 @@ module Vmpooler
           result = atomically_allocate_vms(payload)
         else
           invalid.each do |bad_template|
-            statsd.increment(statsd_prefix + '.checkout.invalid', bad_template)
+            statsd.increment('checkout.invalid.' + bad_template)
           end
           status 404
         end
@@ -430,7 +424,7 @@ module Vmpooler
           result = atomically_allocate_vms(payload)
         else
           invalid.each do |bad_template|
-            statsd.increment(statsd_prefix + '.checkout.invalid', bad_template)
+            statsd.increment('checkout.invalid.' + bad_template)
           end
           status 404
         end

lib/vmpooler/statsd.rb

@@ -2,8 +2,48 @@ require 'rubygems' unless defined?(Gem)
 
 module Vmpooler
   class Statsd
-    def initialize(server = 'statsd', port = 8125)
+    attr_reader :server, :port, :prefix
-      @server = Statsd.new(server, port)
+
+    def initialize(params = {})
+      if params[:server].nil? || params[:server].empty?
+        raise ArgumentError, "Statsd server is required. Config: #{params.inspect}"
+      end
+
+      host    = params[:server]
+      port    = params[:port]   || 8125
+      @prefix = params[:prefix] || 'vmpooler'
+      @server = Statsd.new(host, port)
+    end
+
+    def increment(label)
+      server.increment(prefix + "." + label)
+    end
+
+    def gauge(label, value)
+      server.gauge(prefix + "." + label, value)
+    end
+
+    def timing(label, duration)
+      server.timing(prefix + "." + label, duration)
+    end
+  end
+
+  class DummyStatsd
+    attr_reader :server, :port, :prefix
+
+    def initialize(params = {})
+    end
+
+    def increment(label)
+      true
+    end
+
+    def gauge(label, value)
+      true
+    end
+
+    def timing(label, duration)
+      true
     end
   end
 end

spec/spec_helper.rb

@@ -7,3 +7,4 @@ require 'rbvmomi'
 require 'rspec'
 require 'vmpooler'
 require 'redis'
+require 'vmpooler/statsd'

spec/vmpooler/api/v1/vm_spec.rb

@@ -20,7 +20,7 @@ describe Vmpooler::API::V1 do
 
   describe '/vm' do
     let(:prefix) { '/api/v1' }
-    let(:statsd) { double('stats', :increment => true) }
+    let(:statsd) { Vmpooler::DummyStatsd.new }
     let(:config) {
       {
         config: {

spec/vmpooler/api/v1/vm_template_spec.rb

@@ -20,7 +20,7 @@ describe Vmpooler::API::V1 do
 
   describe '/vm/:template' do
     let(:prefix) { '/api/v1' }
-    let(:statsd) { double('stats', :increment => true) }
+    let(:statsd) { Vmpooler::DummyStatsd.new }
     let(:config) {
       {
         config: {

vmpooler

@@ -9,19 +9,10 @@ config = Vmpooler.config
 redis_host = config[:redis]['server']
 logger_file = config[:config]['logfile']
 graphite = config[:graphite]['server'] ? config[:graphite]['server'] : nil
-# statsd is an addition and my not be present in YAML configuration
-if config[:statsd]
-  statsd = config[:statsd]['server'] ? config[:statsd]['server'] : nil
-  statsd_port = config[:statsd]['port'] ? config[:statsd]['port'] : 8125
-end
 
 api = Thread.new {
   thr = Vmpooler::API.new
-  if statsd
+  thr.helpers.configure(config, Vmpooler.new_redis(redis_host), Vmpooler.new_statsd(config[:statsd]))
-    thr.helpers.configure(config, Vmpooler.new_redis(redis_host), Vmpooler.new_statsd(statsd, statsd_port))
-  else
-    thr.helpers.configure(config, Vmpooler.new_redis(redis_host), statsd=nil)
-  end
   thr.helpers.execute!
 }
 
@@ -31,9 +22,8 @@ manager = Thread.new {
     Vmpooler.new_logger(logger_file),
     Vmpooler.new_redis(redis_host),
     Vmpooler.new_graphite(graphite),
-    Vmpooler.new_statsd(statsd, statsd_port)
+    Vmpooler.new_statsd(config[:statsd])
   ).execute!
 }
 
 [api, manager].each { |t| t.join }
-