From c71b2cd003c152a7bcc465f6818746b4aac5f1a1 Mon Sep 17 00:00:00 2001
From: Rick Bradley <rick@rickbradley.com>
Date: Mon, 11 Jul 2016 15:20:43 -0500
Subject: [PATCH 01/21] Revert "Revert "Merge pull request #155 from
 shermdog/RE-7014-cinext""

This reverts commit 0fd6fff934f554706cabecf2354cee0457a03173.
---
 Gemfile                            |  2 +
 lib/vmpooler.rb                    | 16 ++++++
 lib/vmpooler/api.rb                |  3 +-
 lib/vmpooler/api/v1.rb             | 52 ++++++++++++++-----
 lib/vmpooler/pool_manager.rb       | 17 ++++--
 lib/vmpooler/statsd.rb             |  9 ++++
 spec/spec_helper.rb                |  4 ++
 spec/vmpooler/pool_manager_spec.rb | 83 +++++++++++++++++++++++++++++-
 vmpooler                           | 14 ++++-
 vmpooler.yaml.example              | 31 ++++++++++-
 10 files changed, 207 insertions(+), 24 deletions(-)
 create mode 100644 lib/vmpooler/statsd.rb

diff --git a/Gemfile b/Gemfile
index 4ccc870..fb32937 100644
--- a/Gemfile
+++ b/Gemfile
@@ -12,10 +12,12 @@ gem 'rbvmomi', '>= 1.8'
 gem 'redis', '>= 3.2'
 gem 'sinatra', '>= 1.4'
 gem 'net-ldap', '<= 0.12.1' # keep compatibility w/ jruby & mri-1.9.3
+gem 'statsd-ruby', '>= 1.3.0'
 
 # Test deps
 group :test do
   gem 'rack-test', '>= 0.6'
   gem 'rspec', '>= 3.2'
+  gem 'simplecov', '>= 0.11.2'
   gem 'yarjuf', '>= 2.0'
 end
diff --git a/lib/vmpooler.rb b/lib/vmpooler.rb
index 7cfacbb..5d07036 100644
--- a/lib/vmpooler.rb
+++ b/lib/vmpooler.rb
@@ -7,6 +7,7 @@ module Vmpooler
   require 'rbvmomi'
   require 'redis'
   require 'sinatra/base'
+  require "statsd-ruby"
   require 'time'
   require 'timeout'
   require 'yaml'
@@ -57,6 +58,13 @@ module Vmpooler
       parsed_config[:graphite]['prefix'] ||= 'vmpooler'
     end
 
+    # statsd is an addition and my not be present in YAML configuration
+    if parsed_config[:statsd]
+      if parsed_config[:statsd]['server']
+        parsed_config[:statsd]['prefix'] ||= 'vmpooler'
+      end
+    end
+
     if parsed_config[:tagfilter]
       parsed_config[:tagfilter].keys.each do |tag|
         parsed_config[:tagfilter][tag] = Regexp.new(parsed_config[:tagfilter][tag])
@@ -84,6 +92,14 @@ module Vmpooler
     end
   end
 
+  def self.new_statsd(server, port)
+    if server.nil? || server.empty?
+      nil
+    else
+      Statsd.new server, port
+    end
+  end
+
   def self.pools(conf)
     conf[:pools]
   end
diff --git a/lib/vmpooler/api.rb b/lib/vmpooler/api.rb
index ee51cbc..45a9bfa 100644
--- a/lib/vmpooler/api.rb
+++ b/lib/vmpooler/api.rb
@@ -42,9 +42,10 @@ module Vmpooler
     use Vmpooler::API::Reroute
     use Vmpooler::API::V1
 
-    def configure(config, redis, environment = :production)
+    def configure(config, redis, statsd, environment = :production)
       self.settings.set :config, config
       self.settings.set :redis, redis
+      self.settings.set :statsd, statsd
       self.settings.set :environment, environment
     end
 
diff --git a/lib/vmpooler/api/v1.rb b/lib/vmpooler/api/v1.rb
index f9f29b3..2d76970 100644
--- a/lib/vmpooler/api/v1.rb
+++ b/lib/vmpooler/api/v1.rb
@@ -12,6 +12,16 @@ module Vmpooler
       Vmpooler::API.settings.redis
     end
 
+    def statsd
+      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
@@ -34,13 +44,11 @@ module Vmpooler
 
     def fetch_single_vm(template)
       vm = backend.spop('vmpooler__ready__' + template)
-
       return [vm, template] if vm
 
       aliases = Vmpooler::API.settings.config[:alias]
       if aliases && aliased_template = aliases[template]
         vm = backend.spop('vmpooler__ready__' + aliased_template)
-
         return [vm, aliased_template] if vm
       end
 
@@ -85,14 +93,16 @@ module Vmpooler
       failed = false
       vms = []
 
-      payload.each do |template, count|
+      payload.each do |requested, count|
         count.to_i.times do |_i|
-          vm, name = fetch_single_vm(template)
+          vm, name = fetch_single_vm(requested)
           if !vm
             failed = true
+            statsd.increment(statsd_prefix + '.checkout.empty.' + name, 1)
             break
           else
             vms << [ name, vm ]
+            statsd.increment(statsd_prefix + '.checkout.success.' + name, 1)
           end
         end
       end
@@ -372,8 +382,16 @@ module Vmpooler
 
       payload = JSON.parse(request.body.read)
 
-      if all_templates_valid?(payload)
-        result = atomically_allocate_vms(payload)
+      if payload
+        invalid = invalid_templates(payload)
+        if invalid.empty?
+          result = atomically_allocate_vms(payload)
+        else
+          invalid.each do |bad_template|
+            statsd.increment(statsd_prefix + '.checkout.invalid', bad_template)
+          end
+          status 404
+        end
       else
         status 404
       end
@@ -392,12 +410,12 @@ module Vmpooler
       payload
     end
 
-    def all_templates_valid?(payload)
-      return false unless payload
-
-      payload.keys.all? do |templates|
-        pool_exists?(templates)
+    def invalid_templates(payload)
+      invalid = []
+      payload.keys.each do |template|
+        invalid << template unless pool_exists?(template)
       end
+      invalid
     end
 
     post "#{api_prefix}/vm/:template/?" do
@@ -406,8 +424,16 @@ module Vmpooler
 
       payload = extract_templates_from_query_params(params[:template])
 
-      if all_templates_valid?(payload)
-        result = atomically_allocate_vms(payload)
+      if payload
+        invalid = invalid_templates(payload)
+        if invalid.empty?
+          result = atomically_allocate_vms(payload)
+        else
+          invalid.each do |bad_template|
+            statsd.increment(statsd_prefix + '.checkout.invalid', bad_template)
+          end
+          status 404
+        end
       else
         status 404
       end
diff --git a/lib/vmpooler/pool_manager.rb b/lib/vmpooler/pool_manager.rb
index 1a0454c..5f54169 100644
--- a/lib/vmpooler/pool_manager.rb
+++ b/lib/vmpooler/pool_manager.rb
@@ -1,12 +1,15 @@
 module Vmpooler
   class PoolManager
-    def initialize(config, logger, redis, graphite=nil)
+    def initialize(config, logger, redis, graphite = nil, statsd = nil)
       $config = config
 
       # Load logger library
       $logger = logger
 
-      unless graphite.nil?
+      # statsd and graphite are mutex in the context of vmpooler
+      if statsd
+        $statsd = statsd
+      elsif graphite
         $graphite = graphite
       end
 
@@ -258,7 +261,8 @@ module Vmpooler
         $redis.decr('vmpooler__tasks__clone')
 
         begin
-          $graphite.log($config[:graphite]['prefix'] + ".clone.#{vm['template']}", finish) if defined? $graphite
+          $statsd.timing($config[:statsd]['prefix'] + ".clone.#{vm['template']}", finish) if $statsd
+          $graphite.log($config[:graphite]['prefix'] + ".clone.#{vm['template']}", finish) if $graphite
         rescue
         end
       end
@@ -294,7 +298,7 @@ module Vmpooler
 
           $logger.log('s', "[-] [#{pool}] '#{vm}' destroyed in #{finish} seconds")
 
-          $graphite.log($config[:graphite]['prefix'] + ".destroy.#{pool}", finish) if defined? $graphite
+          $graphite.log($config[:graphite]['prefix'] + ".destroy.#{pool}", finish) if $graphite
         end
       end
     end
@@ -565,7 +569,10 @@ module Vmpooler
       total = $redis.scard('vmpooler__pending__' + pool['name']) + ready
 
       begin
-        if defined? $graphite
+        if $statsd
+          $statsd.gauge($config[:statsd]['prefix'] + '.ready.' + pool['name'], $redis.scard('vmpooler__ready__' + pool['name']))
+          $statsd.gauge($config[:statsd]['prefix'] + '.running.' + pool['name'], $redis.scard('vmpooler__running__' + pool['name']))
+        elsif $graphite
           $graphite.log($config[:graphite]['prefix'] + '.ready.' + pool['name'], $redis.scard('vmpooler__ready__' + pool['name']))
           $graphite.log($config[:graphite]['prefix'] + '.running.' + pool['name'], $redis.scard('vmpooler__running__' + pool['name']))
         end
