Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(WIP) (RE-7014) Add support for statsd #150

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Gemfile
Original file line number Diff line number Diff line change
@@ -1,15 +1,18 @@
source ENV['GEM_SOURCE'] || 'https://rubygems.org'

gem 'json', '>= 1.8'
gem 'net-ldap', '<= 0.11'
gem 'rack', '>= 1.6'
gem 'rake', '>= 10.4'
gem 'rbvmomi', '>= 1.8'
gem 'redis', '>= 3.2'
gem 'sinatra', '>= 1.4'
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
16 changes: 16 additions & 0 deletions lib/vmpooler.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ module Vmpooler
require 'rbvmomi'
require 'redis'
require 'sinatra/base'
require "statsd-ruby"
require 'time'
require 'timeout'
require 'yaml'
Expand Down Expand Up @@ -52,6 +53,13 @@ def self.config(filepath='vmpooler.yaml')
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])
Expand Down Expand Up @@ -79,6 +87,14 @@ def self.new_graphite(server)
end
end

def self.new_statsd(server, port)
if server.nil? or server.empty? or server.length == 0
nil
else
Statsd.new server, port
end
end

def self.pools(conf)
conf[:pools]
end
Expand Down
3 changes: 2 additions & 1 deletion lib/vmpooler/api.rb
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,10 @@ def initialize
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

Expand Down
20 changes: 20 additions & 0 deletions lib/vmpooler/api/v1.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,16 @@ def backend
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
Expand Down Expand Up @@ -79,6 +89,12 @@ def checkout_vm(template, result)
result['ok'] = false
end

if result['ok']
statsd.increment(statsd_prefix + '.checkout_success.' + template, 1)
else
statsd.increment(statsd_prefix + '.checkout_fail.' + template, 1)
end

result
end

Expand Down Expand Up @@ -366,6 +382,10 @@ def checkout_vm(template, result)
result['domain'] = config['domain']
end

if !result['ok']
statsd.increment(statsd_prefix + '.checkout_fail.' + key, 1)
end

JSON.pretty_generate(result)
end

Expand Down
15 changes: 12 additions & 3 deletions lib/vmpooler/pool_manager.rb
Original file line number Diff line number Diff line change
@@ -1,12 +1,17 @@
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
unless statsd.nil?
Copy link
Contributor

@mckern mckern May 10, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've got more to say about this in another comment I'm working on, but nil is already falsey, so you can just use the status of something that might be nil instead of explicitly checking for it.

# No don't do this!
unless graphite.nil?

# Yes do this!
if graphite

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Word - I generally just follow what ever pattern was done before, but I'm by no means opposed to cleaning this up as we go :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer the latter approach if we can try to match general style.

$statsd = statsd
end

unless graphite.nil? || !statsd.nil?
$graphite = graphite
end

Expand Down Expand Up @@ -258,6 +263,7 @@ def clone_vm(template, folder, datastore, target)
$redis.decr('vmpooler__tasks__clone')

begin
$statsd.timing($config[:statsd]['prefix'] + ".clone.#{vm['template']}", finish) if defined? $statsd
$graphite.log($config[:graphite]['prefix'] + ".clone.#{vm['template']}", finish) if defined? $graphite
Copy link
Contributor

@mckern mckern May 10, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like you're checking on $statsd and $graphite a lot. Consider letting the initializer define both of those values (and letting one fall through to nil, and then wrapping those into a general method (this is by no means complete but if you're interested then I can help you flesh it out):

# @return [SomeKindOfObject, nil] an object representing the defined processing system for metric collection
def metric_collector
  $statsd || $graphite
end
metric_collector.increment($config[:statsd]['prefix'] + '.ready.' + pool['name'], $redis.scard('vmpooler__ready__' + pool['name']))

(Disregard the use of $config[:statsd]['prefix']; that's part of the fleshed out solution)

rescue
end
Expand Down Expand Up @@ -565,7 +571,10 @@ def _check_pool(pool)
total = $redis.scard('vmpooler__pending__' + pool['name']) + ready

begin
if defined? $graphite
if defined? $statsd
$statsd.increment($config[:statsd]['prefix'] + '.ready.' + pool['name'], $redis.scard('vmpooler__ready__' + pool['name']))
$statsd.increment($config[:statsd]['prefix'] + '.running.' + pool['name'], $redis.scard('vmpooler__running__' + pool['name']))
elsif defined? $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
Expand Down
12 changes: 12 additions & 0 deletions lib/vmpooler/statsd.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
require 'rubygems' unless defined?(Gem)
Copy link
Contributor

@mckern mckern May 10, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can go away. rubygems is defined and included by default in Ruby 1.9.1 and up.


module Vmpooler
class Statsd
def initialize(
s = 'statsd',
port = 8125
)
Copy link
Contributor

@mckern mckern May 10, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A nitpick and a must-do:

Imperative: Don't declare single letter variable names. Ever. Maybe in iterators, but definitely never in initializers.
Nitpick: The newlines in the required arguments don't help any. It can all be on one line.

@server = Statsd.new s, port
end
end
end
4 changes: 4 additions & 0 deletions spec/spec_helper.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
require 'simplecov'
SimpleCov.start do
add_filter '/spec/'
end
require 'helpers'
require 'rbvmomi'
require 'rspec'
Expand Down
62 changes: 60 additions & 2 deletions spec/vmpooler/pool_manager_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,12 @@
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') }
Expand Down Expand Up @@ -252,6 +251,65 @@
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
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(:increment).with('vmpooler.ready.pool1', 1)
expect(statsd).to receive(:increment).with('vmpooler.running.pool1', 5)
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') }
Expand Down
14 changes: 12 additions & 2 deletions vmpooler
Original file line number Diff line number Diff line change
Expand Up @@ -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!
}

Expand All @@ -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!
}

Expand Down
31 changes: 30 additions & 1 deletion vmpooler.yaml.example
Original file line number Diff line number Diff line change
Expand Up @@ -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:
#
Expand Down