From e9fae3fab2a48f9679ad0b8e941e00fb7e3862b7 Mon Sep 17 00:00:00 2001 From: Rick Bradley Date: Tue, 12 Jul 2016 13:15:48 -0500 Subject: [PATCH] [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 } -