diff --git a/lib/vmpooler/statsd.rb b/lib/vmpooler/statsd.rb
new file mode 100644
index 0000000..b449a59
--- /dev/null
+++ b/lib/vmpooler/statsd.rb
@@ -0,0 +1,9 @@
+require 'rubygems' unless defined?(Gem)
+
+module Vmpooler
+  class Statsd
+    def initialize(server = 'statsd', port = 8125)
+      @server = Statsd.new(server, port)
+    end
+  end
+end
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index 7589276..c401e32 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -1,3 +1,7 @@
+require 'simplecov'
+SimpleCov.start do
+  add_filter '/spec/'
+end
 require 'helpers'
 require 'rbvmomi'
 require 'rspec'
diff --git a/spec/vmpooler/pool_manager_spec.rb b/spec/vmpooler/pool_manager_spec.rb
index a402bf4..9adeab9 100644
--- a/spec/vmpooler/pool_manager_spec.rb
+++ b/spec/vmpooler/pool_manager_spec.rb
@@ -5,13 +5,12 @@ describe 'Pool Manager' do
   let(:logger) { double('logger') }
   let(:redis) { double('redis') }
   let(:config) { {} }
-  let(:graphite) { nil }
   let(:pool) { 'pool1' }
   let(:vm) { 'vm1' }
   let(:timeout) { 5 }
   let(:host) { double('host') }
 
-  subject { Vmpooler::PoolManager.new(config, logger, redis, graphite) }
+  subject { Vmpooler::PoolManager.new(config, logger, redis) }
 
   describe '#_check_pending_vm' do
     let(:pool_helper) { double('pool') }
@@ -252,6 +251,86 @@ describe 'Pool Manager' do
     end
   end
 
+  describe '#_stats_running_ready' do
+    let(:pool_helper) { double('pool') }
+    let(:vsphere) { {pool => pool_helper} }
+    let(:graphite) { double('graphite') }
+    let(:config) { {
+      config: { task_limit: 10 },
+      pools: [ {'name' => 'pool1', 'size' => 5} ],
+      graphite: { 'prefix' => 'vmpooler' }
+    } }
+
+    before do
+      expect(subject).not_to be_nil
+      $vsphere = vsphere
+      allow(logger).to receive(:log)
+      allow(pool_helper).to receive(:find_folder)
+      allow(redis).to receive(:smembers).and_return([])
+      allow(redis).to receive(:set)
+      allow(redis).to receive(:get).with('vmpooler__tasks__clone').and_return(0)
+      allow(redis).to receive(:get).with('vmpooler__empty__pool1').and_return(nil)
+    end
+
+    context 'graphite' do
+      let(:graphite) { double('graphite') }
+      subject { Vmpooler::PoolManager.new(config, logger, redis, graphite) }
+
+      it 'increments graphite when enabled and statsd disabled' do
+        allow(redis).to receive(:scard).with('vmpooler__ready__pool1').and_return(1)
+        allow(redis).to receive(:scard).with('vmpooler__cloning__pool1').and_return(0)
+        allow(redis).to receive(:scard).with('vmpooler__pending__pool1').and_return(0)
+        allow(redis).to receive(:scard).with('vmpooler__running__pool1').and_return(5)
+
+        expect(graphite).to receive(:log).with('vmpooler.ready.pool1', 1)
+        expect(graphite).to receive(:log).with('vmpooler.running.pool1', 5)
+        subject._check_pool(config[:pools][0])
+      end
+
+      it 'increments graphite when ready with 0 when pool empty and statsd disabled' do
+        allow(redis).to receive(:scard).with('vmpooler__ready__pool1').and_return(0)
+        allow(redis).to receive(:scard).with('vmpooler__cloning__pool1').and_return(0)
+        allow(redis).to receive(:scard).with('vmpooler__pending__pool1').and_return(0)
+        allow(redis).to receive(:scard).with('vmpooler__running__pool1').and_return(5)
+
+        expect(graphite).to receive(:log).with('vmpooler.ready.pool1', 0)
+        expect(graphite).to receive(:log).with('vmpooler.running.pool1', 5)
+        subject._check_pool(config[:pools][0])
+      end
+    end
+
+    context 'statsd' do
+      let(:statsd) { double('statsd') }
+      let(:config) { {
+        config: { task_limit: 10 },
+        pools: [ {'name' => 'pool1', 'size' => 5} ],
+        statsd: { 'prefix' => 'vmpooler' }
+      } }
+      subject { Vmpooler::PoolManager.new(config, logger, redis, graphite, statsd) }
+
+      it 'increments statsd when configured' do
+        allow(redis).to receive(:scard).with('vmpooler__ready__pool1').and_return(1)
+        allow(redis).to receive(:scard).with('vmpooler__cloning__pool1').and_return(0)
+        allow(redis).to receive(:scard).with('vmpooler__pending__pool1').and_return(0)
+        allow(redis).to receive(:scard).with('vmpooler__running__pool1').and_return(5)
+
+        expect(statsd).to receive(:gauge).with('vmpooler.ready.pool1', 1)
+        expect(statsd).to receive(:gauge).with('vmpooler.running.pool1', 5)
+        subject._check_pool(config[:pools][0])
+      end
+
+      it 'increments statsd ready with 0 when pool empty' do
+        allow(redis).to receive(:scard).with('vmpooler__running__pool1').and_return(1)
+        allow(redis).to receive(:scard).with('vmpooler__ready__pool1').and_return(0)
+        allow(redis).to receive(:scard).with('vmpooler__pending__pool1').and_return(0)
+        allow(statsd).to receive(:gauge).with('vmpooler.running.pool1', 1)
+
+        expect(statsd).to receive(:gauge).with('vmpooler.ready.pool1', 0)
+        subject._check_pool(config[:pools][0])
+      end
+    end
+  end
+
   describe '#_create_vm_snapshot' do
     let(:snapshot_manager) { 'snapshot_manager' }
     let(:pool_helper) { double('snapshot_manager') }
diff --git a/vmpooler b/vmpooler
index 5d0dd51..44ef89c 100755
--- a/vmpooler
+++ b/vmpooler
@@ -9,10 +9,19 @@ 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
-  thr.helpers.configure(config, Vmpooler.new_redis(redis_host))
+  if 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!
 }
 
@@ -21,7 +30,8 @@ manager = Thread.new {
     config,
     Vmpooler.new_logger(logger_file),
     Vmpooler.new_redis(redis_host),
-    Vmpooler.new_graphite(graphite)
+    Vmpooler.new_graphite(graphite),
+    Vmpooler.new_statsd(statsd, statsd_port)
   ).execute!
 }
 
diff --git a/vmpooler.yaml.example b/vmpooler.yaml.example
index 26f2c51..5476094 100644
--- a/vmpooler.yaml.example
+++ b/vmpooler.yaml.example
@@ -53,10 +53,39 @@
 :redis:
   server: 'redis.company.com'
 
+
+  # :statsd:
+  #
+  # This section contains the connection information required to store
+  # historical data via statsd.  This is mutually exclusive with graphite 
+  # and takes precedence.
+  #
+  # Available configuration parameters:
+  #
+  #   - server
+  #     The FQDN hostname of the statsd daemon.
+  #     (optional)
+  #
+  #   - prefix
+  #     The prefix to use while storing statsd data.
+  #     (optional; default: 'vmpooler')
+  #
+  #   - port
+  #     The UDP port to communicate with statsd daemon.
+  #     (optional; default: 8125)
+
+  # Example:
+
+  :statsd:
+    server: 'statsd.company.com'
+    prefix: 'vmpooler'
+    port: 8125
+
 # :graphite:
 #
 # This section contains the connection information required to store
-# historical data in an external Graphite database.
+# historical data in an external Graphite database.  This is mutually exclusive
+# with statsd.
 #
 # Available configuration parameters:
 #

From 3c531d3ec3819cf9402d86242fdbb201ec4dfe82 Mon Sep 17 00:00:00 2001
From: Rick Bradley <rick@rickbradley.com>
Date: Mon, 11 Jul 2016 16:37:14 -0500
Subject: [PATCH 02/21] Fix some spec errors

These were caused in part by dropping changes from the original PR when we
dropped the v1_spec.rb master test file (in favor of the updated and separated
versions).
---
 spec/vmpooler/api/v1/vm_spec.rb          | 4 +++-
 spec/vmpooler/api/v1/vm_template_spec.rb | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/spec/vmpooler/api/v1/vm_spec.rb b/spec/vmpooler/api/v1/vm_spec.rb
index 5622468..632dc8b 100644
--- a/spec/vmpooler/api/v1/vm_spec.rb
+++ b/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(:config) {
       {
         config: {
@@ -31,6 +31,7 @@ describe Vmpooler::API::V1 do
           {'name' => 'pool1', 'size' => 5},
           {'name' => 'pool2', 'size' => 10}
         ],
+        statsd: { 'prefix' => 'stats_prefix'},
         alias: { 'poolone' => 'pool1' },
         pool_names: [ 'pool1', 'pool2', 'poolone' ]
       }
@@ -43,6 +44,7 @@ describe Vmpooler::API::V1 do
 
       app.settings.set :config, config
       app.settings.set :redis, redis
+      app.settings.set :statsd, statsd
       app.settings.set :config, auth: false
       create_token('abcdefghijklmnopqrstuvwxyz012345', 'jdoe', current_time)
     end
diff --git a/spec/vmpooler/api/v1/vm_template_spec.rb b/spec/vmpooler/api/v1/vm_template_spec.rb
index 3604427..5719536 100644
--- a/spec/vmpooler/api/v1/vm_template_spec.rb
+++ b/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(:config) {
       {
         config: {
@@ -31,6 +31,7 @@ describe Vmpooler::API::V1 do
           {'name' => 'pool1', 'size' => 5},
           {'name' => 'pool2', 'size' => 10}
         ],
+        statsd: { 'prefix' => 'stats_prefix'},
         alias: { 'poolone' => 'pool1' },
         pool_names: [ 'pool1', 'pool2', 'poolone' ]
       }
@@ -43,6 +44,7 @@ describe Vmpooler::API::V1 do
 
       app.settings.set :config, config
       app.settings.set :redis, redis
+      app.settings.set :statsd, statsd
       app.settings.set :config, auth: false
       create_token('abcdefghijklmnopqrstuvwxyz012345', 'jdoe', current_time)
     end

From f45cf10839ed1b14aa3e035818723777eeac7858 Mon Sep 17 00:00:00 2001
From: Rick Bradley <rick@rickbradley.com>
Date: Mon, 11 Jul 2016 22:35:30 -0500
Subject: [PATCH 03/21] [QENG-4075] Fix bug with template name on allocation
 failure

We're returning [nil,nil] in this case, meaning that name will not be set. This
means we'll get an error trying to concatenate the stats string. Use the
requested template name here instead.
---
 lib/vmpooler/api/v1.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/vmpooler/api/v1.rb b/lib/vmpooler/api/v1.rb
index 2d76970..a8ef43f 100644
--- a/lib/vmpooler/api/v1.rb
+++ b/lib/vmpooler/api/v1.rb
@@ -98,7 +98,7 @@ module Vmpooler
           vm, name = fetch_single_vm(requested)
           if !vm
             failed = true
-            statsd.increment(statsd_prefix + '.checkout.empty.' + name, 1)
+            statsd.increment(statsd_prefix + '.checkout.empty.' + requested, 1)
             break
           else
             vms << [ name, vm ]

From e9fae3fab2a48f9679ad0b8e941e00fb7e3862b7 Mon Sep 17 00:00:00 2001
From: Rick Bradley <rick@rickbradley.com>
Date: Tue, 12 Jul 2016 13:15:48 -0500
Subject: [PATCH 04/21] [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                          |  9 ++---
 lib/vmpooler/api/v1.rb                   | 14 +++-----
 lib/vmpooler/statsd.rb                   | 44 ++++++++++++++++++++++--
 spec/spec_helper.rb                      |  1 +
 spec/vmpooler/api/v1/vm_spec.rb          |  2 +-
 spec/vmpooler/api/v1/vm_template_spec.rb |  2 +-
 vmpooler                                 | 14 ++------
 7 files changed, 54 insertions(+), 32 deletions(-)

diff --git a/lib/vmpooler.rb b/lib/vmpooler.rb
index 5d07036..a4b7931 100644
--- a/lib/vmpooler.rb
+++ b/lib/vmpooler.rb
@@ -92,12 +92,9 @@ module Vmpooler
     end
   end
 
-  def self.new_statsd(server, port)
-    if server.nil? || server.empty?
-      nil
-    else
-      Statsd.new server, port
-    end
+  def self.new_statsd(params)
+    return DummyStatsd.new unless params
+    Statsd.new params
   end
 
   def self.pools(conf)
diff --git a/lib/vmpooler/api/v1.rb b/lib/vmpooler/api/v1.rb
index a8ef43f..4db1ff1 100644
--- a/lib/vmpooler/api/v1.rb
+++ b/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
diff --git a/lib/vmpooler/statsd.rb b/lib/vmpooler/statsd.rb
index b449a59..6db50ad 100644
--- a/lib/vmpooler/statsd.rb
+++ b/lib/vmpooler/statsd.rb
@@ -2,8 +2,48 @@ require 'rubygems' unless defined?(Gem)
 
 module Vmpooler
   class Statsd
-    def initialize(server = 'statsd', port = 8125)
-      @server = Statsd.new(server, port)
+    attr_reader :server, :port, :prefix
+
+    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
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index c401e32..83a5cdf 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -7,3 +7,4 @@ require 'rbvmomi'
 require 'rspec'
 require 'vmpooler'
 require 'redis'
+require 'vmpooler/statsd'
diff --git a/spec/vmpooler/api/v1/vm_spec.rb b/spec/vmpooler/api/v1/vm_spec.rb
index 632dc8b..af2c817 100644
--- a/spec/vmpooler/api/v1/vm_spec.rb
+++ b/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: {
diff --git a/spec/vmpooler/api/v1/vm_template_spec.rb b/spec/vmpooler/api/v1/vm_template_spec.rb
index 5719536..7a63626 100644
--- a/spec/vmpooler/api/v1/vm_template_spec.rb
+++ b/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: {
diff --git a/vmpooler b/vmpooler
index 44ef89c..6bfa5e5 100755
--- a/vmpooler
+++ b/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(statsd, statsd_port))
-  else
-    thr.helpers.configure(config, Vmpooler.new_redis(redis_host), statsd=nil)
-  end
+  thr.helpers.configure(config, Vmpooler.new_redis(redis_host), Vmpooler.new_statsd(config[:statsd]))
   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 }
-

From f26e0f9bd72427e7cdf5a1d08090c111f538fe7b Mon Sep 17 00:00:00 2001
From: Rick Bradley <rick@rickbradley.com>
Date: Tue, 12 Jul 2016 13:21:48 -0500
Subject: [PATCH 05/21] [QENG-4075] Drop now-superfluous :statsd config
 defaulting

---
 lib/vmpooler.rb | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/lib/vmpooler.rb b/lib/vmpooler.rb
index a4b7931..215fbce 100644
--- a/lib/vmpooler.rb
+++ b/lib/vmpooler.rb
@@ -58,13 +58,6 @@ module Vmpooler
       parsed_config[:graphite]['prefix'] ||= 'vmpooler'
     end
 
-    # statsd is an addition and my not be present in YAML configuration
-    if parsed_config[:statsd]
-      if parsed_config[:statsd]['server']
-        parsed_config[:statsd]['prefix'] ||= 'vmpooler'
-      end
-    end
-
     if parsed_config[:tagfilter]
       parsed_config[:tagfilter].keys.each do |tag|
         parsed_config[:tagfilter][tag] = Regexp.new(parsed_config[:tagfilter][tag])

From 218f0988009cf7db21eebe5a445cd71e8eb1059a Mon Sep 17 00:00:00 2001
From: Rick Bradley <rick@rickbradley.com>
Date: Tue, 12 Jul 2016 14:24:58 -0500
Subject: [PATCH 06/21] [QENG-4075] Unify graphite and statsd for the pool
 manager

Prior to this, the `pool_manager.rb` library could take handles for both
graphite and statsd endpoints (which were considered mutually exclusive),
and then would use one. There was a bevy of conditional logic around sending
metrics to the graphite/statsd handles (and actually at least one bug of
omission).

Here we refactor more, building on earlier work:

 - Our graphite class comes into line with the API of our Statsd and DummyStatsd classes
 - In `pool_manager.rb` we now accept a single "metrics" handle, and we drop all the conditional logic around statsd vs. graphite
 - We move the inconsistent error handling out of the calling classes and into our metrics classes, actually logging to `$stderr` when we can't publish metrics
 - We unify the setup code to use `config` to determine whether statsd, graphite, or a dummy metrics handle should be used, and make that happen.
 - Cleaned up some tests. We could probably stand to do a bit more work in this area.
---
 lib/vmpooler.rb                    | 20 +++++-----------
 lib/vmpooler/graphite.rb           | 37 ++++++++++++++++++++++++------
 lib/vmpooler/pool_manager.rb       | 33 +++++++-------------------
 lib/vmpooler/statsd.rb             | 10 ++++++--
 spec/vmpooler/pool_manager_spec.rb | 31 +++++++++++--------------
 vmpooler                           |  8 +++----
 6 files changed, 70 insertions(+), 69 deletions(-)

diff --git a/lib/vmpooler.rb b/lib/vmpooler.rb
index 215fbce..bef010f 100644
--- a/lib/vmpooler.rb
+++ b/lib/vmpooler.rb
@@ -54,10 +54,6 @@ module Vmpooler
       end
     end
 
-    if parsed_config[:graphite]['server']
-      parsed_config[:graphite]['prefix'] ||= 'vmpooler'
-    end
-
     if parsed_config[:tagfilter]
       parsed_config[:tagfilter].keys.each do |tag|
         parsed_config[:tagfilter][tag] = Regexp.new(parsed_config[:tagfilter][tag])
@@ -65,7 +61,6 @@ module Vmpooler
     end
 
     parsed_config[:uptime] = Time.now
-
     parsed_config
   end
 
@@ -77,19 +72,16 @@ module Vmpooler
     Vmpooler::Logger.new logfile
   end
 
-  def self.new_graphite(server)
-    if server.nil? or server.empty? or server.length == 0
-      nil
+  def self.new_metrics(params)
+    if config[:statsd]
+      Vmpooler::Statsd.new(config[:statsd])
+    elsif config[:graphite]
+      Vmpooler::Graphite.new(config[:graphite])
     else
-      Vmpooler::Graphite.new server
+      Vmpooler::DummyStatsd.new
     end
   end
 
-  def self.new_statsd(params)
-    return DummyStatsd.new unless params
-    Statsd.new params
-  end
-
   def self.pools(conf)
     conf[:pools]
   end
diff --git a/lib/vmpooler/graphite.rb b/lib/vmpooler/graphite.rb
index 128d07b..3af1039 100644
--- a/lib/vmpooler/graphite.rb
+++ b/lib/vmpooler/graphite.rb
@@ -2,18 +2,41 @@ require 'rubygems' unless defined?(Gem)
 
 module Vmpooler
   class Graphite
-    def initialize(
-      s = 'graphite'
-    )
-      @server = s
+    attr_reader :server, :port, :prefix
+
+    def initialize(params = {})
+      if params[:server].nil? || params[:server].empty?
+        raise ArgumentError, "Graphite server is required. Config: #{params.inspect}"
+      end
+
+      @server = params[:server]
+      @port   = params[:port]   || 2003
+      @prefix = params[:prefix] || "vmpooler"
+    end
+
+    def increment(label)
+      log label, 1
+    end
+
+    def gauge(label, value)
+      log label, value
+    end
+
+    def timing(label, duration)
+      log label, duration
     end
 
     def log(path, value)
       Thread.new do
-        socket = TCPSocket.new(@server, 2003)
-        socket.puts "#{path} #{value} #{Time.now.to_i}"
-        socket.close
+        socket = TCPSocket.new(server, port)
+        begin
+          socket.puts "#{prefix}.#{path} #{value} #{Time.now.to_i}"
+        ensure
+          socket.close
+        end
       end
+    rescue => err
+      $stderr.puts "Failure logging #{path} to graphite server [#{server}:#{port}]: #{err}"
     end
   end
 end
diff --git a/lib/vmpooler/pool_manager.rb b/lib/vmpooler/pool_manager.rb
index 5f54169..7c0e001 100644
--- a/lib/vmpooler/pool_manager.rb
+++ b/lib/vmpooler/pool_manager.rb
@@ -1,17 +1,13 @@
 module Vmpooler
   class PoolManager
-    def initialize(config, logger, redis, graphite = nil, statsd = nil)
+    def initialize(config, logger, redis, metrics = nil)
       $config = config
 
       # Load logger library
       $logger = logger
 
-      # statsd and graphite are mutex in the context of vmpooler
-      if statsd
-        $statsd = statsd
-      elsif graphite
-        $graphite = graphite
-      end
+      # metrics logging handle
+      $metrics = metrics
 
       # Connect to Redis
       $redis = redis
@@ -66,7 +62,7 @@ module Vmpooler
           (host.summary.guest.hostName == vm)
 
         begin
-          Socket.getaddrinfo(vm, nil)
+          Socket.getaddrinfo(vm, nil)  # WTF?
         rescue
         end
 
@@ -260,11 +256,7 @@ module Vmpooler
 
         $redis.decr('vmpooler__tasks__clone')
 
-        begin
-          $statsd.timing($config[:statsd]['prefix'] + ".clone.#{vm['template']}", finish) if $statsd
-          $graphite.log($config[:graphite]['prefix'] + ".clone.#{vm['template']}", finish) if $graphite
-        rescue
-        end
+        $metrics.timing("clone.#{vm['template']}", finish)
       end
     end
 
@@ -297,8 +289,7 @@ module Vmpooler
           finish = '%.2f' % (Time.now - start)
 
           $logger.log('s', "[-] [#{pool}] '#{vm}' destroyed in #{finish} seconds")
-
-          $graphite.log($config[:graphite]['prefix'] + ".destroy.#{pool}", finish) if $graphite
+          $metrics.log("destroy.#{pool}", finish)
         end
       end
     end
@@ -568,16 +559,8 @@ module Vmpooler
       ready = $redis.scard('vmpooler__ready__' + pool['name'])
       total = $redis.scard('vmpooler__pending__' + pool['name']) + ready
 
-      begin
-        if $statsd
-          $statsd.gauge($config[:statsd]['prefix'] + '.ready.' + pool['name'], $redis.scard('vmpooler__ready__' + pool['name']))
-          $statsd.gauge($config[:statsd]['prefix'] + '.running.' + pool['name'], $redis.scard('vmpooler__running__' + pool['name']))
-        elsif $graphite
-          $graphite.log($config[:graphite]['prefix'] + '.ready.' + pool['name'], $redis.scard('vmpooler__ready__' + pool['name']))
-          $graphite.log($config[:graphite]['prefix'] + '.running.' + pool['name'], $redis.scard('vmpooler__running__' + pool['name']))
-        end
-      rescue
-      end
+      $metrics.gauge('ready.' + pool['name'], $redis.scard('vmpooler__ready__' + pool['name']))
+      $metrics.gauge('running.' + pool['name'], $redis.scard('vmpooler__running__' + pool['name']))
 
       if $redis.get('vmpooler__empty__' + pool['name'])
         unless ready == 0
diff --git a/lib/vmpooler/statsd.rb b/lib/vmpooler/statsd.rb
index 6db50ad..2bcb047 100644
--- a/lib/vmpooler/statsd.rb
+++ b/lib/vmpooler/statsd.rb
@@ -10,21 +10,27 @@ module Vmpooler
       end
 
       host    = params[:server]
-      port    = params[:port]   || 8125
+      @port    = params[:port]   || 8125
       @prefix = params[:prefix] || 'vmpooler'
-      @server = Statsd.new(host, port)
+      @server = Statsd.new(host, @port)
     end
 
     def increment(label)
       server.increment(prefix + "." + label)
+    rescue => err
+      $stderr.puts "Failure incrementing #{prefix}.#{label} on statsd server [#{server}:#{port}]: #{err}"
     end
 
     def gauge(label, value)
       server.gauge(prefix + "." + label, value)
+    rescue => err
+      $stderr.puts "Failure updating gauge #{prefix}.#{label} on statsd server [#{server}:#{port}]: #{err}"
     end
 
     def timing(label, duration)
       server.timing(prefix + "." + label, duration)
+    rescue => err
+      $stderr.puts "Failure updating timing #{prefix}.#{label} on statsd server [#{server}:#{port}]: #{err}"
     end
   end
 
diff --git a/spec/vmpooler/pool_manager_spec.rb b/spec/vmpooler/pool_manager_spec.rb
index 9adeab9..4ba7870 100644
--- a/spec/vmpooler/pool_manager_spec.rb
+++ b/spec/vmpooler/pool_manager_spec.rb
@@ -4,13 +4,14 @@ require 'time'
 describe 'Pool Manager' do
   let(:logger) { double('logger') }
   let(:redis) { double('redis') }
+  let(:metrics) { Vmpooler::DummyStatsd.new }
   let(:config) { {} }
   let(:pool) { 'pool1' }
   let(:vm) { 'vm1' }
   let(:timeout) { 5 }
   let(:host) { double('host') }
 
-  subject { Vmpooler::PoolManager.new(config, logger, redis) }
+  subject { Vmpooler::PoolManager.new(config, logger, redis, metrics) }
 
   describe '#_check_pending_vm' do
     let(:pool_helper) { double('pool') }
@@ -190,9 +191,7 @@ describe 'Pool Manager' do
 
         subject._check_running_vm(vm, pool, timeout)
       end
-
     end
-
   end
 
   describe '#move_running_to_completed' do
@@ -239,22 +238,21 @@ describe 'Pool Manager' do
     end
 
     context 'logging' do
-
       it 'logs empty pool' do
         allow(redis).to receive(:scard).with('vmpooler__pending__pool1').and_return(0)
         allow(redis).to receive(:scard).with('vmpooler__ready__pool1').and_return(0)
+        allow(redis).to receive(:scard).with('vmpooler__running__pool1').and_return(0)
 
         expect(logger).to receive(:log).with('s', "[!] [pool1] is empty")
         subject._check_pool(config[:pools][0])
       end
-
     end
   end
 
   describe '#_stats_running_ready' do
     let(:pool_helper) { double('pool') }
     let(:vsphere) { {pool => pool_helper} }
-    let(:graphite) { double('graphite') }
+    let(:graphite) { Vmpooler::DummyStatsd.new }
     let(:config) { {
       config: { task_limit: 10 },
       pools: [ {'name' => 'pool1', 'size' => 5} ],
@@ -273,7 +271,6 @@ describe 'Pool Manager' do
     end
 
     context 'graphite' do
-      let(:graphite) { double('graphite') }
       subject { Vmpooler::PoolManager.new(config, logger, redis, graphite) }
 
       it 'increments graphite when enabled and statsd disabled' do
@@ -282,8 +279,8 @@ describe 'Pool Manager' do
         allow(redis).to receive(:scard).with('vmpooler__pending__pool1').and_return(0)
         allow(redis).to receive(:scard).with('vmpooler__running__pool1').and_return(5)
 
-        expect(graphite).to receive(:log).with('vmpooler.ready.pool1', 1)
-        expect(graphite).to receive(:log).with('vmpooler.running.pool1', 5)
+        expect(graphite).to receive(:gauge).with('ready.pool1', 1)
+        expect(graphite).to receive(:gauge).with('running.pool1', 5)
         subject._check_pool(config[:pools][0])
       end
 
@@ -293,20 +290,20 @@ describe 'Pool Manager' do
         allow(redis).to receive(:scard).with('vmpooler__pending__pool1').and_return(0)
         allow(redis).to receive(:scard).with('vmpooler__running__pool1').and_return(5)
 
-        expect(graphite).to receive(:log).with('vmpooler.ready.pool1', 0)
-        expect(graphite).to receive(:log).with('vmpooler.running.pool1', 5)
+        expect(graphite).to receive(:gauge).with('ready.pool1', 0)
+        expect(graphite).to receive(:gauge).with('running.pool1', 5)
         subject._check_pool(config[:pools][0])
       end
     end
 
     context 'statsd' do
-      let(:statsd) { double('statsd') }
+      let(:statsd) { Vmpooler::DummyStatsd.new }
       let(:config) { {
         config: { task_limit: 10 },
         pools: [ {'name' => 'pool1', 'size' => 5} ],
         statsd: { 'prefix' => 'vmpooler' }
       } }
-      subject { Vmpooler::PoolManager.new(config, logger, redis, graphite, statsd) }
+      subject { Vmpooler::PoolManager.new(config, logger, redis, statsd) }
 
       it 'increments statsd when configured' do
         allow(redis).to receive(:scard).with('vmpooler__ready__pool1').and_return(1)
@@ -314,8 +311,8 @@ describe 'Pool Manager' do
         allow(redis).to receive(:scard).with('vmpooler__pending__pool1').and_return(0)
         allow(redis).to receive(:scard).with('vmpooler__running__pool1').and_return(5)
 
-        expect(statsd).to receive(:gauge).with('vmpooler.ready.pool1', 1)
-        expect(statsd).to receive(:gauge).with('vmpooler.running.pool1', 5)
+        expect(statsd).to receive(:gauge).with('ready.pool1', 1)
+        expect(statsd).to receive(:gauge).with('running.pool1', 5)
         subject._check_pool(config[:pools][0])
       end
 
@@ -323,9 +320,9 @@ describe 'Pool Manager' do
         allow(redis).to receive(:scard).with('vmpooler__running__pool1').and_return(1)
         allow(redis).to receive(:scard).with('vmpooler__ready__pool1').and_return(0)
         allow(redis).to receive(:scard).with('vmpooler__pending__pool1').and_return(0)
-        allow(statsd).to receive(:gauge).with('vmpooler.running.pool1', 1)
+        allow(statsd).to receive(:gauge).with('running.pool1', 1)
 
-        expect(statsd).to receive(:gauge).with('vmpooler.ready.pool1', 0)
+        expect(statsd).to receive(:gauge).with('ready.pool1', 0)
         subject._check_pool(config[:pools][0])
       end
     end
diff --git a/vmpooler b/vmpooler
index 6bfa5e5..74a516d 100755
--- a/vmpooler
+++ b/vmpooler
@@ -8,11 +8,12 @@ require 'lib/vmpooler'
 config = Vmpooler.config
 redis_host = config[:redis]['server']
 logger_file = config[:config]['logfile']
-graphite = config[:graphite]['server'] ? config[:graphite]['server'] : nil
+
+metrics = Vmpooler.new_metrics(config)
 
 api = Thread.new {
   thr = Vmpooler::API.new
-  thr.helpers.configure(config, Vmpooler.new_redis(redis_host), Vmpooler.new_statsd(config[:statsd]))
+  thr.helpers.configure(config, Vmpooler.new_redis(redis_host), metrics)
   thr.helpers.execute!
 }
 
@@ -21,8 +22,7 @@ manager = Thread.new {
     config,
     Vmpooler.new_logger(logger_file),
     Vmpooler.new_redis(redis_host),
-    Vmpooler.new_graphite(graphite),
-    Vmpooler.new_statsd(config[:statsd])
+    metrics)
   ).execute!
 }
 

From ad56bbc73236eea2ff23fd7718a54ec07b43a117 Mon Sep 17 00:00:00 2001
From: Rick Bradley <rick@rickbradley.com>
Date: Tue, 12 Jul 2016 14:34:56 -0500
Subject: [PATCH 07/21] [QENG-4075] Clean up pool manager, specs

Prior to this, `pool_manager.rb` allowed the `metrics` argument to be optional,
but at this point it will be an instance of `Vmpooler::Statsd`,
'Vmpooler::Graphite', or `Vmpooler::DummyStatsd`, so making this non-optional.

Cleaned up that file's tests, cosmetically, as well as recognizing that the
behavioral difference between graphite and statsd does not depend on the pool
manager.
---
 lib/vmpooler/pool_manager.rb       |  2 +-
 spec/vmpooler/pool_manager_spec.rb | 54 +++++-------------------------
 2 files changed, 10 insertions(+), 46 deletions(-)

diff --git a/lib/vmpooler/pool_manager.rb b/lib/vmpooler/pool_manager.rb
index 7c0e001..b2e348e 100644
--- a/lib/vmpooler/pool_manager.rb
+++ b/lib/vmpooler/pool_manager.rb
@@ -1,6 +1,6 @@
 module Vmpooler
   class PoolManager
-    def initialize(config, logger, redis, metrics = nil)
+    def initialize(config, logger, redis, metrics)
       $config = config
 
       # Load logger library
diff --git a/spec/vmpooler/pool_manager_spec.rb b/spec/vmpooler/pool_manager_spec.rb
index 4ba7870..c569578 100644
--- a/spec/vmpooler/pool_manager_spec.rb
+++ b/spec/vmpooler/pool_manager_spec.rb
@@ -23,14 +23,12 @@ describe 'Pool Manager' do
     end
 
     context 'host not in pool' do
-
       it 'calls fail_pending_vm' do
         allow(pool_helper).to receive(:find_vm).and_return(nil)
         allow(redis).to receive(:hget)
         expect(redis).to receive(:hget).with(String, 'clone').once
         subject._check_pending_vm(vm, pool, timeout)
       end
-
     end
 
     context 'host is in pool' do
@@ -58,7 +56,6 @@ describe 'Pool Manager' do
     end
 
     context 'a host without correct summary' do
-
       it 'does nothing when summary is nil' do
         allow(host).to receive(:summary).and_return nil
         subject.move_pending_vm_to_ready(vm, pool, host)
@@ -114,7 +111,6 @@ describe 'Pool Manager' do
 
         subject.move_pending_vm_to_ready(vm, pool, host)
       end
-
     end
   end
 
@@ -252,7 +248,7 @@ describe 'Pool Manager' do
   describe '#_stats_running_ready' do
     let(:pool_helper) { double('pool') }
     let(:vsphere) { {pool => pool_helper} }
-    let(:graphite) { Vmpooler::DummyStatsd.new }
+    let(:metrics) { Vmpooler::DummyStatsd.new }
     let(:config) { {
       config: { task_limit: 10 },
       pools: [ {'name' => 'pool1', 'size' => 5} ],
@@ -270,59 +266,28 @@ describe 'Pool Manager' do
       allow(redis).to receive(:get).with('vmpooler__empty__pool1').and_return(nil)
     end
 
-    context 'graphite' do
-      subject { Vmpooler::PoolManager.new(config, logger, redis, graphite) }
+    context 'metrics' do
+      subject { Vmpooler::PoolManager.new(config, logger, redis, metrics) }
 
-      it 'increments graphite when enabled and statsd disabled' do
+      it 'increments metrics' do
         allow(redis).to receive(:scard).with('vmpooler__ready__pool1').and_return(1)
         allow(redis).to receive(:scard).with('vmpooler__cloning__pool1').and_return(0)
         allow(redis).to receive(:scard).with('vmpooler__pending__pool1').and_return(0)
         allow(redis).to receive(:scard).with('vmpooler__running__pool1').and_return(5)
 
-        expect(graphite).to receive(:gauge).with('ready.pool1', 1)
-        expect(graphite).to receive(:gauge).with('running.pool1', 5)
+        expect(metrics).to receive(:gauge).with('ready.pool1', 1)
+        expect(metrics).to receive(:gauge).with('running.pool1', 5)
         subject._check_pool(config[:pools][0])
       end
 
-      it 'increments graphite when ready with 0 when pool empty and statsd disabled' do
+      it 'increments metrics when ready with 0 when pool empty' do
         allow(redis).to receive(:scard).with('vmpooler__ready__pool1').and_return(0)
         allow(redis).to receive(:scard).with('vmpooler__cloning__pool1').and_return(0)
         allow(redis).to receive(:scard).with('vmpooler__pending__pool1').and_return(0)
         allow(redis).to receive(:scard).with('vmpooler__running__pool1').and_return(5)
 
-        expect(graphite).to receive(:gauge).with('ready.pool1', 0)
-        expect(graphite).to receive(:gauge).with('running.pool1', 5)
-        subject._check_pool(config[:pools][0])
-      end
-    end
-
-    context 'statsd' do
-      let(:statsd) { Vmpooler::DummyStatsd.new }
-      let(:config) { {
-        config: { task_limit: 10 },
-        pools: [ {'name' => 'pool1', 'size' => 5} ],
-        statsd: { 'prefix' => 'vmpooler' }
-      } }
-      subject { Vmpooler::PoolManager.new(config, logger, redis, statsd) }
-
-      it 'increments statsd when configured' do
-        allow(redis).to receive(:scard).with('vmpooler__ready__pool1').and_return(1)
-        allow(redis).to receive(:scard).with('vmpooler__cloning__pool1').and_return(0)
-        allow(redis).to receive(:scard).with('vmpooler__pending__pool1').and_return(0)
-        allow(redis).to receive(:scard).with('vmpooler__running__pool1').and_return(5)
-
-        expect(statsd).to receive(:gauge).with('ready.pool1', 1)
-        expect(statsd).to receive(:gauge).with('running.pool1', 5)
-        subject._check_pool(config[:pools][0])
-      end
-
-      it 'increments statsd ready with 0 when pool empty' do
-        allow(redis).to receive(:scard).with('vmpooler__running__pool1').and_return(1)
-        allow(redis).to receive(:scard).with('vmpooler__ready__pool1').and_return(0)
-        allow(redis).to receive(:scard).with('vmpooler__pending__pool1').and_return(0)
-        allow(statsd).to receive(:gauge).with('running.pool1', 1)
-
-        expect(statsd).to receive(:gauge).with('ready.pool1', 0)
+        expect(metrics).to receive(:gauge).with('ready.pool1', 0)
+        expect(metrics).to receive(:gauge).with('running.pool1', 5)
         subject._check_pool(config[:pools][0])
       end
     end
@@ -395,5 +360,4 @@ describe 'Pool Manager' do
       subject._check_snapshot_queue
     end
   end
-
 end

From 3a299af59d4b82a9d7f5db3256c3938057444e32 Mon Sep 17 00:00:00 2001
From: Rick Bradley <rick@rickbradley.com>
Date: Tue, 12 Jul 2016 14:38:54 -0500
Subject: [PATCH 08/21] [QENG-4075] update example vmpooler.yaml file

This documents the changes to :server being mandatory for all metrics
endpoints, as well as the graphite endpoint supporting an optional :port
configuration value.
---
 vmpooler.yaml.example | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/vmpooler.yaml.example b/vmpooler.yaml.example
index 5476094..64044e4 100644
--- a/vmpooler.yaml.example
+++ b/vmpooler.yaml.example
@@ -57,21 +57,21 @@
   # :statsd:
   #
   # This section contains the connection information required to store
-  # historical data via statsd.  This is mutually exclusive with graphite 
+  # historical data via statsd.  This is mutually exclusive with graphite
   # and takes precedence.
   #
   # Available configuration parameters:
   #
   #   - server
   #     The FQDN hostname of the statsd daemon.
-  #     (optional)
+  #     (required)
   #
   #   - prefix
   #     The prefix to use while storing statsd data.
   #     (optional; default: 'vmpooler')
   #
   #   - port
-  #     The UDP port to communicate with statsd daemon.
+  #     The UDP port to communicate with the statsd daemon.
   #     (optional; default: 8125)
 
   # Example:
@@ -91,11 +91,15 @@
 #
 #   - server
 #     The FQDN hostname of the Graphite server.
-#     (optional)
+#     (required)
 #
 #   - prefix
 #     The prefix to use while storing Graphite data.
 #     (optional; default: 'vmpooler')
+#
+#   - port
+#     The TCP port to communicate with the graphite server.
+#     (optional; default: 2003)
 
 # Example:
 
@@ -275,4 +279,3 @@
     size: 5
     timeout: 15
     ready_ttl: 1440
-

From 10e00a44f5b04e9f4913da7f70054b396e93bdb3 Mon Sep 17 00:00:00 2001
From: Rick Bradley <rick@rickbradley.com>
Date: Tue, 12 Jul 2016 14:50:10 -0500
Subject: [PATCH 09/21] [QENG-4075] Rename usages of statsd -> metrics

Really, let's just support a generic metrics interface.
---
 lib/vmpooler/api.rb                      |  4 ++--
 lib/vmpooler/api/v1.rb                   | 12 ++++++------
 spec/vmpooler/api/v1/vm_spec.rb          |  4 ++--
 spec/vmpooler/api/v1/vm_template_spec.rb |  4 ++--
 4 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/lib/vmpooler/api.rb b/lib/vmpooler/api.rb
index 45a9bfa..68649e9 100644
--- a/lib/vmpooler/api.rb
+++ b/lib/vmpooler/api.rb
@@ -42,10 +42,10 @@ module Vmpooler
     use Vmpooler::API::Reroute
     use Vmpooler::API::V1
 
-    def configure(config, redis, statsd, environment = :production)
+    def configure(config, redis, metrics, environment = :production)
       self.settings.set :config, config
       self.settings.set :redis, redis
-      self.settings.set :statsd, statsd
+      self.settings.set :metrics, metrics
       self.settings.set :environment, environment
     end
 
diff --git a/lib/vmpooler/api/v1.rb b/lib/vmpooler/api/v1.rb
index 4db1ff1..bc599fe 100644
--- a/lib/vmpooler/api/v1.rb
+++ b/lib/vmpooler/api/v1.rb
@@ -12,8 +12,8 @@ module Vmpooler
       Vmpooler::API.settings.redis
     end
 
-    def statsd
-      Vmpooler::API.settings.statsd
+    def metrics
+      Vmpooler::API.settings.metrics
     end
 
     def config
@@ -92,11 +92,11 @@ module Vmpooler
           vm, name = fetch_single_vm(requested)
           if !vm
             failed = true
-            statsd.increment('checkout.empty.' + requested)
+            metrics.increment('checkout.empty.' + requested)
             break
           else
             vms << [ name, vm ]
-            statsd.increment('checkout.success.' + name)
+            metrics.increment('checkout.success.' + name)
           end
         end
       end
@@ -382,7 +382,7 @@ module Vmpooler
           result = atomically_allocate_vms(payload)
         else
           invalid.each do |bad_template|
-            statsd.increment('checkout.invalid.' + bad_template)
+            metrics.increment('checkout.invalid.' + bad_template)
           end
           status 404
         end
@@ -424,7 +424,7 @@ module Vmpooler
           result = atomically_allocate_vms(payload)
         else
           invalid.each do |bad_template|
-            statsd.increment('checkout.invalid.' + bad_template)
+            metrics.increment('checkout.invalid.' + bad_template)
           end
           status 404
         end
diff --git a/spec/vmpooler/api/v1/vm_spec.rb b/spec/vmpooler/api/v1/vm_spec.rb
index af2c817..0c14561 100644
--- a/spec/vmpooler/api/v1/vm_spec.rb
+++ b/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) { Vmpooler::DummyStatsd.new }
+    let(:metrics) { Vmpooler::DummyStatsd.new }
     let(:config) {
       {
         config: {
@@ -44,7 +44,7 @@ describe Vmpooler::API::V1 do
 
       app.settings.set :config, config
       app.settings.set :redis, redis
-      app.settings.set :statsd, statsd
+      app.settings.set :metrics, metrics
       app.settings.set :config, auth: false
       create_token('abcdefghijklmnopqrstuvwxyz012345', 'jdoe', current_time)
     end
diff --git a/spec/vmpooler/api/v1/vm_template_spec.rb b/spec/vmpooler/api/v1/vm_template_spec.rb
index 7a63626..aaabf4a 100644
--- a/spec/vmpooler/api/v1/vm_template_spec.rb
+++ b/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) { Vmpooler::DummyStatsd.new }
+    let(:metrics) { Vmpooler::DummyStatsd.new }
     let(:config) {
       {
         config: {
@@ -44,7 +44,7 @@ describe Vmpooler::API::V1 do
 
       app.settings.set :config, config
       app.settings.set :redis, redis
-      app.settings.set :statsd, statsd
+      app.settings.set :metrics, metrics
       app.settings.set :config, auth: false
       create_token('abcdefghijklmnopqrstuvwxyz012345', 'jdoe', current_time)
     end

From 3efed6321cd5ca6e2ab072a8da7a8ea80e6a0dbd Mon Sep 17 00:00:00 2001
From: Rick Bradley <rick@rickbradley.com>
Date: Tue, 12 Jul 2016 14:52:09 -0500
Subject: [PATCH 10/21] (maint) move statsd-ruby require into Vmpooler::Statsd
 class

We've managed to move mentions of this out of the calling code, so let's
move the require.
---
 lib/vmpooler.rb        | 1 -
 lib/vmpooler/statsd.rb | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/vmpooler.rb b/lib/vmpooler.rb
index bef010f..610f78d 100644
--- a/lib/vmpooler.rb
+++ b/lib/vmpooler.rb
@@ -7,7 +7,6 @@ module Vmpooler
   require 'rbvmomi'
   require 'redis'
   require 'sinatra/base'
-  require "statsd-ruby"
   require 'time'
   require 'timeout'
   require 'yaml'
diff --git a/lib/vmpooler/statsd.rb b/lib/vmpooler/statsd.rb
index 2bcb047..bb71101 100644
--- a/lib/vmpooler/statsd.rb
+++ b/lib/vmpooler/statsd.rb
@@ -1,4 +1,5 @@
 require 'rubygems' unless defined?(Gem)
+require 'statsd-ruby'
 
 module Vmpooler
   class Statsd

From 2f500715f0b077c352d5c8956883658a7b4636ae Mon Sep 17 00:00:00 2001
From: Rick Bradley <rick@rickbradley.com>
Date: Tue, 12 Jul 2016 14:54:20 -0500
Subject: [PATCH 11/21] (maint) metrics.log -> metrics.timing

We missed this during the refactoring. Bringing this up to date.
---
 lib/vmpooler/pool_manager.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/vmpooler/pool_manager.rb b/lib/vmpooler/pool_manager.rb
index b2e348e..53895bb 100644
--- a/lib/vmpooler/pool_manager.rb
+++ b/lib/vmpooler/pool_manager.rb
@@ -289,7 +289,7 @@ module Vmpooler
           finish = '%.2f' % (Time.now - start)
 
           $logger.log('s', "[-] [#{pool}] '#{vm}' destroyed in #{finish} seconds")
-          $metrics.log("destroy.#{pool}", finish)
+          $metrics.timing("destroy.#{pool}", finish)
         end
       end
     end

From c49c252f8f05359b3f534776c20221e8cbef089c Mon Sep 17 00:00:00 2001
From: Rick Bradley <rick@rickbradley.com>
Date: Tue, 12 Jul 2016 15:22:13 -0500
Subject: [PATCH 12/21] [QENG-4075] Allow specifying 'graphs:' for dashboard

Prior to this the dashboard front-end would use the configuration settings
for `graphite[:server]`/`graphite[:prefix]` to locate a graphite server
to use for rendering graphs.

Now that we have multiple possible metrics backends, the front-end graph
host for the dashboard could be entirely different from the back-end metrics
server that we publish to (if any).

This decouples those settings:

 - use `graphs[:server]` / `graphs[:prefix]` for the graphite-compatible web front-end to use for dashboard display graphs
 - fall back to `graphite[:server]`/`graphite[:prefix]` if `graphs` is not specified, in order to support legacy `vmpooler.yaml` configurations.

Note that since `statsd` takes precedence over `graphite`, it's possible to specify both `statsd` (for publishing) and `graphite` (for reading). We still prefer `graphs` over `graphite`.

Updated the example `vmpooler.yaml` config file.
---
 lib/vmpooler/api/dashboard.rb | 72 ++++++++++++++++++++++++++---------
 vmpooler.yaml.example         | 26 +++++++++++++
 2 files changed, 79 insertions(+), 19 deletions(-)

diff --git a/lib/vmpooler/api/dashboard.rb b/lib/vmpooler/api/dashboard.rb
index b188dc5..3ef085a 100644
--- a/lib/vmpooler/api/dashboard.rb
+++ b/lib/vmpooler/api/dashboard.rb
@@ -1,9 +1,57 @@
 module Vmpooler
   class API
   class Dashboard < Sinatra::Base
+
+    # handle to the App's configuration information
+    def config
+      @config ||= Vmpooler::API.settings.config
+    end
+
+    # configuration setting for server hosting graph URLs to view
+    def graph_server
+      return @graph_server if @graph_server
+
+      if config[:graphs]
+        return false unless config[:graphs]['server']
+        @graph_server = config[:graphs]['server']
+      elsif config[:graphite]
+        return false unless config[:graphite]['server']
+        @graph_server = config[:graphite]['server']
+      else
+        false
+      end
+    end
+
+    # configuration setting for URL prefix for graphs to view
+    def graph_prefix
+      return @graph_prefix if @graph_prefix
+
+      if config[:graphs]
+        return false unless config[:graphs]['prefix']
+        @graph_prefix = config[:graphs]['prefix']
+      elsif config[:graphite]
+        return false unless config[:graphite]['prefix']
+        @graph_prefix = config[:graphite]['prefix']
+      else
+        false
+      end
+    end
+
+    # what is the base URL for viewable graphs?
+    def graph_url
+      return false unless graph_server && graph_prefix
+      @graph_url ||= "http://#{graph_server}/render?target=#{graph_prefix}"
+    end
+
+    # return a full URL to a viewable graph for a given metrics target (graphite syntax)
+    def graph_link(target = "")
+      return "" unless graph_url
+      graph_url + target
+    end
+
+
     get '/dashboard/stats/vmpooler/pool/?' do
       content_type :json
-
       result = {}
 
       Vmpooler::API.settings.config[:pools].each do |pool|
@@ -13,13 +61,11 @@ module Vmpooler
       end
 
       if params[:history]
-        if Vmpooler::API.settings.config[:graphite]['server']
+        if graph_url
           history ||= {}
 
           begin
-            buffer = open(
-              'http://' + Vmpooler::API.settings.config[:graphite]['server'] + '/render?target=' + Vmpooler::API.settings.config[:graphite]['prefix'] + '.ready.*&from=-1hour&format=json'
-            ).read
+            buffer = open(graph_link('.qready.*&from=-1hour&format=json')).read
             history = JSON.parse(buffer)
 
             history.each do |pool|
@@ -52,59 +98,47 @@ module Vmpooler
           end
         end
       end
-
       JSON.pretty_generate(result)
     end
 
     get '/dashboard/stats/vmpooler/running/?' do
       content_type :json
-
       result = {}
 
       Vmpooler::API.settings.config[:pools].each do |pool|
         running = Vmpooler::API.settings.redis.scard('vmpooler__running__' + pool['name'])
         pool['major'] = Regexp.last_match[1] if pool['name'] =~ /^(\w+)\-/
-
         result[pool['major']] ||= {}
-
         result[pool['major']]['running'] = result[pool['major']]['running'].to_i + running.to_i
       end
 
       if params[:history]
-        if Vmpooler::API.settings.config[:graphite]['server']
+        if graph_url
           begin
-            buffer = open(
-              'http://' + Vmpooler::API.settings.config[:graphite]['server'] + '/render?target=' + Vmpooler::API.settings.config[:graphite]['prefix'] + '.running.*&from=-1hour&format=json'
-            ).read
+            buffer = open(graph_link('.running.*&from=-1hour&format=json')).read
             JSON.parse(buffer).each do |pool|
               if pool['target'] =~ /.*\.(.*)$/
                 pool['name'] = Regexp.last_match[1]
-
                 pool['major'] = Regexp.last_match[1] if pool['name'] =~ /^(\w+)\-/
-
                 result[pool['major']]['history'] ||= Array.new
 
                 for i in 0..pool['datapoints'].length
                   if
                     pool['datapoints'][i] &&
                     pool['datapoints'][i][0]
-
                     pool['last'] = pool['datapoints'][i][0]
-
                     result[pool['major']]['history'][i] ||= 0
                     result[pool['major']]['history'][i] = result[pool['major']]['history'][i].to_i + pool['datapoints'][i][0].to_i
                   else
                     result[pool['major']]['history'][i] = result[pool['major']]['history'][i].to_i + pool['last'].to_i
                   end
                 end
-
               end
             end
           rescue
           end
         end
       end
-
       JSON.pretty_generate(result)
     end
   end
diff --git a/vmpooler.yaml.example b/vmpooler.yaml.example
index 64044e4..4e54891 100644
--- a/vmpooler.yaml.example
+++ b/vmpooler.yaml.example
@@ -54,6 +54,32 @@
   server: 'redis.company.com'
 
 
+  # :graphs:
+  #
+  # This section contains the server and prefix information for a graphite-
+  # compatible web front-end where graphs may be viewed. This is used by the
+  # vmpooler dashboard to retrieve statistics and graphs for a given instance.
+  #
+  # NOTE: This is not the endpoint for publishing metrics data. See `graphite:`
+  #       and `statsd:` below.
+  #
+  # NOTE: If `graphs:` is not set, for legacy compatibility, `graphite:` will be
+  #       consulted for `server`/`prefix` information to use in locating a
+  #       graph server for our dashboard.  `graphs:` is recommended over
+  #       `graphite:`
+  #
+  #
+  # Available configuration parameters:
+  #
+  #
+  #   - server
+  #     The FQDN hostname of the statsd daemon.
+  #     (required)
+  #
+  #   - prefix
+  #     The prefix to use while storing statsd data.
+  #     (optional; default: 'vmpooler')
+
   # :statsd:
   #
   # This section contains the connection information required to store

From ac2b7c090c3a886d403934ea58e48c8a7223cdd9 Mon Sep 17 00:00:00 2001
From: Rick Bradley <rick@rickbradley.com>
Date: Tue, 12 Jul 2016 15:36:59 -0500
Subject: [PATCH 13/21] (maint) fix variable reference in new_metrics

This was referencing config directly, when what we want is for a
hash to be passed in (derived from config).
---
 lib/vmpooler.rb | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/vmpooler.rb b/lib/vmpooler.rb
index 610f78d..3ea3412 100644
--- a/lib/vmpooler.rb
+++ b/lib/vmpooler.rb
@@ -72,10 +72,10 @@ module Vmpooler
   end
 
   def self.new_metrics(params)
-    if config[:statsd]
-      Vmpooler::Statsd.new(config[:statsd])
-    elsif config[:graphite]
-      Vmpooler::Graphite.new(config[:graphite])
+    if params[:statsd]
+      Vmpooler::Statsd.new(params[:statsd])
+    elsif params[:graphite]
+      Vmpooler::Graphite.new(params[:graphite])
     else
       Vmpooler::DummyStatsd.new
     end

From 648ec6e774e34445ea738a9c0dd5f2c0d2b01699 Mon Sep 17 00:00:00 2001
From: Rick Bradley <rick@rickbradley.com>
Date: Tue, 12 Jul 2016 15:41:47 -0500
Subject: [PATCH 14/21] (maint) Fix typo in updated graph link call

---
 lib/vmpooler/api/dashboard.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/vmpooler/api/dashboard.rb b/lib/vmpooler/api/dashboard.rb
index 3ef085a..0139528 100644
--- a/lib/vmpooler/api/dashboard.rb
+++ b/lib/vmpooler/api/dashboard.rb
@@ -65,7 +65,7 @@ module Vmpooler
           history ||= {}
 
           begin
-            buffer = open(graph_link('.qready.*&from=-1hour&format=json')).read
+            buffer = open(graph_link('.ready.*&from=-1hour&format=json')).read
             history = JSON.parse(buffer)
 
             history.each do |pool|

From 90a3213425d7e134a39bf986dc4860f73e16901d Mon Sep 17 00:00:00 2001
From: Rick Bradley <rick@rickbradley.com>
Date: Tue, 12 Jul 2016 15:42:09 -0500
Subject: [PATCH 15/21] (maint) default :graphs prefix to 'vmpooler'

---
 lib/vmpooler/api/dashboard.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/vmpooler/api/dashboard.rb b/lib/vmpooler/api/dashboard.rb
index 0139528..297ade3 100644
--- a/lib/vmpooler/api/dashboard.rb
+++ b/lib/vmpooler/api/dashboard.rb
@@ -27,7 +27,7 @@ module Vmpooler
       return @graph_prefix if @graph_prefix
 
       if config[:graphs]
-        return false unless config[:graphs]['prefix']
+        return "vmpooler" unless config[:graphs]['prefix']
         @graph_prefix = config[:graphs]['prefix']
       elsif config[:graphite]
         return false unless config[:graphite]['prefix']

From e93461bd94712c4451bb678554c98ee9f1c101f3 Mon Sep 17 00:00:00 2001
From: Rick Bradley <rick@rickbradley.com>
Date: Tue, 12 Jul 2016 16:28:20 -0500
Subject: [PATCH 16/21] (maint) Fix parse error in vmpooler script

The things you find through manual QA :troll:
---
 vmpooler | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/vmpooler b/vmpooler
index 74a516d..20eba53 100755
--- a/vmpooler
+++ b/vmpooler
@@ -22,7 +22,7 @@ manager = Thread.new {
     config,
     Vmpooler.new_logger(logger_file),
     Vmpooler.new_redis(redis_host),
-    metrics)
+    metrics
   ).execute!
 }
 

From 6dc91f98c60495a6ee5276d454b0a53aa56987e9 Mon Sep 17 00:00:00 2001
From: Rick Bradley <rick@rickbradley.com>
Date: Tue, 12 Jul 2016 16:35:31 -0500
Subject: [PATCH 17/21] (maint) use strings instead of symbols in config

Nested hash data comes back with string keys, not symbols. Be consistent.
---
 lib/vmpooler/graphite.rb | 8 ++++----
 lib/vmpooler/statsd.rb   | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/lib/vmpooler/graphite.rb b/lib/vmpooler/graphite.rb
index 3af1039..d7d1a0d 100644
--- a/lib/vmpooler/graphite.rb
+++ b/lib/vmpooler/graphite.rb
@@ -5,13 +5,13 @@ module Vmpooler
     attr_reader :server, :port, :prefix
 
     def initialize(params = {})
-      if params[:server].nil? || params[:server].empty?
+      if params["server"].nil? || params["server"].empty?
         raise ArgumentError, "Graphite server is required. Config: #{params.inspect}"
       end
 
-      @server = params[:server]
-      @port   = params[:port]   || 2003
-      @prefix = params[:prefix] || "vmpooler"
+      @server = params["server"]
+      @port   = params["port"]   || 2003
+      @prefix = params["prefix"] || "vmpooler"
     end
 
     def increment(label)
diff --git a/lib/vmpooler/statsd.rb b/lib/vmpooler/statsd.rb
index bb71101..2bca00a 100644
--- a/lib/vmpooler/statsd.rb
+++ b/lib/vmpooler/statsd.rb
@@ -6,13 +6,13 @@ module Vmpooler
     attr_reader :server, :port, :prefix
 
     def initialize(params = {})
-      if params[:server].nil? || params[:server].empty?
+      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'
+      host    = params["server"]
+      @port    = params["port"]   || 8125
+      @prefix = params["prefix"] || 'vmpooler'
       @server = Statsd.new(host, @port)
     end
 

From fb323a70bfc248734077bcaa88824bca512b3e54 Mon Sep 17 00:00:00 2001
From: Rick Bradley <rick@rickbradley.com>
Date: Tue, 12 Jul 2016 16:50:50 -0500
Subject: [PATCH 18/21] [QENG-4075] Factor out Vmpooler::DummyStatsd

This makes it visible to lib/vmpooler.rb, as well as putting this dummy
metrics endpoint in its own file for easier discovery.
---
 lib/vmpooler.rb              |  2 +-
 lib/vmpooler/dummy_statsd.rb | 20 ++++++++++++++++++++
 lib/vmpooler/statsd.rb       | 19 -------------------
 3 files changed, 21 insertions(+), 20 deletions(-)
 create mode 100644 lib/vmpooler/dummy_statsd.rb

diff --git a/lib/vmpooler.rb b/lib/vmpooler.rb
index 3ea3412..0e5707c 100644
--- a/lib/vmpooler.rb
+++ b/lib/vmpooler.rb
@@ -12,7 +12,7 @@ module Vmpooler
   require 'yaml'
   require 'set'
 
-  %w( api graphite logger pool_manager vsphere_helper ).each do |lib|
+  %w( api graphite logger pool_manager vsphere_helper statsd dummy_statsd ).each do |lib|
     begin
       require "vmpooler/#{lib}"
     rescue LoadError
diff --git a/lib/vmpooler/dummy_statsd.rb b/lib/vmpooler/dummy_statsd.rb
new file mode 100644
index 0000000..2b1b621
--- /dev/null
+++ b/lib/vmpooler/dummy_statsd.rb
@@ -0,0 +1,20 @@
+module Vmpooler
+  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
diff --git a/lib/vmpooler/statsd.rb b/lib/vmpooler/statsd.rb
index 2bca00a..60d8b21 100644
--- a/lib/vmpooler/statsd.rb
+++ b/lib/vmpooler/statsd.rb
@@ -34,23 +34,4 @@ module Vmpooler
       $stderr.puts "Failure updating timing #{prefix}.#{label} on statsd server [#{server}:#{port}]: #{err}"
     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

From 912cc489c0dc7cc5161b689b4125bbcba8a77389 Mon Sep 17 00:00:00 2001
From: Rick Bradley <rick@rickbradley.com>
Date: Tue, 12 Jul 2016 17:05:50 -0500
Subject: [PATCH 19/21] (maint) clean up statsd inclusion and require lines

The library is actually required as 'statsd' and not 'ruby-statsd', best I can tell.
---
 Gemfile                | 2 +-
 lib/vmpooler/statsd.rb | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Gemfile b/Gemfile
index fb32937..9c9a05c 100644
--- a/Gemfile
+++ b/Gemfile
@@ -12,7 +12,7 @@ gem 'rbvmomi', '>= 1.8'
 gem 'redis', '>= 3.2'
 gem 'sinatra', '>= 1.4'
 gem 'net-ldap', '<= 0.12.1' # keep compatibility w/ jruby & mri-1.9.3
-gem 'statsd-ruby', '>= 1.3.0'
+gem 'statsd-ruby', '>= 1.3.0', :require => 'statsd'
 
 # Test deps
 group :test do
diff --git a/lib/vmpooler/statsd.rb b/lib/vmpooler/statsd.rb
index 60d8b21..ac2ccc2 100644
--- a/lib/vmpooler/statsd.rb
+++ b/lib/vmpooler/statsd.rb
@@ -1,5 +1,5 @@
 require 'rubygems' unless defined?(Gem)
-require 'statsd-ruby'
+require 'statsd'
 
 module Vmpooler
   class Statsd

From 0ecf2ae3458580710048718b54dfd46b9cd2cee2 Mon Sep 17 00:00:00 2001
From: Rick Bradley <rick@rickbradley.com>
Date: Tue, 12 Jul 2016 21:56:45 -0500
Subject: [PATCH 20/21] (maint) construct ::Statsd instead of Statsd

Because it's ambiguous in this scope, and, well, it doesn't
actually work in production.
---
 lib/vmpooler/statsd.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/vmpooler/statsd.rb b/lib/vmpooler/statsd.rb
index ac2ccc2..94a4065 100644
--- a/lib/vmpooler/statsd.rb
+++ b/lib/vmpooler/statsd.rb
@@ -13,7 +13,7 @@ module Vmpooler
       host    = params["server"]
       @port    = params["port"]   || 8125
       @prefix = params["prefix"] || 'vmpooler'
-      @server = Statsd.new(host, @port)
+      @server = ::Statsd.new(host, @port)
     end
 
     def increment(label)

From 3101a44f6367737a9e540389fd8845967ccafcf8 Mon Sep 17 00:00:00 2001
From: Rick Bradley <rick@rickbradley.com>
Date: Wed, 13 Jul 2016 10:59:41 -0500
Subject: [PATCH 21/21] [QENG-4075] Also track completely invalid requests

When we don't even get a pool name we still want metrics to be recorded.
---
 lib/vmpooler/api/v1.rb | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/vmpooler/api/v1.rb b/lib/vmpooler/api/v1.rb
index bc599fe..4bd9852 100644
--- a/lib/vmpooler/api/v1.rb
+++ b/lib/vmpooler/api/v1.rb
@@ -387,6 +387,7 @@ module Vmpooler
           status 404
         end
       else
+        metrics.increment('checkout.invalid.unknown')
         status 404
       end
 
@@ -429,6 +430,7 @@ module Vmpooler
           status 404
         end
       else
+        metrics.increment('checkout.invalid.unknown')
         status 404
       end