From ab92eb366d623924afb29de70d5cdf5cc2754704 Mon Sep 17 00:00:00 2001 From: John O'Connor Date: Thu, 7 May 2020 15:03:16 +0100 Subject: [PATCH 01/12] (MAINT) Change Rubocop Screening Screen out redundant begin block rubocop compliance checks as these are causing vsphere.rb and pool_manager.rb to fail with differing ruby version checks. --- .rubocop.yml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/.rubocop.yml b/.rubocop.yml index ec42bff..2d50ca0 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -51,6 +51,10 @@ Metrics/ModuleLength: Style/WordArray: Enabled: false +# RedundantBegin is causing lib/pool_manager & vsphere.rb to fail in Ruby 2.5+ +Style/RedundantBegin: + Enabled: false + # Either sytnax for regex is ok Style/RegexpLiteral: Enabled: false @@ -93,4 +97,4 @@ Naming/MethodParameterName: # Standard comparisons seem more readable Style/NumericPredicate: Enabled: true - EnforcedStyle: comparison \ No newline at end of file + EnforcedStyle: comparison From c6ab52372ac7d58b3135e35f9ac4db952520e0f7 Mon Sep 17 00:00:00 2001 From: John O'Connor Date: Wed, 24 Jun 2020 17:23:19 +0100 Subject: [PATCH 02/12] (MAINT) Change redis.exists calls The mock_redis backend.exists return code has change with release 0.24. See https://github.com/sds/mock_redis/compare/v0.23.0...v0.24.0#diff-af51dcbfed678206fc95148d957ff5bf This meant that some spec tests started to fail after the new gem was released. So have changed all backend.exists to use the safer exists? method. --- lib/vmpooler/api/helpers.rb | 2 +- lib/vmpooler/api/v1.rb | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/vmpooler/api/helpers.rb b/lib/vmpooler/api/helpers.rb index 87ad5a5..1fd4325 100644 --- a/lib/vmpooler/api/helpers.rb +++ b/lib/vmpooler/api/helpers.rb @@ -13,7 +13,7 @@ module Vmpooler def valid_token?(backend) return false unless has_token? - backend.exists('vmpooler__token__' + request.env['HTTP_X_AUTH_TOKEN']) ? true : false + backend.exists?('vmpooler__token__' + request.env['HTTP_X_AUTH_TOKEN']) ? true : false end def validate_token(backend) diff --git a/lib/vmpooler/api/v1.rb b/lib/vmpooler/api/v1.rb index 29406f8..5ecb2b5 100644 --- a/lib/vmpooler/api/v1.rb +++ b/lib/vmpooler/api/v1.rb @@ -360,7 +360,7 @@ module Vmpooler request_id ||= generate_request_id result['request_id'] = request_id - if backend.exists("vmpooler__odrequest__#{request_id}") + if backend.exists?("vmpooler__odrequest__#{request_id}") result['message'] = "request_id '#{request_id}' has already been created" status 409 metrics.increment('ondemandrequest.generate.duplicaterequests') @@ -1164,7 +1164,7 @@ module Vmpooler params[:hostname] = hostname_shorten(params[:hostname], config['domain']) - if backend.exists('vmpooler__vm__' + params[:hostname]) + if backend.exists?('vmpooler__vm__' + params[:hostname]) begin jdata = JSON.parse(request.body.read) rescue StandardError @@ -1240,7 +1240,7 @@ module Vmpooler params[:hostname] = hostname_shorten(params[:hostname], config['domain']) - if ((params[:size].to_i > 0 )and (backend.exists('vmpooler__vm__' + params[:hostname]))) + if ((params[:size].to_i > 0 )and (backend.exists?('vmpooler__vm__' + params[:hostname]))) result[params[:hostname]] = {} result[params[:hostname]]['disk'] = "+#{params[:size]}gb" @@ -1263,7 +1263,7 @@ module Vmpooler params[:hostname] = hostname_shorten(params[:hostname], config['domain']) - if backend.exists('vmpooler__vm__' + params[:hostname]) + if backend.exists?('vmpooler__vm__' + params[:hostname]) result[params[:hostname]] = {} o = [('a'..'z'), ('0'..'9')].map(&:to_a).flatten From ffab7def9e01ab95a5a7188f4314c88da72da8e9 Mon Sep 17 00:00:00 2001 From: John O'Connor Date: Wed, 15 Apr 2020 11:59:10 +0100 Subject: [PATCH 03/12] (POOLER-160) Add Prometheus Stats Feeds Add a new Prometheus class as an additional stats feed along with the existing feeds. Move the metrics initialisation code into its own class and sub-class the individual metrics implementations under this. --- bin/vmpooler | 2 +- lib/vmpooler.rb | 21 +- lib/vmpooler/metrics.rb | 24 ++ lib/vmpooler/{ => metrics}/dummy_statsd.rb | 2 +- lib/vmpooler/{ => metrics}/graphite.rb | 2 +- lib/vmpooler/metrics/promstats.rb | 299 +++++++++++++++++++++ lib/vmpooler/{ => metrics}/statsd.rb | 2 +- spec/spec_helper.rb | 2 +- spec/unit/promstats_spec.rb | 247 +++++++++++++++++ 9 files changed, 585 insertions(+), 16 deletions(-) create mode 100644 lib/vmpooler/metrics.rb rename lib/vmpooler/{ => metrics}/dummy_statsd.rb (89%) rename lib/vmpooler/{ => metrics}/graphite.rb (97%) create mode 100644 lib/vmpooler/metrics/promstats.rb rename lib/vmpooler/{ => metrics}/statsd.rb (97%) create mode 100644 spec/unit/promstats_spec.rb diff --git a/bin/vmpooler b/bin/vmpooler index 3730c1f..a54c99b 100755 --- a/bin/vmpooler +++ b/bin/vmpooler @@ -11,7 +11,7 @@ redis_connection_pool_size = config[:redis]['connection_pool_size'] redis_connection_pool_timeout = config[:redis]['connection_pool_timeout'] logger_file = config[:config]['logfile'] -metrics = Vmpooler.new_metrics(config) +metrics = Vmpooler::Metrics.init(config) torun_threads = [] if ARGV.count == 0 diff --git a/lib/vmpooler.rb b/lib/vmpooler.rb index aa65ff3..b7dff93 100644 --- a/lib/vmpooler.rb +++ b/lib/vmpooler.rb @@ -15,7 +15,10 @@ module Vmpooler require 'timeout' require 'yaml' - %w[api graphite logger pool_manager statsd dummy_statsd generic_connection_pool].each do |lib| + require 'prometheus/middleware/collector' + require 'prometheus/middleware/exporter' + + %w[api metrics logger pool_manager generic_connection_pool].each do |lib| require "vmpooler/#{lib}" end @@ -103,6 +106,12 @@ module Vmpooler parsed_config[:graphite]['prefix'] = ENV['GRAPHITE_PREFIX'] if ENV['GRAPHITE_PREFIX'] parsed_config[:graphite]['port'] = string_to_int(ENV['GRAPHITE_PORT']) if ENV['GRAPHITE_PORT'] + if parsed_config.key? :prometheus +# parsed_config[:prometheus]['endpoint'] = ENV['PROMETHEUS_ENDPOINT'] if ENV['PROMETHEUS_ENDPOINT'] +# parsed_config[:prometheus]['prefix'] = ENV['PROMETHEUS_PREFIX'] if ENV['PROMETHEUS_PREFIX'] +# parsed_config[:prometheus]['metrics_prefix'] = ENV['PROMETHEUS_METRICS_PREFIX'] if ENV['PROMETHEUS_METRICS_PREFIX'] + end + parsed_config[:auth] = parsed_config[:auth] || {} if ENV['AUTH_PROVIDER'] if parsed_config.key? :auth parsed_config[:auth]['provider'] = ENV['AUTH_PROVIDER'] if ENV['AUTH_PROVIDER'] @@ -184,16 +193,6 @@ module Vmpooler Vmpooler::Logger.new logfile end - def self.new_metrics(params) - if params[:statsd] - Vmpooler::Statsd.new(params[:statsd]) - elsif params[:graphite] - Vmpooler::Graphite.new(params[:graphite]) - else - Vmpooler::DummyStatsd.new - end - end - def self.pools(conf) conf[:pools] end diff --git a/lib/vmpooler/metrics.rb b/lib/vmpooler/metrics.rb new file mode 100644 index 0000000..7d704e0 --- /dev/null +++ b/lib/vmpooler/metrics.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +module Vmpooler + class Metrics + # static class instantiate appropriate metrics object. + def self.init(params) + if params[:statsd] + metrics = Vmpooler::Statsd.new(params[:statsd]) + elsif params[:graphite] + metrics = Vmpooler::Graphite.new(params[:graphite]) + elsif params[:prometheus] + metrics = Vmpooler::Promstats.new(params[:prometheus]) + else + metrics = Vmpooler::DummyStatsd.new + end + metrics + end + end +end + +require 'vmpooler/metrics/statsd' +require 'vmpooler/metrics/dummy_statsd' +require 'vmpooler/metrics/graphite' +require 'vmpooler/metrics/promstats' diff --git a/lib/vmpooler/dummy_statsd.rb b/lib/vmpooler/metrics/dummy_statsd.rb similarity index 89% rename from lib/vmpooler/dummy_statsd.rb rename to lib/vmpooler/metrics/dummy_statsd.rb index fa23833..2cc4ac6 100644 --- a/lib/vmpooler/dummy_statsd.rb +++ b/lib/vmpooler/metrics/dummy_statsd.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module Vmpooler - class DummyStatsd + class DummyStatsd < Metrics attr_reader :server, :port, :prefix def initialize(*) diff --git a/lib/vmpooler/graphite.rb b/lib/vmpooler/metrics/graphite.rb similarity index 97% rename from lib/vmpooler/graphite.rb rename to lib/vmpooler/metrics/graphite.rb index 2b207c9..23740ed 100644 --- a/lib/vmpooler/graphite.rb +++ b/lib/vmpooler/metrics/graphite.rb @@ -3,7 +3,7 @@ require 'rubygems' unless defined?(Gem) module Vmpooler - class Graphite + class Graphite < Metrics attr_reader :server, :port, :prefix def initialize(params = {}) diff --git a/lib/vmpooler/metrics/promstats.rb b/lib/vmpooler/metrics/promstats.rb new file mode 100644 index 0000000..2b159b2 --- /dev/null +++ b/lib/vmpooler/metrics/promstats.rb @@ -0,0 +1,299 @@ +# frozen_string_literal: true + +module Vmpooler + class Promstats < Metrics + attr_reader :prefix, :endpoint, :metrics_prefix + + # Constants for Metric Types + M_COUNTER = 1 + M_GAUGE = 2 + M_SUMMARY = 3 + M_HISTOGRAM = 4 + + # Customised Bucket set to use for the Pooler clone times set to more appropriate intervals. + POOLER_TIME_BUCKETS = [1.0, 2.5, 5.0, 10.0, 20.0, 50.0, 100.0, 200.0, 500.0, 1000.0, 2000.0].freeze + # Same for Redis connection times - this is the same as the current Prometheus Default. + # https://github.com/prometheus/client_ruby/blob/master/lib/prometheus/client/histogram.rb#L14 + REDIS_CONNECT_BUCKETS = [0.005, 0.01, 0.025, 0.05, 0.1, 0.25, 0.5, 1, 2.5, 5, 10].freeze + + @p_metrics = {} + + def initialize(params = {}) + @prefix = params['prefix'] || 'vmpooler' + @metrics_prefix = params['metrics_prefix'] || 'vmpooler' + @endpoint = params['endpoint'] || '/prometheus' + + # Hmm - think this is breaking the logger ..... + @logger = params.delete('logger') || Logger.new(STDOUT) + + # Setup up prometheus registry and data structures + @prometheus = Prometheus::Client.registry + end + + # Metrics structure used to register the metrics and also translate/interpret the incoming metrics. + def vmpooler_metrics_table + { + errors: { + mtype: M_COUNTER, + docstring: 'Count of Errors for pool', + prom_metric_prefix: "#{@metrics_prefix}_errors", + metric_suffixes: { + markedasfailed: 'Timeout waiting for instance to initialise', + duplicatehostname: 'Unable to create instance due to duplicate hostname' + }, + param_labels: %i[template_name] + }, + usage: { + mtype: M_COUNTER, + docstring: 'Number of Pool Instances of this created', + prom_metric_prefix: "#{@metrics_prefix}_usage", + param_labels: %i[user poolname] + }, + user: { + # This metrics is leads to a lot of label values which is likely to challenge data storage + # on prometheus - see Best Practices: https://prometheus.io/docs/practices/naming/#labels + # So it is likely that this metric may need to be simplified or broken into a number + # of smaller metrics to capture the detail without challenging prometheus + mtype: M_COUNTER, + docstring: 'vmpooler user counters', + prom_metric_prefix: "#{@metrics_prefix}_user", + param_labels: %i[user instancex value_stream branch project job_name component_to_test poolname] + }, + checkout: { + mtype: M_COUNTER, + docstring: 'Pool Checkout counts', + prom_metric_prefix: "#{@metrics_prefix}_checkout", + metric_suffixes: { + nonresponsive: 'Checkout Failed - Non Responsive Machine', + empty: 'Checkout Failed - no machine', + success: 'Successful Checkout', + invalid: 'Checkout Failed - Invalid Template' + }, + param_labels: %i[poolname] + }, + config: { + mtype: M_COUNTER, + docstring: 'vmpooler Pool Configuration Request', + prom_metric_prefix: "#{@metrics_prefix}_config", + metric_suffixes: { invalid: 'Invalid' }, + param_labels: %i[poolname] + }, + poolreset: { + mtype: M_COUNTER, + docstring: 'Pool Reset Counter', + prom_metric_prefix: "#{@metrics_prefix}_poolreset", + metric_suffixes: { invalid: 'Invalid Pool' }, + param_labels: %i[poolname] + }, + connect: { + mtype: M_COUNTER, + docstring: 'vmpooler Connect (to vSphere)', + prom_metric_prefix: "#{@metrics_prefix}_connect", + metric_suffixes: { + open: 'Connect Succeeded', + fail: 'Connect Failed' + }, + param_labels: [] + }, + migrate_from: { + mtype: M_COUNTER, + docstring: 'vmpooler Machine Migrated from', + prom_metric_prefix: "#{@metrics_prefix}_migrate_from", + param_labels: %i[host_name] + }, + migrate_to: { + mtype: M_COUNTER, + docstring: 'vmpooler Machine Migrated to', + prom_metric_prefix: "#{@metrics_prefix}_migrate_to", + param_labels: %i[host_name] + }, + ready: { + mtype: M_GAUGE, + docstring: 'vmpooler Number of Machines in Ready State', + prom_metric_prefix: "#{@metrics_prefix}_ready", + param_labels: %i[poolname] + }, + running: { + mtype: M_GAUGE, + docstring: 'vmpooler Number of Machines Running', + prom_metric_prefix: "#{@metrics_prefix}_running", + param_labels: %i[poolname] + }, + connection_available: { + mtype: M_GAUGE, + docstring: 'vmpooler Redis Connections Available', + prom_metric_prefix: "#{@metrics_prefix}_connection_available", + param_labels: %i[type provider] + }, + time_to_ready_state: { + mtype: M_HISTOGRAM, + buckets: POOLER_TIME_BUCKETS, + docstring: 'Time taken for machine to read ready state for pool', + prom_metric_prefix: "#{@metrics_prefix}_time_to_ready_state", + param_labels: %i[poolname] + }, + migrate: { + mtype: M_HISTOGRAM, + buckets: POOLER_TIME_BUCKETS, + docstring: 'vmpooler Time taken to migrate machine for pool', + prom_metric_prefix: "#{@metrics_prefix}_migrate", + param_labels: %i[poolname] + }, + clone: { + mtype: M_HISTOGRAM, + buckets: POOLER_TIME_BUCKETS, + docstring: 'vmpooler Time taken to Clone Machine', + prom_metric_prefix: "#{@metrics_prefix}_clone", + param_labels: %i[poolname] + }, + destroy: { + mtype: M_HISTOGRAM, + buckets: POOLER_TIME_BUCKETS, + docstring: 'vmpooler Time taken to Destroy Machine', + prom_metric_prefix: "#{@metrics_prefix}_destroy", + param_labels: %i[poolname] + }, + connection_waited: { + mtype: M_HISTOGRAM, + buckets: REDIS_CONNECT_BUCKETS, + docstring: 'vmpooler Redis Connection Wait Time', + prom_metric_prefix: "#{@metrics_prefix}_connection_waited", + param_labels: %i[type provider] + } + } + end + + # Helper to add individual prom metric. + # Allow Histograms to specify the bucket size. + def add_prometheus_metric(metric_spec, name, docstring) + case metric_spec[:mtype] + when M_COUNTER + metric_class = Prometheus::Client::Counter + when M_GAUGE + metric_class = Prometheus::Client::Gauge + when M_SUMMARY + metric_class = Prometheus::Client::Summary + when M_HISTOGRAM + metric_class = Prometheus::Client::Histogram + else + raise("Unable to register metric #{name} with metric type #{metric_spec[:mtype]}") + end + + if (metric_spec[:mtype] == M_HISTOGRAM) && (metric_spec.key? :buckets) + prom_metric = metric_class.new( + name.to_sym, + docstring: docstring, + labels: metric_spec[:param_labels] + [:vmpooler_instance], + buckets: metric_spec[:buckets], + preset_labels: { vmpooler_instance: @prefix } + ) + else + prom_metric = metric_class.new( + name.to_sym, + docstring: docstring, + labels: metric_spec[:param_labels] + [:vmpooler_instance], + preset_labels: { vmpooler_instance: @prefix } + ) + end + @prometheus.register(prom_metric) + end + + # Top level method to register all the prometheus metrics. + + def setup_prometheus_metrics + @p_metrics = vmpooler_metrics_table + @p_metrics.each do |_name, metric_spec| + if metric_spec.key? :metric_suffixes + # Iterate thru the suffixes if provided to register multiple counters here. + metric_spec[:metric_suffixes].each do |metric_suffix| + add_prometheus_metric( + metric_spec, + "#{metric_spec[:prom_metric_prefix]}_#{metric_suffix[0]}", + "#{metric_spec[:docstring]} #{metric_suffix[1]}" + ) + end + else + # No Additional counter suffixes so register this as metric. + add_prometheus_metric( + metric_spec, + metric_spec[:prom_metric_prefix], + metric_spec[:docstring] + ) + end + end + end + + # locate a metric and check/interpet the sub-fields. + def find_metric(label) + sublabels = label.split('.') + metric_key = sublabels.shift.to_sym + raise("Invalid Metric #{metric_key} for #{label}") unless @p_metrics.key? metric_key + + metric = @p_metrics[metric_key].clone + + if metric.key? :metric_suffixes + metric_subkey = sublabels.shift.to_sym + raise("Invalid Metric #{metric_key}_#{metric_subkey} for #{label}") unless metric[:metric_suffixes].key? metric_subkey.to_sym + + metric[:metric_name] = "#{metric[:prom_metric_prefix]}_#{metric_subkey}" + else + metric[:metric_name] = metric[:prom_metric_prefix] + end + + # Check if we are looking for a parameter value at last element. + if metric.key? :param_labels + metric[:labels] = {} + # Special case processing here - if there is only one parameter label then make sure + # we append all of the remaining contents of the metric with "." separators to ensure + # we get full nodenames (e.g. for Migration to node operations) + if metric[:param_labels].length == 1 + metric[:labels][metric[:param_labels].first] = sublabels.join('.') + else + metric[:param_labels].reverse_each do |param_label| + metric[:labels][param_label] = sublabels.pop(1).first + end + end + end + metric + end + + # Helper to get lab metrics. + def get(label) + metric = find_metric(label) + [metric, @prometheus.get(metric[:metric_name])] + end + + # Note - Catch and log metrics failures so they can be noted, but don't interrupt vmpooler operation. + def increment(label) + begin + counter_metric, c = get(label) + c.increment(labels: counter_metric[:labels]) + rescue StandardError => e + @logger.log('s', "[!] prometheus error logging metric #{label} increment : #{e}") + end + end + + def gauge(label, value) + begin + unless value.nil? + gauge_metric, g = get(label) + g.set(value.to_i, labels: gauge_metric[:labels]) + end + rescue StandardError => e + @logger.log('s', "[!] prometheus error logging gauge #{label}, value #{value}: #{e}") + end + end + + def timing(label, duration) + begin + # https://prometheus.io/docs/practices/histograms/ + unless duration.nil? + histogram_metric, hm = get(label) + hm.observe(duration.to_f, labels: histogram_metric[:labels]) + end + rescue StandardError => e + @logger.log('s', "[!] prometheus error logging timing event label #{label}, duration #{duration}: #{e}") + end + end + end +end diff --git a/lib/vmpooler/statsd.rb b/lib/vmpooler/metrics/statsd.rb similarity index 97% rename from lib/vmpooler/statsd.rb rename to lib/vmpooler/metrics/statsd.rb index 53e9551..a942e42 100644 --- a/lib/vmpooler/statsd.rb +++ b/lib/vmpooler/metrics/statsd.rb @@ -4,7 +4,7 @@ require 'rubygems' unless defined?(Gem) require 'statsd' module Vmpooler - class Statsd + class Statsd < Metrics attr_reader :server, :port, :prefix def initialize(params = {}) diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index b213bfb..080f797 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -8,7 +8,7 @@ require 'rbvmomi' require 'rspec' require 'vmpooler' require 'redis' -require 'vmpooler/statsd' +require 'vmpooler/metrics' def project_root_dir File.dirname(File.dirname(__FILE__)) diff --git a/spec/unit/promstats_spec.rb b/spec/unit/promstats_spec.rb new file mode 100644 index 0000000..5db495d --- /dev/null +++ b/spec/unit/promstats_spec.rb @@ -0,0 +1,247 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe 'prometheus' do + logger = MockLogger.new + params = { 'prefix': 'test', 'metrics_prefix': 'mtest', 'endpoint': 'eptest' } + subject = Vmpooler::Promstats.new(logger, params) + let(:logger) { MockLogger.new } + + describe '#initialise' do + it 'returns a Metrics object' do + expect(Vmpooler::Promstats.new(logger)).to be_a(Vmpooler::Metrics) + end + end + + describe '#find_metric' do + context "Single Value Parameters" do + let!(:foo_metrics) do + { metric_suffixes: { bar: 'baz' }, + param_labels: %i[first second last] } + end + let!(:labels_hash) { { labels: { :first => nil, :second => nil, :last => nil } } } + before { subject.instance_variable_set(:@p_metrics, { foo: foo_metrics }) } + + it 'returns the metric for a given label including parsed labels' do + expect(subject.find_metric('foo.bar')).to include(metric_name: '_bar') + expect(subject.find_metric('foo.bar')).to include(foo_metrics) + expect(subject.find_metric('foo.bar')).to include(labels_hash) + end + + it 'raises an error when the given label is not present in metrics' do + expect { subject.find_metric('bogus') }.to raise_error(RuntimeError, 'Invalid Metric bogus for bogus') + end + + it 'raises an error when the given label specifies metric_suffixes but the following suffix not present in metrics' do + expect { subject.find_metric('foo.metric_suffixes.bogus') }.to raise_error(RuntimeError, 'Invalid Metric foo_metric_suffixes for foo.metric_suffixes.bogus') + end + end + + context "Node Name Handling" do + let!(:node_metrics) do + { metric_name: 'connection_to', + param_labels: %i[node] } + end + let!(:nodename_hash) { { labels: { :node => 'test.bar.net'}}} + before { subject.instance_variable_set(:@p_metrics, { connection_to: node_metrics }) } + + it 'Return final remaining fields (e.g. fqdn) in last label' do + expect(subject.find_metric('connection_to.test.bar.net')).to include(nodename_hash) + end + end + end + + context 'setup_prometheus_metrics' do + before(:all) do + Prometheus::Client.config.data_store = Prometheus::Client::DataStores::Synchronized.new + subject.setup_prometheus_metrics + end + let(:MCOUNTER) { 1 } + + describe '#setup_prometheus_metrics' do + it 'calls add_prometheus_metric for each item in list' do + Prometheus::Client.config.data_store = Prometheus::Client::DataStores::Synchronized.new + expect(subject).to receive(:add_prometheus_metric).at_least(subject.vmpooler_metrics_table.size).times + subject.setup_prometheus_metrics + end + end + + describe '#increment' do + it 'Increments checkout.nonresponsive.#{template_backend}' do + template_backend = 'test' + expect { subject.increment("checkout.nonresponsive.#{template_backend}") }.to change { + metric, po = subject.get("checkout.nonresponsive.#{template_backend}") + po.get(labels: metric[:labels]) + }.by(1) + end + it 'Increments checkout.empty. + requested' do + requested = 'test' + expect { subject.increment('checkout.empty.' + requested) }.to change { + metric, po = subject.get('checkout.empty.' + requested) + po.get(labels: metric[:labels]) + }.by(1) + end + it 'Increments checkout.success. + vmtemplate' do + vmtemplate = 'test-template' + expect { subject.increment('checkout.success.' + vmtemplate) }.to change { + metric, po = subject.get('checkout.success.' + vmtemplate) + po.get(labels: metric[:labels]) + }.by(1) + end + it 'Increments checkout.invalid. + bad_template' do + bad_template = 'test-template' + expect { subject.increment('checkout.invalid.' + bad_template) }.to change { + metric, po = subject.get('checkout.invalid.' + bad_template) + po.get(labels: metric[:labels]) + }.by(1) + end + it 'Increments checkout.invalid.unknown' do + expect { subject.increment('checkout.invalid.unknown') }.to change { + metric, po = subject.get('checkout.invalid.unknown') + po.get(labels: metric[:labels]) + }.by(1) + end + it 'Increments config.invalid.#{bad_template}' do + bad_template = 'test-template' + expect { subject.increment("config.invalid.#{bad_template}") }.to change { + metric, po = subject.get("config.invalid.#{bad_template}") + po.get(labels: metric[:labels]) + }.by(1) + end + it 'Increments config.invalid.unknown' do + expect { subject.increment('config.invalid.unknown') }.to change { + metric, po = subject.get('config.invalid.unknown') + po.get(labels: metric[:labels]) + }.by(1) + end + it 'Increments poolreset.invalid.#{bad_pool}' do + bad_pool = 'test-pool' + expect { subject.increment("poolreset.invalid.#{bad_pool}") }.to change { + metric, po = subject.get("poolreset.invalid.#{bad_pool}") + po.get(labels: metric[:labels]) + }.by(1) + end + it 'Increments poolreset.invalid.unknown' do + expect { subject.increment('poolreset.invalid.unknown') }.to change { + metric, po = subject.get('poolreset.invalid.unknown') + po.get(labels: metric[:labels]) + }.by(1) + end + it 'Increments errors.markedasfailed.#{pool}' do + pool = 'test-pool' + expect { subject.increment("errors.markedasfailed.#{pool}") }.to change { + metric, po = subject.get("errors.markedasfailed.#{pool}") + po.get(labels: metric[:labels]) + }.by(1) + end + it 'Increments errors.duplicatehostname.#{pool_name}' do + pool_name = 'test-pool' + expect { subject.increment("errors.duplicatehostname.#{pool_name}") }.to change { + metric, po = subject.get("errors.duplicatehostname.#{pool_name}") + po.get(labels: metric[:labels]) + }.by(1) + end + it 'Increments usage.#{user}.#{poolname}' do + user = 'myuser' + poolname = 'test-pool' + expect { subject.increment("usage.#{user}.#{poolname}") }.to change { + metric, po = subject.get("usage.#{user}.#{poolname}") + po.get(labels: metric[:labels]) + }.by(1) + end + it 'Increments label :user' do + # subject.increment(:user, :instance, :value_stream, :branch, :project, :job_name, :component_to_test, :poolname) - showing labels here + pending 'increment only supports a string containing a dot separator' + expect { subject.increment(:user) }.to change { + metric, po = subject.get(:user) + po.get(labels: metric[:labels]) + }.by(1) + end + it 'Increments connect.open' do + expect { subject.increment('connect.open') }.to change { + metric, po = subject.get('connect.open') + po.get(labels: metric[:labels]) + }.by(1) + end + it 'Increments connect.fail' do + expect { subject.increment('connect.fail') }.to change { + metric, po = subject.get('connect.fail') + po.get(labels: metric[:labels]) + }.by(1) + end + it 'Increments migrate_from.#{vm_hash[\'host_name\']}' do + vm_hash = { 'host_name': 'testhost.testdomain' } + expect { subject.increment("migrate_from.#{vm_hash['host_name']}") }.to change { + metric, po = subject.get("migrate_from.#{vm_hash['host_name']}") + po.get(labels: metric[:labels]) + }.by(1) + end + it 'Increments "migrate_to.#{dest_host_name}"' do + dest_host_name = 'testhost.testdomain' + expect { subject.increment("migrate_to.#{dest_host_name}") }.to change { + metric, po = subject.get("migrate_to.#{dest_host_name}") + po.get(labels: metric[:labels]) + }.by(1) + end + end + + describe '#gauge' do + # metrics.gauge("ready.#{pool_name}", $redis.scard("vmpooler__ready__#{pool_name}")) + it 'sets value of ready.#{pool_name} to $redis.scard("vmpooler__ready__#{pool_name}"))' do + # is there a specific redis value that should be tested? + pool_name = 'test-pool' + test_value = 42 + expect { subject.gauge("ready.#{pool_name}", test_value) }.to change { + metric, po = subject.get("ready.#{pool_name}") + po.get(labels: metric[:labels]) + }.from(0).to(42) + end + # metrics.gauge("running.#{pool_name}", $redis.scard("vmpooler__running__#{pool_name}")) + it 'sets value of running.#{pool_name} to $redis.scard("vmpooler__running__#{pool_name}"))' do + # is there a specific redis value that should be tested? + pool_name = 'test-pool' + test_value = 42 + expect { subject.gauge("running.#{pool_name}", test_value) }.to change { + metric, po = subject.get("running.#{pool_name}") + po.get(labels: metric[:labels]) + }.from(0).to(42) + end + end + + describe '#timing' do + it 'sets histogram value of time_to_ready_state.#{pool} to finish' do + pool = 'test-pool' + finish = 42 + expect { subject.timing("time_to_ready_state.#{pool}", finish) }.to change { + metric, po = subject.get("time_to_ready_state.#{pool}") + po.get(labels: metric[:labels]) + } + end + it 'sets histogram value of clone.#{pool} to finish' do + pool = 'test-pool' + finish = 42 + expect { subject.timing("clone.#{pool}", finish) }.to change { + metric, po = subject.get("clone.#{pool}") + po.get(labels: metric[:labels]) + } + end + it 'sets histogram value of migrate.#{pool} to finish' do + pool = 'test-pool' + finish = 42 + expect { subject.timing("migrate.#{pool}", finish) }.to change { + metric, po = subject.get("migrate.#{pool}") + po.get(labels: metric[:labels]) + } + end + it 'sets histogram value of destroy.#{pool} to finish' do + pool = 'test-pool' + finish = 42 + expect { subject.timing("destroy.#{pool}", finish) }.to change { + metric, po = subject.get("destroy.#{pool}") + po.get(labels: metric[:labels]) + } + end + end + end +end From bbd76bde4c34e347a92b8b72052cf9a8065ac4c4 Mon Sep 17 00:00:00 2001 From: John O'Connor Date: Thu, 23 Apr 2020 16:37:29 +0100 Subject: [PATCH 04/12] (POOLER-160) Add Prometheus to pooler startup This is a re-architect of the vmpooler initialisation code to: 1. Allow an API service for both manager and the api 2. Add the Prometheus endpoints to the web service. Needed to change the way the Rack Service is started as instantiating using ".New" leads to a failure to initialise the http Stats collection. 3. Selectively load the pooler api and/or Prometheus endpoints. 4. Rework API Spec tests for revised API loading. Needed to tidy up the initialisation and perform a reset! after each test to avoid "leaks" and dependencies between the tests. --- Gemfile | 1 + bin/vmpooler | 10 ++- lib/vmpooler/api.rb | 77 +++++++++++---------- lib/vmpooler/metrics/promstats.rb | 2 + spec/integration/api/v1/config_spec.rb | 12 +++- spec/integration/api/v1/ondemandvm_spec.rb | 15 ++-- spec/integration/api/v1/poolreset.rb | 9 ++- spec/integration/api/v1/status_spec.rb | 11 ++- spec/integration/api/v1/token_spec.rb | 17 +++-- spec/integration/api/v1/vm_hostname_spec.rb | 24 ++++--- spec/integration/api/v1/vm_spec.rb | 16 +++-- spec/integration/api/v1/vm_template_spec.rb | 16 +++-- spec/integration/dashboard_spec.rb | 42 ++++++++--- spec/unit/pool_manager_spec.rb | 1 - vmpooler.gemspec | 1 + 15 files changed, 166 insertions(+), 88 deletions(-) diff --git a/Gemfile b/Gemfile index e97d7cb..26eef16 100644 --- a/Gemfile +++ b/Gemfile @@ -8,6 +8,7 @@ gem 'rake', '~> 13.0' gem 'redis', '~> 4.1' gem 'rbvmomi', '~> 2.1' gem 'sinatra', '~> 2.0' +gem 'prometheus-client', '~> 2.0' gem 'net-ldap', '~> 0.16' gem 'statsd-ruby', '~> 1.4.0', :require => 'statsd' gem 'connection_pool', '~> 2.2' diff --git a/bin/vmpooler b/bin/vmpooler index a54c99b..17578ac 100755 --- a/bin/vmpooler +++ b/bin/vmpooler @@ -25,12 +25,16 @@ end if torun.include? 'api' api = Thread.new do - thr = Vmpooler::API.new redis = Vmpooler.new_redis(redis_host, redis_port, redis_password) - thr.helpers.configure(config, redis, metrics) - thr.helpers.execute! + Vmpooler::API.execute(torun, config, redis, metrics) end torun_threads << api +elsif metrics.respond_to?(:setup_prometheus_metrics) + # Run the cut down API - Prometheus Metrics only. + prometheus_only_api = Thread.new do + Vmpooler::API.execute(torun, config, nil, metrics) + end + torun_threads << prometheus_only_api end if torun.include? 'manager' diff --git a/lib/vmpooler/api.rb b/lib/vmpooler/api.rb index cf7e8ab..068f024 100644 --- a/lib/vmpooler/api.rb +++ b/lib/vmpooler/api.rb @@ -2,51 +2,52 @@ module Vmpooler class API < Sinatra::Base - def initialize - super - end - - not_found do - content_type :json - - result = { - ok: false - } - - JSON.pretty_generate(result) - end - - # Load dashboard components - begin - require 'dashboard' - rescue LoadError - require File.expand_path(File.join(File.dirname(__FILE__), 'dashboard')) - end - - use Vmpooler::Dashboard - # Load API components - %w[helpers dashboard reroute v1].each do |lib| - begin - require "api/#{lib}" - rescue LoadError - require File.expand_path(File.join(File.dirname(__FILE__), 'api', lib)) - end + %w[helpers dashboard reroute v1 request_logger].each do |lib| + require "vmpooler/api/#{lib}" end + # Load dashboard components + require 'vmpooler/dashboard' - use Vmpooler::API::Dashboard - use Vmpooler::API::Reroute - use Vmpooler::API::V1 - - def configure(config, redis, metrics) + def self.execute(torun, config, redis, metrics) self.settings.set :config, config - self.settings.set :redis, redis + self.settings.set :redis, redis unless redis.nil? self.settings.set :metrics, metrics self.settings.set :checkoutlock, Mutex.new - end - def execute! - self.settings.run! + # Deflating in all situations + # https://www.schneems.com/2017/11/08/80-smaller-rails-footprint-with-rack-deflate/ + use Rack::Deflater + + # not_found clause placed here to fix rspec test issue. + not_found do + content_type :json + + result = { + ok: false + } + + JSON.pretty_generate(result) + end + + if metrics.respond_to?(:setup_prometheus_metrics) + # Prometheus metrics are only setup if actually specified + # in the config file. + metrics.setup_prometheus_metrics + + use Prometheus::Middleware::Collector, metrics_prefix: "#{metrics.metrics_prefix}_http" + use Prometheus::Middleware::Exporter, path: metrics.endpoint + end + + if torun.include? 'api' + use Vmpooler::Dashboard + use Vmpooler::API::Dashboard + use Vmpooler::API::Reroute + use Vmpooler::API::V1 + end + + # Get thee started O WebServer + self.run! end end end diff --git a/lib/vmpooler/metrics/promstats.rb b/lib/vmpooler/metrics/promstats.rb index 2b159b2..b5bd657 100644 --- a/lib/vmpooler/metrics/promstats.rb +++ b/lib/vmpooler/metrics/promstats.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +require 'prometheus/client' + module Vmpooler class Promstats < Metrics attr_reader :prefix, :endpoint, :metrics_prefix diff --git a/spec/integration/api/v1/config_spec.rb b/spec/integration/api/v1/config_spec.rb index 91008a4..7462307 100644 --- a/spec/integration/api/v1/config_spec.rb +++ b/spec/integration/api/v1/config_spec.rb @@ -8,6 +8,13 @@ describe Vmpooler::API::V1 do Vmpooler::API end + # Added to ensure no leakage in rack state from previous tests. + # Removes all routes, filters, middleware and extension hooks from the current class + # https://rubydoc.info/gems/sinatra/Sinatra/Base#reset!-class_method + before(:each) do + app.reset! + end + let(:config) { { config: { @@ -32,9 +39,8 @@ describe Vmpooler::API::V1 do let(:current_time) { Time.now } before(:each) do - app.settings.set :config, config - app.settings.set :redis, redis - app.settings.set :metrics, metrics + expect(app).to receive(:run!).once + app.execute(['api'], config, redis, metrics) app.settings.set :config, auth: false create_token('abcdefghijklmnopqrstuvwxyz012345', 'jdoe', current_time) end diff --git a/spec/integration/api/v1/ondemandvm_spec.rb b/spec/integration/api/v1/ondemandvm_spec.rb index 0665973..f9b31b2 100644 --- a/spec/integration/api/v1/ondemandvm_spec.rb +++ b/spec/integration/api/v1/ondemandvm_spec.rb @@ -5,7 +5,14 @@ describe Vmpooler::API::V1 do include Rack::Test::Methods def app() - Vmpooler::API end + Vmpooler::API + end + # Added to ensure no leakage in rack state from previous tests. + # Removes all routes, filters, middleware and extension hooks from the current class + # https://rubydoc.info/gems/sinatra/Sinatra/Base#reset!-class_method + before(:each) do + app.reset! + end describe '/ondemandvm' do let(:prefix) { '/api/v1' } @@ -32,13 +39,11 @@ describe Vmpooler::API::V1 do let(:current_time) { Time.now } let(:vmname) { 'abcdefghijkl' } let(:checkoutlock) { Mutex.new } - let(:redis) { MockRedis.new } let(:uuid) { SecureRandom.uuid } before(:each) do - app.settings.set :config, config - app.settings.set :redis, redis - app.settings.set :metrics, metrics + expect(app).to receive(:run!).once + app.execute(['api'], config, redis, metrics) app.settings.set :config, auth: false app.settings.set :checkoutlock, checkoutlock create_token('abcdefghijklmnopqrstuvwxyz012345', 'jdoe', current_time) diff --git a/spec/integration/api/v1/poolreset.rb b/spec/integration/api/v1/poolreset.rb index 9c87c6c..043989e 100644 --- a/spec/integration/api/v1/poolreset.rb +++ b/spec/integration/api/v1/poolreset.rb @@ -8,6 +8,10 @@ describe Vmpooler::API::V1 do Vmpooler::API end + after(:each) do + Vmpooler::API.reset! + end + let(:config) { { config: { @@ -32,9 +36,8 @@ describe Vmpooler::API::V1 do let(:current_time) { Time.now } before(:each) do - app.settings.set :config, config - app.settings.set :redis, redis - app.settings.set :metrics, metrics + expect(app).to receive(:run!).once + app.execute(['api'], config, redis, metrics) app.settings.set :config, auth: false create_token('abcdefghijklmnopqrstuvwxyz012345', 'jdoe', current_time) end diff --git a/spec/integration/api/v1/status_spec.rb b/spec/integration/api/v1/status_spec.rb index 10f127f..486c263 100644 --- a/spec/integration/api/v1/status_spec.rb +++ b/spec/integration/api/v1/status_spec.rb @@ -12,6 +12,13 @@ describe Vmpooler::API::V1 do Vmpooler::API end + # Added to ensure no leakage in rack state from previous tests. + # Removes all routes, filters, middleware and extension hooks from the current class + # https://rubydoc.info/gems/sinatra/Sinatra/Base#reset!-class_method + before(:each) do + app.reset! + end + describe 'status and metrics endpoints' do let(:prefix) { '/api/v1' } @@ -32,8 +39,8 @@ describe Vmpooler::API::V1 do let(:current_time) { Time.now } before(:each) do - app.settings.set :config, config - app.settings.set :redis, redis + expect(app).to receive(:run!).once + app.execute(['api'], config, redis, nil) app.settings.set :config, auth: false create_token('abcdefghijklmnopqrstuvwxyz012345', 'jdoe', current_time) end diff --git a/spec/integration/api/v1/token_spec.rb b/spec/integration/api/v1/token_spec.rb index 983dac6..72e60bd 100644 --- a/spec/integration/api/v1/token_spec.rb +++ b/spec/integration/api/v1/token_spec.rb @@ -8,14 +8,21 @@ describe Vmpooler::API::V1 do Vmpooler::API end + # Added to ensure no leakage in rack state from previous tests. + # Removes all routes, filters, middleware and extension hooks from the current class + # https://rubydoc.info/gems/sinatra/Sinatra/Base#reset!-class_method + before(:each) do + app.reset! + end + describe '/token' do let(:prefix) { '/api/v1' } let(:current_time) { Time.now } let(:config) { { } } - before do - app.settings.set :config, config - app.settings.set :redis, redis + before(:each) do + expect(app).to receive(:run!).once + app.execute(['api'], config, redis, nil) end describe 'GET /token' do @@ -97,7 +104,9 @@ describe Vmpooler::API::V1 do let(:prefix) { '/api/v1' } let(:current_time) { Time.now } - before do + before(:each) do + expect(app).to receive(:run!).once + app.execute(['api'], config, redis, nil) app.settings.set :config, config app.settings.set :redis, redis end diff --git a/spec/integration/api/v1/vm_hostname_spec.rb b/spec/integration/api/v1/vm_hostname_spec.rb index bca9866..18fcec3 100644 --- a/spec/integration/api/v1/vm_hostname_spec.rb +++ b/spec/integration/api/v1/vm_hostname_spec.rb @@ -12,8 +12,16 @@ describe Vmpooler::API::V1 do Vmpooler::API end + # Added to ensure no leakage in rack state from previous tests. + # Removes all routes, filters, middleware and extension hooks from the current class + # https://rubydoc.info/gems/sinatra/Sinatra/Base#reset!-class_method + before(:each) do + app.reset! + end + describe '/vm/:hostname' do let(:prefix) { '/api/v1' } + let(:metrics) { Vmpooler::DummyStatsd.new } let(:config) { { @@ -33,11 +41,9 @@ describe Vmpooler::API::V1 do let(:current_time) { Time.now } - let(:redis) { MockRedis.new } - before(:each) do - app.settings.set :config, config - app.settings.set :redis, redis + expect(app).to receive(:run!).once + app.execute(['api'], config, redis, metrics) create_token('abcdefghijklmnopqrstuvwxyz012345', 'jdoe', current_time) end @@ -81,9 +87,9 @@ describe Vmpooler::API::V1 do end context '(tagfilter configured)' do - let(:config) { { - tagfilter: { 'url' => '(.*)\/' }, - } } + before(:each) do + app.settings.set :config, tagfilter: { 'url' => '(.*)\/' } + end it 'correctly filters tags' do create_vm('testhost', redis) @@ -104,7 +110,9 @@ describe Vmpooler::API::V1 do end context '(auth not configured)' do - let(:config) { { auth: false } } + before(:each) do + app.settings.set :config, auth: false + end it 'allows VM lifetime to be modified without a token' do create_vm('testhost', redis) diff --git a/spec/integration/api/v1/vm_spec.rb b/spec/integration/api/v1/vm_spec.rb index 8e3799b..7f5c29a 100644 --- a/spec/integration/api/v1/vm_spec.rb +++ b/spec/integration/api/v1/vm_spec.rb @@ -8,6 +8,13 @@ describe Vmpooler::API::V1 do Vmpooler::API end + # Added to ensure no leakage in rack state from previous tests. + # Removes all routes, filters, middleware and extension hooks from the current class + # https://rubydoc.info/gems/sinatra/Sinatra/Base#reset!-class_method + before(:each) do + app.reset! + end + describe '/vm' do let(:prefix) { '/api/v1' } let(:metrics) { Vmpooler::DummyStatsd.new } @@ -32,9 +39,8 @@ describe Vmpooler::API::V1 do let(:checkoutlock) { Mutex.new } before(:each) do - app.settings.set :config, config - app.settings.set :redis, redis - app.settings.set :metrics, metrics + expect(app).to receive(:run!).once + app.execute(['api'], config, redis, metrics) app.settings.set :config, auth: false app.settings.set :checkoutlock, checkoutlock create_token('abcdefghijklmnopqrstuvwxyz012345', 'jdoe', current_time) @@ -104,8 +110,8 @@ describe Vmpooler::API::V1 do end it 'returns 503 for empty pool when aliases are not defined' do - Vmpooler::API.settings.config.delete(:alias) - Vmpooler::API.settings.config[:pool_names] = ['pool1', 'pool2'] + app.settings.config.delete(:alias) + app.settings.config[:pool_names] = ['pool1', 'pool2'] create_ready_vm 'pool1', vmname, redis post "#{prefix}/vm/pool1" diff --git a/spec/integration/api/v1/vm_template_spec.rb b/spec/integration/api/v1/vm_template_spec.rb index 043c8e5..da83116 100644 --- a/spec/integration/api/v1/vm_template_spec.rb +++ b/spec/integration/api/v1/vm_template_spec.rb @@ -8,6 +8,13 @@ describe Vmpooler::API::V1 do Vmpooler::API end + # Added to ensure no leakage in rack state from previous tests. + # Removes all routes, filters, middleware and extension hooks from the current class + # https://rubydoc.info/gems/sinatra/Sinatra/Base#reset!-class_method + before(:each) do + app.reset! + end + describe '/vm/:template' do let(:prefix) { '/api/v1' } let(:metrics) { Vmpooler::DummyStatsd.new } @@ -33,9 +40,8 @@ describe Vmpooler::API::V1 do let(:checkoutlock) { Mutex.new } before(:each) do - app.settings.set :config, config - app.settings.set :redis, redis - app.settings.set :metrics, metrics + expect(app).to receive(:run!).once + app.execute(['api'], config, redis, metrics) app.settings.set :config, auth: false app.settings.set :checkoutlock, checkoutlock create_token('abcdefghijklmnopqrstuvwxyz012345', 'jdoe', current_time) @@ -84,8 +90,8 @@ describe Vmpooler::API::V1 do end it 'returns 503 for empty pool when aliases are not defined' do - Vmpooler::API.settings.config.delete(:alias) - Vmpooler::API.settings.config[:pool_names] = ['pool1', 'pool2'] + app.settings.config.delete(:alias) + app.settings.config[:pool_names] = ['pool1', 'pool2'] create_ready_vm 'pool1', 'abcdefghijklmnop', redis post "#{prefix}/vm/pool1" diff --git a/spec/integration/dashboard_spec.rb b/spec/integration/dashboard_spec.rb index 9d5a64a..32929b7 100644 --- a/spec/integration/dashboard_spec.rb +++ b/spec/integration/dashboard_spec.rb @@ -5,13 +5,35 @@ describe Vmpooler::API do include Rack::Test::Methods def app() - described_class + Vmpooler::API + end + + # Added to ensure no leakage in rack state from previous tests. + # Removes all routes, filters, middleware and extension hooks from the current class + # https://rubydoc.info/gems/sinatra/Sinatra/Base#reset!-class_method + before(:each) do + app.reset! end describe 'Dashboard' do + let(:config) { { + pools: [ + {'name' => 'pool1', 'size' => 5} + ], + graphite: {} + } } + + before(:each) do + expect(app).to receive(:run!) + app.execute(['api'], config, redis, nil) + app.settings.set :config, auth: false + end + context '/' do - before { get '/' } + before(:each) do + get '/' + end it { expect(last_response.status).to eq(302) } it { expect(last_response.location).to eq('http://example.org/dashboard/') } @@ -21,7 +43,7 @@ describe Vmpooler::API do ENV['SITE_NAME'] = 'test pooler' ENV['VMPOOLER_CONFIG'] = 'thing' - before do + before(:each) do get '/dashboard/' end @@ -31,10 +53,12 @@ describe Vmpooler::API do end context 'unknown route' do - before { get '/doesnotexist' } + before(:each) do + get '/doesnotexist' + end - it { expect(last_response.status).to eq(404) } it { expect(last_response.header['Content-Type']).to eq('application/json') } + it { expect(last_response.status).to eq(404) } it { expect(last_response.body).to eq(JSON.pretty_generate({ok: false})) } end @@ -47,10 +71,8 @@ describe Vmpooler::API do graphite: {} } } - before do - $config = config + before(:each) do app.settings.set :config, config - app.settings.set :redis, redis app.settings.set :environment, :test end @@ -120,10 +142,8 @@ describe Vmpooler::API do graphite: {} } } - before do - $config = config + before(:each) do app.settings.set :config, config - app.settings.set :redis, redis app.settings.set :environment, :test end diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb index 8e75f45..88688d6 100644 --- a/spec/unit/pool_manager_spec.rb +++ b/spec/unit/pool_manager_spec.rb @@ -27,7 +27,6 @@ describe 'Pool Manager' do timeout: 5 ) { MockRedis.new } } - let(:redis) { MockRedis.new } let(:provider) { Vmpooler::PoolManager::Provider::Base.new(config, logger, metrics, redis_connection_pool, 'mock_provider', provider_options) } diff --git a/vmpooler.gemspec b/vmpooler.gemspec index b0c5ff2..f10e18f 100644 --- a/vmpooler.gemspec +++ b/vmpooler.gemspec @@ -24,6 +24,7 @@ Gem::Specification.new do |s| s.add_dependency 'redis', '~> 4.1' s.add_dependency 'rbvmomi', '~> 2.1' s.add_dependency 'sinatra', '~> 2.0' + s.add_dependency 'prometheus-client', '~> 2.0' s.add_dependency 'net-ldap', '~> 0.16' s.add_dependency 'statsd-ruby', '~> 1.4' s.add_dependency 'connection_pool', '~> 2.2' From 5c38ba240a82bd9d9020c7d068eb706b6e15d917 Mon Sep 17 00:00:00 2001 From: John O'Connor Date: Fri, 22 May 2020 17:07:33 +0100 Subject: [PATCH 05/12] (POOLER-160) Revise some smelly logger code Need the logger code in promstats.rb so move logger initialisation to the top of the vmpooler script, remove the class method from vmpooler.rb and make appropriate downstream changes to metrics and pooler manger handling. This also means we can start decent logging in the API if we wish do. Fixed up to rebase on top of Matts POOLER-158 changes --- bin/vmpooler | 5 +++-- lib/vmpooler.rb | 10 ---------- lib/vmpooler/metrics.rb | 8 ++++---- lib/vmpooler/metrics/graphite.rb | 5 +++-- lib/vmpooler/metrics/promstats.rb | 6 ++---- lib/vmpooler/metrics/statsd.rb | 9 +++++---- 6 files changed, 17 insertions(+), 26 deletions(-) diff --git a/bin/vmpooler b/bin/vmpooler index 17578ac..214480e 100755 --- a/bin/vmpooler +++ b/bin/vmpooler @@ -11,7 +11,8 @@ redis_connection_pool_size = config[:redis]['connection_pool_size'] redis_connection_pool_timeout = config[:redis]['connection_pool_timeout'] logger_file = config[:config]['logfile'] -metrics = Vmpooler::Metrics.init(config) +logger = Vmpooler::Logger.new logger_file +metrics = Vmpooler::Metrics.init(logger, config) torun_threads = [] if ARGV.count == 0 @@ -41,7 +42,7 @@ if torun.include? 'manager' manager = Thread.new do Vmpooler::PoolManager.new( config, - Vmpooler.new_logger(logger_file), + logger, Vmpooler.redis_connection_pool(redis_host, redis_port, redis_password, redis_connection_pool_size, redis_connection_pool_timeout, metrics), metrics ).execute! diff --git a/lib/vmpooler.rb b/lib/vmpooler.rb index b7dff93..4c12362 100644 --- a/lib/vmpooler.rb +++ b/lib/vmpooler.rb @@ -106,12 +106,6 @@ module Vmpooler parsed_config[:graphite]['prefix'] = ENV['GRAPHITE_PREFIX'] if ENV['GRAPHITE_PREFIX'] parsed_config[:graphite]['port'] = string_to_int(ENV['GRAPHITE_PORT']) if ENV['GRAPHITE_PORT'] - if parsed_config.key? :prometheus -# parsed_config[:prometheus]['endpoint'] = ENV['PROMETHEUS_ENDPOINT'] if ENV['PROMETHEUS_ENDPOINT'] -# parsed_config[:prometheus]['prefix'] = ENV['PROMETHEUS_PREFIX'] if ENV['PROMETHEUS_PREFIX'] -# parsed_config[:prometheus]['metrics_prefix'] = ENV['PROMETHEUS_METRICS_PREFIX'] if ENV['PROMETHEUS_METRICS_PREFIX'] - end - parsed_config[:auth] = parsed_config[:auth] || {} if ENV['AUTH_PROVIDER'] if parsed_config.key? :auth parsed_config[:auth]['provider'] = ENV['AUTH_PROVIDER'] if ENV['AUTH_PROVIDER'] @@ -189,10 +183,6 @@ module Vmpooler Redis.new(host: host, port: port, password: password) end - def self.new_logger(logfile) - Vmpooler::Logger.new logfile - end - def self.pools(conf) conf[:pools] end diff --git a/lib/vmpooler/metrics.rb b/lib/vmpooler/metrics.rb index 7d704e0..241dc18 100644 --- a/lib/vmpooler/metrics.rb +++ b/lib/vmpooler/metrics.rb @@ -3,13 +3,13 @@ module Vmpooler class Metrics # static class instantiate appropriate metrics object. - def self.init(params) + def self.init(logger, params) if params[:statsd] - metrics = Vmpooler::Statsd.new(params[:statsd]) + metrics = Vmpooler::Statsd.new(logger, params[:statsd]) elsif params[:graphite] - metrics = Vmpooler::Graphite.new(params[:graphite]) + metrics = Vmpooler::Graphite.new(logger, params[:graphite]) elsif params[:prometheus] - metrics = Vmpooler::Promstats.new(params[:prometheus]) + metrics = Vmpooler::Promstats.new(logger, params[:prometheus]) else metrics = Vmpooler::DummyStatsd.new end diff --git a/lib/vmpooler/metrics/graphite.rb b/lib/vmpooler/metrics/graphite.rb index 23740ed..e94e77a 100644 --- a/lib/vmpooler/metrics/graphite.rb +++ b/lib/vmpooler/metrics/graphite.rb @@ -6,12 +6,13 @@ module Vmpooler class Graphite < Metrics attr_reader :server, :port, :prefix - def initialize(params = {}) + def initialize(logger, params = {}) raise ArgumentError, "Graphite server is required. Config: #{params.inspect}" if params['server'].nil? || params['server'].empty? @server = params['server'] @port = params['port'] || 2003 @prefix = params['prefix'] || 'vmpooler' + @logger = logger end def increment(label) @@ -38,7 +39,7 @@ module Vmpooler rescue Errno::EADDRNOTAVAIL => e warn "Could not assign address to graphite server #{server}: #{e}" rescue StandardError => e - warn "Failure logging #{path} to graphite server [#{server}:#{port}]: #{e}" + @logger.log('s', "[!] Failure logging #{path} to graphite server [#{server}:#{port}]: #{e}") end end end diff --git a/lib/vmpooler/metrics/promstats.rb b/lib/vmpooler/metrics/promstats.rb index b5bd657..86ca64c 100644 --- a/lib/vmpooler/metrics/promstats.rb +++ b/lib/vmpooler/metrics/promstats.rb @@ -20,13 +20,11 @@ module Vmpooler @p_metrics = {} - def initialize(params = {}) + def initialize(logger, params = {}) @prefix = params['prefix'] || 'vmpooler' @metrics_prefix = params['metrics_prefix'] || 'vmpooler' @endpoint = params['endpoint'] || '/prometheus' - - # Hmm - think this is breaking the logger ..... - @logger = params.delete('logger') || Logger.new(STDOUT) + @logger = logger # Setup up prometheus registry and data structures @prometheus = Prometheus::Client.registry diff --git a/lib/vmpooler/metrics/statsd.rb b/lib/vmpooler/metrics/statsd.rb index a942e42..def7b58 100644 --- a/lib/vmpooler/metrics/statsd.rb +++ b/lib/vmpooler/metrics/statsd.rb @@ -7,31 +7,32 @@ module Vmpooler class Statsd < Metrics attr_reader :server, :port, :prefix - def initialize(params = {}) + def initialize(logger, params = {}) raise ArgumentError, "Statsd server is required. Config: #{params.inspect}" if params['server'].nil? || params['server'].empty? host = params['server'] @port = params['port'] || 8125 @prefix = params['prefix'] || 'vmpooler' @server = ::Statsd.new(host, @port) + @logger = logger end def increment(label) server.increment(prefix + '.' + label) rescue StandardError => e - warn "Failure incrementing #{prefix}.#{label} on statsd server [#{server}:#{port}]: #{e}" + @logger.log('s', "[!] Failure incrementing #{prefix}.#{label} on statsd server [#{server}:#{port}]: #{e}") end def gauge(label, value) server.gauge(prefix + '.' + label, value) rescue StandardError => e - warn "Failure updating gauge #{prefix}.#{label} on statsd server [#{server}:#{port}]: #{e}" + @logger.log('s', "[!] Failure updating gauge #{prefix}.#{label} on statsd server [#{server}:#{port}]: #{e}") end def timing(label, duration) server.timing(prefix + '.' + label, duration) rescue StandardError => e - warn "Failure updating timing #{prefix}.#{label} on statsd server [#{server}:#{port}]: #{e}" + @logger.log('s', "[!] Failure updating timing #{prefix}.#{label} on statsd server [#{server}:#{port}]: #{e}") end end end From 72564de4b42821c88d1a3486d1c8d096892a871f Mon Sep 17 00:00:00 2001 From: John O'Connor Date: Fri, 22 May 2020 19:21:15 +0100 Subject: [PATCH 06/12] (POOLER-160) Revise connection metrics The redis pooler connection metric used "metric_prefix" which is misleading, so split this into connpool_type and connpool_provider. Also remove some earlier jruby compatibility code to reduce rebase conflicts when this is rebased on top of Matt's changes. --- lib/vmpooler.rb | 3 ++- lib/vmpooler/generic_connection_pool.rb | 12 ++++++---- lib/vmpooler/providers/dummy.rb | 3 ++- lib/vmpooler/providers/vsphere.rb | 3 ++- spec/unit/generic_connection_pool_spec.rb | 28 ++++++++++++----------- spec/unit/pool_manager_spec.rb | 3 ++- spec/unit/providers/vsphere_spec.rb | 5 ++-- 7 files changed, 33 insertions(+), 24 deletions(-) diff --git a/lib/vmpooler.rb b/lib/vmpooler.rb index 4c12362..98b4bd6 100644 --- a/lib/vmpooler.rb +++ b/lib/vmpooler.rb @@ -169,7 +169,8 @@ module Vmpooler def self.redis_connection_pool(host, port, password, size, timeout, metrics) Vmpooler::PoolManager::GenericConnectionPool.new( metrics: metrics, - metric_prefix: 'redis_connection_pool', + connpool_type: 'redis_connection_pool', + connpool_provider: 'manager', size: size, timeout: timeout ) do diff --git a/lib/vmpooler/generic_connection_pool.rb b/lib/vmpooler/generic_connection_pool.rb index 9e9fc0c..5299b23 100644 --- a/lib/vmpooler/generic_connection_pool.rb +++ b/lib/vmpooler/generic_connection_pool.rb @@ -11,8 +11,10 @@ module Vmpooler def initialize(options = {}, &block) super(options, &block) @metrics = options[:metrics] - @metric_prefix = options[:metric_prefix] - @metric_prefix = 'connectionpool' if @metric_prefix.nil? || @metric_prefix == '' + @connpool_type = options[:connpool_type] + @connpool_type = 'connectionpool' if @connpool_type.nil? || @connpool_type == '' + @connpool_provider = options[:connpool_provider] + @connpool_provider = 'unknown' if @connpool_provider.nil? || @connpool_provider == '' end def with_metrics(options = {}) @@ -20,15 +22,15 @@ module Vmpooler start = Time.now conn = checkout(options) timespan_ms = ((Time.now - start) * 1000).to_i - @metrics&.gauge(@metric_prefix + '.available', @available.length) - @metrics&.timing(@metric_prefix + '.waited', timespan_ms) + @metrics&.gauge("connection_available.#{@connpool_type}.#{@connpool_provider}", @available.length) + @metrics&.timing("connection_waited.#{@connpool_type}.#{@connpool_provider}", timespan_ms) begin Thread.handle_interrupt(Exception => :immediate) do yield conn end ensure checkin - @metrics&.gauge(@metric_prefix + '.available', @available.length) + @metrics&.gauge("connection_available.#{@connpool_type}.#{@connpool_provider}", @available.length) end end end diff --git a/lib/vmpooler/providers/dummy.rb b/lib/vmpooler/providers/dummy.rb index 100bcb6..c4ea7cc 100644 --- a/lib/vmpooler/providers/dummy.rb +++ b/lib/vmpooler/providers/dummy.rb @@ -29,7 +29,8 @@ module Vmpooler logger.log('d', "[#{name}] ConnPool - Creating a connection pool of size #{connpool_size} with timeout #{connpool_timeout}") @connection_pool = Vmpooler::PoolManager::GenericConnectionPool.new( metrics: metrics, - metric_prefix: "#{name}_provider_connection_pool", + connpool_type: 'provider_connection_pool', + connpool_provider: name, size: connpool_size, timeout: connpool_timeout ) do diff --git a/lib/vmpooler/providers/vsphere.rb b/lib/vmpooler/providers/vsphere.rb index 79cee25..63373ec 100644 --- a/lib/vmpooler/providers/vsphere.rb +++ b/lib/vmpooler/providers/vsphere.rb @@ -25,7 +25,8 @@ module Vmpooler logger.log('d', "[#{name}] ConnPool - Creating a connection pool of size #{connpool_size} with timeout #{connpool_timeout}") @connection_pool = Vmpooler::PoolManager::GenericConnectionPool.new( metrics: metrics, - metric_prefix: "#{name}_provider_connection_pool", + connpool_type: 'provider_connection_pool', + connpool_provider: name, size: connpool_size, timeout: connpool_timeout ) do diff --git a/spec/unit/generic_connection_pool_spec.rb b/spec/unit/generic_connection_pool_spec.rb index ff57472..350e6f5 100644 --- a/spec/unit/generic_connection_pool_spec.rb +++ b/spec/unit/generic_connection_pool_spec.rb @@ -2,15 +2,17 @@ require 'spec_helper' describe 'GenericConnectionPool' do let(:metrics) { Vmpooler::DummyStatsd.new } - let(:metric_prefix) { 'prefix' } - let(:default_metric_prefix) { 'connectionpool' } + let(:connpool_type) { 'test_connection_pool' } + let(:connpool_provider) { 'testprovider' } + let(:default_connpool_type) { 'connectionpool' } let(:connection_object) { double('connection') } let(:pool_size) { 1 } let(:pool_timeout) { 1 } subject { Vmpooler::PoolManager::GenericConnectionPool.new( metrics: metrics, - metric_prefix: metric_prefix, + connpool_type: connpool_type, + connpool_provider: connpool_provider, size: pool_size, timeout: pool_timeout ) { connection_object } @@ -65,7 +67,7 @@ describe 'GenericConnectionPool' do context 'When metrics are configured' do it 'should emit a gauge metric when the connection is grabbed and released' do - expect(metrics).to receive(:gauge).with(/\.available/,Integer).exactly(2).times + expect(metrics).to receive(:gauge).with(/connection_available/,Integer).exactly(2).times subject.with_metrics do |conn1| # do nothing @@ -73,7 +75,7 @@ describe 'GenericConnectionPool' do end it 'should emit a timing metric when the connection is grabbed' do - expect(metrics).to receive(:timing).with(/\.waited/,Integer).exactly(1).times + expect(metrics).to receive(:timing).with(/connection_waited/,Integer).exactly(1).times subject.with_metrics do |conn1| # do nothing @@ -81,8 +83,8 @@ describe 'GenericConnectionPool' do end it 'should emit metrics with the specified prefix' do - expect(metrics).to receive(:gauge).with(/#{metric_prefix}\./,Integer).at_least(1).times - expect(metrics).to receive(:timing).with(/#{metric_prefix}\./,Integer).at_least(1).times + expect(metrics).to receive(:gauge).with(/#{connpool_type}\./,Integer).at_least(1).times + expect(metrics).to receive(:timing).with(/#{connpool_type}\./,Integer).at_least(1).times subject.with_metrics do |conn1| # do nothing @@ -90,11 +92,11 @@ describe 'GenericConnectionPool' do end context 'Metrix prefix is missing' do - let(:metric_prefix) { nil } + let(:connpool_type) { nil } it 'should emit metrics with default prefix' do - expect(metrics).to receive(:gauge).with(/#{default_metric_prefix}\./,Integer).at_least(1).times - expect(metrics).to receive(:timing).with(/#{default_metric_prefix}\./,Integer).at_least(1).times + expect(metrics).to receive(:gauge).with(/#{default_connpool_type}\./,Integer).at_least(1).times + expect(metrics).to receive(:timing).with(/#{default_connpool_type}\./,Integer).at_least(1).times subject.with_metrics do |conn1| # do nothing @@ -103,11 +105,11 @@ describe 'GenericConnectionPool' do end context 'Metrix prefix is empty' do - let(:metric_prefix) { '' } + let(:connpool_type) { '' } it 'should emit metrics with default prefix' do - expect(metrics).to receive(:gauge).with(/#{default_metric_prefix}\./,Integer).at_least(1).times - expect(metrics).to receive(:timing).with(/#{default_metric_prefix}\./,Integer).at_least(1).times + expect(metrics).to receive(:gauge).with(/#{default_connpool_type}\./,Integer).at_least(1).times + expect(metrics).to receive(:timing).with(/#{default_connpool_type}\./,Integer).at_least(1).times subject.with_metrics do |conn1| # do nothing diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb index 88688d6..efe6b0e 100644 --- a/spec/unit/pool_manager_spec.rb +++ b/spec/unit/pool_manager_spec.rb @@ -22,7 +22,8 @@ describe 'Pool Manager' do let(:provider_options) { {} } let(:redis_connection_pool) { Vmpooler::PoolManager::GenericConnectionPool.new( metrics: metrics, - metric_prefix: 'redis_connection_pool', + connpool_type: 'redis_connection_pool', + connpool_provider: 'testprovider', size: 1, timeout: 5 ) { MockRedis.new } diff --git a/spec/unit/providers/vsphere_spec.rb b/spec/unit/providers/vsphere_spec.rb index f8514b5..90146b9 100644 --- a/spec/unit/providers/vsphere_spec.rb +++ b/spec/unit/providers/vsphere_spec.rb @@ -79,7 +79,8 @@ EOT let(:vmname) { 'vm1' } let(:redis_connection_pool) { Vmpooler::PoolManager::GenericConnectionPool.new( metrics: metrics, - metric_prefix: 'redis_connection_pool', + connpool_type: 'redis_connection_pool', + connpool_provider: 'testprovider', size: 1, timeout: 5 ) { MockRedis.new } @@ -167,7 +168,7 @@ EOT end it 'should record metrics' do - expect(metrics).to receive(:timing).with('redis_connection_pool.waited', 0) + expect(metrics).to receive(:timing).with('connection_waited.redis_connection_pool.testprovider', 0) expect(metrics).to receive(:timing).with("destroy.#{pool}", finish) subject.destroy_vm_and_log(vmname, vm_object, pool, data_ttl) From b6dcd772282045633efe223b7d0932937c13c1f7 Mon Sep 17 00:00:00 2001 From: John O'Connor Date: Tue, 16 Jun 2020 17:11:33 +0100 Subject: [PATCH 07/12] (POOLER-170) Revise vmpooler usage stats Break down the usage stats into smaller groups so as to manage the number of stat lines collected for Prometheus. This may need some further revision to filter out Litmus stats, or otherwise collect litmus usage information. --- lib/vmpooler/metrics/promstats.rb | 30 ++++++---- lib/vmpooler/pool_manager.rb | 34 ++++------- spec/unit/pool_manager_spec.rb | 93 ++++++++++++++++++++++--------- spec/unit/promstats_spec.rb | 36 +++++++++--- 4 files changed, 126 insertions(+), 67 deletions(-) diff --git a/lib/vmpooler/metrics/promstats.rb b/lib/vmpooler/metrics/promstats.rb index 86ca64c..9a9e7a6 100644 --- a/lib/vmpooler/metrics/promstats.rb +++ b/lib/vmpooler/metrics/promstats.rb @@ -43,21 +43,29 @@ module Vmpooler }, param_labels: %i[template_name] }, - usage: { + user: { mtype: M_COUNTER, - docstring: 'Number of Pool Instances of this created', - prom_metric_prefix: "#{@metrics_prefix}_usage", + docstring: 'Number of Pool Instances this user created created', + prom_metric_prefix: "#{@metrics_prefix}_user", param_labels: %i[user poolname] }, - user: { - # This metrics is leads to a lot of label values which is likely to challenge data storage - # on prometheus - see Best Practices: https://prometheus.io/docs/practices/naming/#labels - # So it is likely that this metric may need to be simplified or broken into a number - # of smaller metrics to capture the detail without challenging prometheus + usage_jenkins_instance: { mtype: M_COUNTER, - docstring: 'vmpooler user counters', - prom_metric_prefix: "#{@metrics_prefix}_user", - param_labels: %i[user instancex value_stream branch project job_name component_to_test poolname] + docstring: 'Pools by Jenkins Instance usage', + prom_metric_prefix: "#{@metrics_prefix}_usage_jenkins_instance", + param_labels: %i[jenkins_instance value_stream poolname] + }, + usage_branch_project: { + mtype: M_COUNTER, + docstring: 'Pools by branch/project usage', + prom_metric_prefix: "#{@metrics_prefix}_usage_branch_project", + param_labels: %i[branch project poolname] + }, + usage_job_component: { + mtype: M_COUNTER, + docstring: 'Pools by job/component usage', + prom_metric_prefix: "#{@metrics_prefix}_usage_job_component", + param_labels: %i[job_name component_to_test poolname] }, checkout: { mtype: M_COUNTER, diff --git a/lib/vmpooler/pool_manager.rb b/lib/vmpooler/pool_manager.rb index bed6a48..cdd6f63 100644 --- a/lib/vmpooler/pool_manager.rb +++ b/lib/vmpooler/pool_manager.rb @@ -491,15 +491,16 @@ module Vmpooler return if checkout.nil? user ||= 'unauthenticated' - unless jenkins_build_url - user = user.gsub('.', '_') - $metrics.increment("usage.#{user}.#{poolname}") - return - end + user = user.gsub('.', '_') + $metrics.increment("user.#{user}.#{poolname}") + return unless jenkins_build_url + + # TBD - Add Filter for Litmus here as well - to ignore for the moment. url_parts = jenkins_build_url.split('/')[2..-1] - instance = url_parts[0] + jenkins_instance = url_parts[0].gsub('.', '_') value_stream_parts = url_parts[2].split('_') + value_stream_parts = value_stream_parts.map { |s| s.gsub('.', '_') } value_stream = value_stream_parts.shift branch = value_stream_parts.pop project = value_stream_parts.shift @@ -507,22 +508,9 @@ module Vmpooler build_metadata_parts = url_parts[3] component_to_test = component_to_test('RMM_COMPONENT_TO_TEST_NAME', build_metadata_parts) - metric_parts = [ - 'usage', - user, - instance, - value_stream, - branch, - project, - job_name, - component_to_test, - poolname - ] - - metric_parts = metric_parts.reject(&:nil?) - metric_parts = metric_parts.map { |s| s.gsub('.', '_') } - - $metrics.increment(metric_parts.join('.')) + $metrics.increment("usage_jenkins_instance.#{jenkins_instance}.#{value_stream}.#{poolname}") + $metrics.increment("usage_branch_project.#{branch}.#{project}.#{poolname}") + $metrics.increment("usage_job_component.#{job_name}.#{component_to_test}.#{poolname}") rescue StandardError => e $logger.log('d', "[!] [#{poolname}] failed while evaluating usage labels on '#{vm}' with an error: #{e}") raise @@ -537,7 +525,7 @@ module Vmpooler next if value.nil? return value if key == match end - nil + 'none' end def purge_unused_vms_and_folders diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb index efe6b0e..ed7d0ae 100644 --- a/spec/unit/pool_manager_spec.rb +++ b/spec/unit/pool_manager_spec.rb @@ -1110,7 +1110,7 @@ EOT it 'should emit a metric' do redis_connection_pool.with do |redis| - expect(metrics).to receive(:increment).with("usage.unauthenticated.#{template}") + expect(metrics).to receive(:increment).with("user.unauthenticated.#{template}") subject.get_vm_usage_labels(vm, redis) end @@ -1126,7 +1126,8 @@ EOT end it 'should emit a metric' do - expect(metrics).to receive(:increment).with("usage.#{user}.#{template}") + expect(metrics).to receive(:increment).with("user.#{user}.#{template}") + expect(metrics).not_to receive(:increment) redis_connection_pool.with do |redis| subject.get_vm_usage_labels(vm, redis) @@ -1135,7 +1136,7 @@ EOT context 'with a user with period in name' do let(:user) { 'test.user'.gsub('.', '_') } - let(:metric_string) { "usage.#{user}.#{template}" } + let(:metric_string) { "user.#{user}.#{template}" } let(:metric_nodes) { metric_string.split('.') } before(:each) do @@ -1146,6 +1147,7 @@ EOT it 'should emit a metric with the character replaced' do expect(metrics).to receive(:increment).with(metric_string) + expect(metrics).not_to receive(:increment) redis_connection_pool.with do |redis| subject.get_vm_usage_labels(vm, redis) @@ -1155,7 +1157,6 @@ EOT it 'should include three nodes' do expect(metric_nodes.count).to eq(3) end - end context 'with a jenkins_build_url label' do @@ -1167,16 +1168,11 @@ EOT let(:branch) { value_stream_parts.pop } let(:project) { value_stream_parts.shift } let(:job_name) { value_stream_parts.join('_') } - let(:metric_string_nodes) { - [ - 'usage', user, instance, value_stream, branch, project, job_name, template - ] - } - let(:metric_string_sub) { - metric_string_nodes.map { |s| s.gsub('.', '_') - } - } - let(:metric_string) { metric_string_sub.join('.') } + + let(:metric_string_1) { "user.#{user}.#{template}" } + let(:metric_string_2) { "usage_jenkins_instance.#{instance.gsub('.', '_')}.#{value_stream.gsub('.', '_')}.#{template}" } + let(:metric_string_3) { "usage_branch_project.#{branch.gsub('.', '_')}.#{project.gsub('.', '_')}.#{template}" } + let(:metric_string_4) { "usage_job_component.#{job_name.gsub('.', '_')}.none.#{template}" } before(:each) do redis_connection_pool.with do |redis| @@ -1184,8 +1180,12 @@ EOT end end - it 'should emit a metric with information from the URL' do - expect(metrics).to receive(:increment).with(metric_string) + it 'should emit 4 metric withs information from the URL' do + expect(metrics).to receive(:increment).with(metric_string_1) + expect(metrics).to receive(:increment).with(metric_string_2) + expect(metrics).to receive(:increment).with(metric_string_3) + expect(metrics).to receive(:increment).with(metric_string_4) + expect(metrics).not_to receive(:increment) redis_connection_pool.with do |redis| subject.get_vm_usage_labels(vm, redis) @@ -1207,14 +1207,23 @@ EOT let(:expected_string) { "usage.#{user}.#{instance}.#{value_stream}.#{branch}.#{project}.#{job_name}.#{build_component}.#{template}" } let(:metric_nodes) { expected_string.split('.') } + let(:metric_string_1) { "user.#{user}.#{template}" } + let(:metric_string_2) { "usage_jenkins_instance.#{instance.gsub('.', '_')}.#{value_stream.gsub('.', '_')}.#{template}" } + let(:metric_string_3) { "usage_branch_project.#{branch.gsub('.', '_')}.#{project.gsub('.', '_')}.#{template}" } + let(:metric_string_4) { "usage_job_component.#{job_name.gsub('.', '_')}.#{build_component}.#{template}" } + before(:each) do redis_connection_pool.with do |redis| create_tag(vm, 'jenkins_build_url', jenkins_build_url, redis) end end - it 'should emit a metric with information from the URL' do - expect(metrics).to receive(:increment).with(expected_string) + it 'should emit 4 metrics with information from the URL' do + expect(metrics).to receive(:increment).with(metric_string_1) + expect(metrics).to receive(:increment).with(metric_string_2) + expect(metrics).to receive(:increment).with(metric_string_3) + expect(metrics).to receive(:increment).with(metric_string_4) + expect(metrics).not_to receive(:increment) redis_connection_pool.with do |redis| subject.get_vm_usage_labels(vm, redis) @@ -1236,20 +1245,54 @@ EOT let(:project) { value_stream_parts.shift } let(:job_name) { value_stream_parts.join('_') } + let(:metric_string_1) { "user.#{user}.#{template}" } + let(:metric_string_2) { "usage_jenkins_instance.#{instance.gsub('.', '_')}.#{value_stream.gsub('.', '_')}.#{template}" } + let(:metric_string_3) { "usage_branch_project.#{branch.gsub('.', '_')}.#{project.gsub('.', '_')}.#{template}" } + let(:metric_string_4) { "usage_job_component.#{job_name.gsub('.', '_')}.none.#{template}" } + before(:each) do redis_connection_pool.with do |redis| create_tag(vm, 'jenkins_build_url', jenkins_build_url, redis) end end - it 'should emit a metric with information from the URL without a build_component' do - expect(metrics).to receive(:increment).with("usage.#{user}.#{instance}.#{value_stream}.#{branch}.#{project}.#{job_name}.#{template}") - + it 'should emit 4 metrics with information from the URL without a build_component' do + expect(metrics).to receive(:increment).with(metric_string_1) + expect(metrics).to receive(:increment).with(metric_string_2) + expect(metrics).to receive(:increment).with(metric_string_3) + expect(metrics).to receive(:increment).with(metric_string_4) + expect(metrics).not_to receive(:increment) + redis_connection_pool.with do |redis| subject.get_vm_usage_labels(vm, redis) end end end + + + end + + context 'with a litmus job' do + let(:jenkins_build_url) { 'https://litmus_manual' } + + let(:metric_string_1) { "user.#{user}.#{template}" } + let(:metric_string_2) { "usage_litmus.#{user}.#{template}" } + + before(:each) do + redis_connection_pool.with do |redis| + create_tag(vm, 'jenkins_build_url', jenkins_build_url, redis) + end + end + + it 'should emit 2 metrics with the second indicating a litmus job' do + expect(metrics).to receive(:increment).with(metric_string_1) + expect(metrics).to receive(:increment).with(metric_string_2) + expect(metrics).not_to receive(:increment) + + redis_connection_pool.with do |redis| + subject.get_vm_usage_labels(vm, redis) + end + end end end @@ -1269,21 +1312,21 @@ EOT end context 'when match contains no value' do - it 'should return nil' do - expect(subject.component_to_test(matching_key, matching_key)).to be nil + it 'should return none' do + expect(subject.component_to_test(matching_key, matching_key)).to eq('none') end end end context 'when string contains no key value pairs' do it 'should return' do - expect(subject.component_to_test(matching_key, nonmatrix_string)).to be nil + expect(subject.component_to_test(matching_key, nonmatrix_string)).to eq('none') end end context 'when labels_string is a job number' do it 'should return nil' do - expect(subject.component_to_test(matching_key, '25')).to be nil + expect(subject.component_to_test(matching_key, '25')).to eq('none') end end diff --git a/spec/unit/promstats_spec.rb b/spec/unit/promstats_spec.rb index 5db495d..d721cce 100644 --- a/spec/unit/promstats_spec.rb +++ b/spec/unit/promstats_spec.rb @@ -142,22 +142,42 @@ describe 'prometheus' do po.get(labels: metric[:labels]) }.by(1) end - it 'Increments usage.#{user}.#{poolname}' do + it 'Increments user.#{user}.#{poolname}' do user = 'myuser' poolname = 'test-pool' - expect { subject.increment("usage.#{user}.#{poolname}") }.to change { - metric, po = subject.get("usage.#{user}.#{poolname}") + expect { subject.increment("user.#{user}.#{poolname}") }.to change { + metric, po = subject.get("user.#{user}.#{poolname}") po.get(labels: metric[:labels]) }.by(1) end - it 'Increments label :user' do - # subject.increment(:user, :instance, :value_stream, :branch, :project, :job_name, :component_to_test, :poolname) - showing labels here - pending 'increment only supports a string containing a dot separator' - expect { subject.increment(:user) }.to change { - metric, po = subject.get(:user) + it 'Increments label usage_jenkins_instance.#{jenkins_instance}.#{value_stream}.#{poolname}' do + jenkins_instance = 'jenkins_test_instance' + value_stream = 'notional_value' + poolname = 'test-pool' + expect { subject.increment("usage_jenkins_instance.#{jenkins_instance}.#{value_stream}.#{poolname}") }.to change { + metric, po = subject.get("usage_jenkins_instance.#{jenkins_instance}.#{value_stream}.#{poolname}") po.get(labels: metric[:labels]) }.by(1) end + it 'Increments label usage_branch_project.#{branch}.#{project}.#{poolname}' do + branch = 'treetop' + project = 'test-project' + poolname = 'test-pool' + expect { subject.increment("usage_branch_project.#{branch}.#{project}.#{poolname}") }.to change { + metric, po = subject.get("usage_branch_project.#{branch}.#{project}.#{poolname}") + po.get(labels: metric[:labels]) + }.by(1) + end + it 'Increments label usage_job_component.#{job_name}.#{component_to_test}.#{poolname}' do + job_name = 'a-job' + component_to_test = 'component-name' + poolname = 'test-pool' + expect { subject.increment("usage_job_component.#{job_name}.#{component_to_test}.#{poolname}") }.to change { + metric, po = subject.get("usage_job_component.#{job_name}.#{component_to_test}.#{poolname}") + po.get(labels: metric[:labels]) + }.by(1) + end + it 'Increments connect.open' do expect { subject.increment('connect.open') }.to change { metric, po = subject.get('connect.open') From cb955a1bedae2ec67cb44eaac10c184095a9f825 Mon Sep 17 00:00:00 2001 From: John O'Connor Date: Wed, 17 Jun 2020 22:32:56 +0100 Subject: [PATCH 08/12] (POOLER-177) Filter hostname from API Paths Use the example provided in the Ruby Client to provide a customised collector appropriate to log all calls to the API. The customised filtering is used to replace individual node names and templates for the /vm and request ID's for the /ondemand endpoints. This module was failing our rubocop checks so have updated it since it now forms part of vmpooler. Separate trapping for litmus jobs is also included so that they don't interfere with stats from the jenkins pipelines. --- lib/vmpooler.rb | 3 - lib/vmpooler/api.rb | 5 +- lib/vmpooler/api/v1.rb | 12 ++ .../metrics/promstats/collector_middleware.rb | 119 +++++++++++++++ lib/vmpooler/pool_manager.rb | 7 +- spec/unit/collector_middleware_spec.rb | 135 ++++++++++++++++++ spec/unit/generic_connection_pool_spec.rb | 2 +- 7 files changed, 277 insertions(+), 6 deletions(-) create mode 100644 lib/vmpooler/metrics/promstats/collector_middleware.rb create mode 100644 spec/unit/collector_middleware_spec.rb diff --git a/lib/vmpooler.rb b/lib/vmpooler.rb index 98b4bd6..20eb26b 100644 --- a/lib/vmpooler.rb +++ b/lib/vmpooler.rb @@ -15,9 +15,6 @@ module Vmpooler require 'timeout' require 'yaml' - require 'prometheus/middleware/collector' - require 'prometheus/middleware/exporter' - %w[api metrics logger pool_manager generic_connection_pool].each do |lib| require "vmpooler/#{lib}" end diff --git a/lib/vmpooler/api.rb b/lib/vmpooler/api.rb index 068f024..cf516a7 100644 --- a/lib/vmpooler/api.rb +++ b/lib/vmpooler/api.rb @@ -35,7 +35,10 @@ module Vmpooler # in the config file. metrics.setup_prometheus_metrics - use Prometheus::Middleware::Collector, metrics_prefix: "#{metrics.metrics_prefix}_http" + # Using customised collector that filters out hostnames on API paths + require 'vmpooler/metrics/promstats/collector_middleware' + require 'prometheus/middleware/exporter' + use Vmpooler::Metrics::Promstats::CollectorMiddleware, metrics_prefix: "#{metrics.metrics_prefix}_http" use Prometheus::Middleware::Exporter, path: metrics.endpoint end diff --git a/lib/vmpooler/api/v1.rb b/lib/vmpooler/api/v1.rb index 5ecb2b5..c5cb0bf 100644 --- a/lib/vmpooler/api/v1.rb +++ b/lib/vmpooler/api/v1.rb @@ -809,6 +809,7 @@ module Vmpooler post "#{api_prefix}/ondemandvm/?" do content_type :json + metrics.increment('api_vm.post.ondemand.requestid') need_token! if Vmpooler::API.settings.config[:auth] @@ -846,6 +847,7 @@ module Vmpooler post "#{api_prefix}/ondemandvm/:template/?" do content_type :json result = { 'ok' => false } + metrics.increment('api_vm.delete.ondemand.template') need_token! if Vmpooler::API.settings.config[:auth] @@ -872,6 +874,7 @@ module Vmpooler get "#{api_prefix}/ondemandvm/:requestid/?" do content_type :json + metrics.increment('api_vm.get.ondemand.request') status 404 result = check_ondemand_request(params[:requestid]) @@ -882,6 +885,7 @@ module Vmpooler delete "#{api_prefix}/ondemandvm/:requestid/?" do content_type :json need_token! if Vmpooler::API.settings.config[:auth] + metrics.increment('api_vm.delete.ondemand.request') status 404 result = delete_ondemand_request(params[:requestid]) @@ -892,6 +896,7 @@ module Vmpooler post "#{api_prefix}/vm/?" do content_type :json result = { 'ok' => false } + metrics.increment('api_vm.post.vm.checkout') payload = JSON.parse(request.body.read) @@ -1034,6 +1039,7 @@ module Vmpooler post "#{api_prefix}/vm/:template/?" do content_type :json result = { 'ok' => false } + metrics.increment('api_vm.get.vm.template') payload = extract_templates_from_query_params(params[:template]) @@ -1057,6 +1063,7 @@ module Vmpooler get "#{api_prefix}/vm/:hostname/?" do content_type :json + metrics.increment('api_vm.get.vm.hostname') result = {} @@ -1129,6 +1136,7 @@ module Vmpooler delete "#{api_prefix}/vm/:hostname/?" do content_type :json + metrics.increment('api_vm.delete.vm.hostname') result = {} @@ -1156,6 +1164,7 @@ module Vmpooler put "#{api_prefix}/vm/:hostname/?" do content_type :json + metrics.increment('api_vm.put.vm.modify') status 404 result = { 'ok' => false } @@ -1232,6 +1241,7 @@ module Vmpooler post "#{api_prefix}/vm/:hostname/disk/:size/?" do content_type :json + metrics.increment('api_vm.post.vm.disksize') need_token! if Vmpooler::API.settings.config[:auth] @@ -1255,6 +1265,7 @@ module Vmpooler post "#{api_prefix}/vm/:hostname/snapshot/?" do content_type :json + metrics.increment('api_vm.post.vm.snapshot') need_token! if Vmpooler::API.settings.config[:auth] @@ -1280,6 +1291,7 @@ module Vmpooler post "#{api_prefix}/vm/:hostname/snapshot/:snapshot/?" do content_type :json + metrics.increment('api_vm.post.vm.disksize') need_token! if Vmpooler::API.settings.config[:auth] diff --git a/lib/vmpooler/metrics/promstats/collector_middleware.rb b/lib/vmpooler/metrics/promstats/collector_middleware.rb new file mode 100644 index 0000000..c05c866 --- /dev/null +++ b/lib/vmpooler/metrics/promstats/collector_middleware.rb @@ -0,0 +1,119 @@ +# frozen_string_literal: true + +# This is an adapted Collector module for vmpooler based on the sample implementation +# available in the prometheus client_ruby library +# https://github.com/prometheus/client_ruby/blob/master/lib/prometheus/middleware/collector.rb +# +# The code was also failing Rubocop on PR check, so have addressed all the offenses. +# +# The method strip_ids_from_path has been adapted to add a match for hostnames in paths +# to replace with a single ":hostname" string to avoid proliferation of stat lines for +# each new vm hostname deleted, modified or otherwise queried. + +require 'benchmark' +require 'prometheus/client' +require 'vmpooler/logger' + +module Vmpooler + class Metrics + class Promstats + # CollectorMiddleware is an implementation of Rack Middleware customised + # for vmpooler use. + # + # By default metrics are registered on the global registry. Set the + # `:registry` option to use a custom registry. + # + # By default metrics all have the prefix "http_server". Set to something + # else if you like. + # + # The request counter metric is broken down by code, method and path by + # default. Set the `:counter_label_builder` option to use a custom label + # builder. + # + # The request duration metric is broken down by method and path by default. + # Set the `:duration_label_builder` option to use a custom label builder. + # + # Label Builder functions will receive a Rack env and a status code, and must + # return a hash with the labels for that request. They must also accept an empty + # env, and return a hash with the correct keys. This is necessary to initialize + # the metrics with the correct set of labels. + class CollectorMiddleware + attr_reader :app, :registry + + def initialize(app, options = {}) + @app = app + @registry = options[:registry] || Prometheus::Client.registry + @metrics_prefix = options[:metrics_prefix] || 'http_server' + + init_request_metrics + init_exception_metrics + end + + def call(env) # :nodoc: + trace(env) { @app.call(env) } + end + + protected + + def init_request_metrics + @requests = @registry.counter( + :"#{@metrics_prefix}_requests_total", + docstring: + 'The total number of HTTP requests handled by the Rack application.', + labels: %i[code method path] + ) + @durations = @registry.histogram( + :"#{@metrics_prefix}_request_duration_seconds", + docstring: 'The HTTP response duration of the Rack application.', + labels: %i[method path] + ) + end + + def init_exception_metrics + @exceptions = @registry.counter( + :"#{@metrics_prefix}_exceptions_total", + docstring: 'The total number of exceptions raised by the Rack application.', + labels: [:exception] + ) + end + + def trace(env) + response = nil + duration = Benchmark.realtime { response = yield } + record(env, response.first.to_s, duration) + response + rescue StandardError => e + @exceptions.increment(labels: { exception: e.class.name }) + raise + end + + def record(env, code, duration) + counter_labels = { + code: code, + method: env['REQUEST_METHOD'].downcase, + path: strip_ids_from_path(env['PATH_INFO']) + } + + duration_labels = { + method: env['REQUEST_METHOD'].downcase, + path: strip_ids_from_path(env['PATH_INFO']) + } + + @requests.increment(labels: counter_labels) + @durations.observe(duration, labels: duration_labels) + rescue # rubocop:disable Style/RescueStandardError + nil + end + + def strip_ids_from_path(path) + # Custom for /vm path - so we just collect aggrate stats for all usage along this one + # path. Custom counters are then added more specific endpoints in v1.rb + # Since we aren't parsing UID/GIDs as in the original example, these are removed. + path + .gsub(%r{/vm/.+$}, '/vm') + .gsub(%r{/ondemand/.+$}, '/ondemand') + end + end + end + end +end diff --git a/lib/vmpooler/pool_manager.rb b/lib/vmpooler/pool_manager.rb index cdd6f63..0d56fbc 100644 --- a/lib/vmpooler/pool_manager.rb +++ b/lib/vmpooler/pool_manager.rb @@ -496,7 +496,12 @@ module Vmpooler return unless jenkins_build_url - # TBD - Add Filter for Litmus here as well - to ignore for the moment. + if jenkins_build_url.include? 'litmus' + # Very simple filter for Litmus jobs - just count them coming through for the moment. + $metrics.increment("usage_litmus.#{user}.#{poolname}") + return + end + url_parts = jenkins_build_url.split('/')[2..-1] jenkins_instance = url_parts[0].gsub('.', '_') value_stream_parts = url_parts[2].split('_') diff --git a/spec/unit/collector_middleware_spec.rb b/spec/unit/collector_middleware_spec.rb new file mode 100644 index 0000000..671e118 --- /dev/null +++ b/spec/unit/collector_middleware_spec.rb @@ -0,0 +1,135 @@ + +require 'rack/test' +require 'vmpooler/metrics/promstats/collector_middleware' + + +describe Vmpooler::Metrics::Promstats::CollectorMiddleware do + include Rack::Test::Methods + + # Reset the data store + before do + Prometheus::Client.config.data_store = Prometheus::Client::DataStores::Synchronized.new + end + + let(:registry) do + Prometheus::Client::Registry.new + end + + let(:original_app) do + ->(_) { [200, { 'Content-Type' => 'text/html' }, ['OK']] } + end + + let!(:app) do + described_class.new(original_app, registry: registry) + end + + let(:dummy_error) { RuntimeError.new("Dummy error from tests") } + + it 'returns the app response' do + get '/foo' + + expect(last_response).to be_ok + expect(last_response.body).to eql('OK') + end + + it 'handles errors in the registry gracefully' do + counter = registry.get(:http_server_requests_total) + expect(counter).to receive(:increment).and_raise(dummy_error) + + get '/foo' + + expect(last_response).to be_ok + end + + it 'traces request information' do + expect(Benchmark).to receive(:realtime).and_yield.and_return(0.2) + + get '/foo' + + metric = :http_server_requests_total + labels = { method: 'get', path: '/foo', code: '200' } + expect(registry.get(metric).get(labels: labels)).to eql(1.0) + + metric = :http_server_request_duration_seconds + labels = { method: 'get', path: '/foo' } + expect(registry.get(metric).get(labels: labels)).to include("0.1" => 0, "0.25" => 1) + end + + it 'normalizes paths cotaining /vm by default' do + expect(Benchmark).to receive(:realtime).and_yield.and_return(0.3) + + get '/foo/vm/bar-mumble-flame' + + metric = :http_server_requests_total + labels = { method: 'get', path: '/foo/vm', code: '200' } + expect(registry.get(metric).get(labels: labels)).to eql(1.0) + + metric = :http_server_request_duration_seconds + labels = { method: 'get', path: '/foo/vm' } + expect(registry.get(metric).get(labels: labels)).to include("0.1" => 0, "0.5" => 1) + end + + it 'normalizes paths containing /ondemandvm by ' do + expect(Benchmark).to receive(:realtime).and_yield.and_return(0.3) + + get '/foo/ondemand/bar/fatman' + + metric = :http_server_requests_total + labels = { method: 'get', path: '/foo/ondemand', code: '200' } + expect(registry.get(metric).get(labels: labels)).to eql(1.0) + + metric = :http_server_request_duration_seconds + labels = { method: 'get', path: '/foo/ondemand' } + expect(registry.get(metric).get(labels: labels)).to include("0.1" => 0, "0.5" => 1) + end + + context 'when the app raises an exception' do + let(:original_app) do + lambda do |env| + raise dummy_error if env['PATH_INFO'] == '/broken' + + [200, { 'Content-Type' => 'text/html' }, ['OK']] + end + end + + before do + get '/foo' + end + + it 'traces exceptions' do + expect { get '/broken' }.to raise_error RuntimeError + + metric = :http_server_exceptions_total + labels = { exception: 'RuntimeError' } + expect(registry.get(metric).get(labels: labels)).to eql(1.0) + end + end + + context 'when provided a custom metrics_prefix' do + let!(:app) do + described_class.new( + original_app, + registry: registry, + metrics_prefix: 'lolrus', + ) + end + + it 'provides alternate metric names' do + expect( + registry.get(:lolrus_requests_total), + ).to be_a(Prometheus::Client::Counter) + expect( + registry.get(:lolrus_request_duration_seconds), + ).to be_a(Prometheus::Client::Histogram) + expect( + registry.get(:lolrus_exceptions_total), + ).to be_a(Prometheus::Client::Counter) + end + + it "doesn't register the default metrics" do + expect(registry.get(:http_server_requests_total)).to be(nil) + expect(registry.get(:http_server_request_duration_seconds)).to be(nil) + expect(registry.get(:http_server_exceptions_total)).to be(nil) + end + end +end diff --git a/spec/unit/generic_connection_pool_spec.rb b/spec/unit/generic_connection_pool_spec.rb index 350e6f5..5c24215 100644 --- a/spec/unit/generic_connection_pool_spec.rb +++ b/spec/unit/generic_connection_pool_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe 'GenericConnectionPool' do - let(:metrics) { Vmpooler::DummyStatsd.new } + let(:metrics) { Vmpooler::Metrics::DummyStatsd.new } let(:connpool_type) { 'test_connection_pool' } let(:connpool_provider) { 'testprovider' } let(:default_connpool_type) { 'connectionpool' } From 8ed8c43970fa6e429f4605991e32364573128f51 Mon Sep 17 00:00:00 2001 From: John O'Connor Date: Thu, 18 Jun 2020 21:27:08 +0100 Subject: [PATCH 09/12] (POOLER-160) Revise Metrics Classwork Review changes suggested to revise the Metrics related files into a more logical class structure. Also fixup grammar typos in docs strings and any trailing metrics that have been recently added to vmpooler. --- lib/vmpooler/metrics.rb | 18 +- lib/vmpooler/metrics/dummy_statsd.rb | 26 +- lib/vmpooler/metrics/graphite.rb | 70 +-- lib/vmpooler/metrics/promstats.rb | 559 ++++++++++---------- lib/vmpooler/metrics/statsd.rb | 50 +- spec/integration/api/v1/config_spec.rb | 2 +- spec/integration/api/v1/ondemandvm_spec.rb | 2 +- spec/integration/api/v1/poolreset.rb | 2 +- spec/integration/api/v1/vm_hostname_spec.rb | 2 +- spec/integration/api/v1/vm_spec.rb | 2 +- spec/integration/api/v1/vm_template_spec.rb | 2 +- spec/unit/pool_manager_spec.rb | 2 +- spec/unit/promstats_spec.rb | 30 +- spec/unit/providers/base_spec.rb | 2 +- spec/unit/providers/dummy_spec.rb | 2 +- spec/unit/providers/vsphere_spec.rb | 2 +- 16 files changed, 409 insertions(+), 364 deletions(-) diff --git a/lib/vmpooler/metrics.rb b/lib/vmpooler/metrics.rb index 241dc18..98c17f9 100644 --- a/lib/vmpooler/metrics.rb +++ b/lib/vmpooler/metrics.rb @@ -1,24 +1,24 @@ # frozen_string_literal: true +require 'vmpooler/metrics/statsd' +require 'vmpooler/metrics/graphite' +require 'vmpooler/metrics/promstats' +require 'vmpooler/metrics/dummy_statsd' + module Vmpooler class Metrics # static class instantiate appropriate metrics object. def self.init(logger, params) if params[:statsd] - metrics = Vmpooler::Statsd.new(logger, params[:statsd]) + metrics = Vmpooler::Metrics::Statsd.new(logger, params[:statsd]) elsif params[:graphite] - metrics = Vmpooler::Graphite.new(logger, params[:graphite]) + metrics = Vmpooler::Metrics::Graphite.new(logger, params[:graphite]) elsif params[:prometheus] - metrics = Vmpooler::Promstats.new(logger, params[:prometheus]) + metrics = Vmpooler::Metrics::Promstats.new(logger, params[:prometheus]) else - metrics = Vmpooler::DummyStatsd.new + metrics = Vmpooler::Metrics::DummyStatsd.new end metrics end end end - -require 'vmpooler/metrics/statsd' -require 'vmpooler/metrics/dummy_statsd' -require 'vmpooler/metrics/graphite' -require 'vmpooler/metrics/promstats' diff --git a/lib/vmpooler/metrics/dummy_statsd.rb b/lib/vmpooler/metrics/dummy_statsd.rb index 2cc4ac6..96f2eaa 100644 --- a/lib/vmpooler/metrics/dummy_statsd.rb +++ b/lib/vmpooler/metrics/dummy_statsd.rb @@ -1,22 +1,24 @@ # frozen_string_literal: true module Vmpooler - class DummyStatsd < Metrics - attr_reader :server, :port, :prefix + class Metrics + class DummyStatsd < Metrics + attr_reader :server, :port, :prefix - def initialize(*) - end + def initialize(*) + end - def increment(*) - true - end + def increment(*) + true + end - def gauge(*) - true - end + def gauge(*) + true + end - def timing(*) - true + def timing(*) + true + end end end end diff --git a/lib/vmpooler/metrics/graphite.rb b/lib/vmpooler/metrics/graphite.rb index e94e77a..128805d 100644 --- a/lib/vmpooler/metrics/graphite.rb +++ b/lib/vmpooler/metrics/graphite.rb @@ -3,43 +3,45 @@ require 'rubygems' unless defined?(Gem) module Vmpooler - class Graphite < Metrics - attr_reader :server, :port, :prefix + class Metrics + class Graphite < Metrics + attr_reader :server, :port, :prefix - def initialize(logger, params = {}) - raise ArgumentError, "Graphite server is required. Config: #{params.inspect}" if params['server'].nil? || params['server'].empty? + def initialize(logger, params = {}) + raise ArgumentError, "Graphite server is required. Config: #{params.inspect}" if params['server'].nil? || params['server'].empty? - @server = params['server'] - @port = params['port'] || 2003 - @prefix = params['prefix'] || 'vmpooler' - @logger = logger - 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, port) - begin - socket.puts "#{prefix}.#{path} #{value} #{Time.now.to_i}" - ensure - socket.close - end + @server = params['server'] + @port = params['port'] || 2003 + @prefix = params['prefix'] || 'vmpooler' + @logger = logger + 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, port) + begin + socket.puts "#{prefix}.#{path} #{value} #{Time.now.to_i}" + ensure + socket.close + end + end + rescue Errno::EADDRNOTAVAIL => e + warn "Could not assign address to graphite server #{server}: #{e}" + rescue StandardError => e + @logger.log('s', "[!] Failure logging #{path} to graphite server [#{server}:#{port}]: #{e}") end - rescue Errno::EADDRNOTAVAIL => e - warn "Could not assign address to graphite server #{server}: #{e}" - rescue StandardError => e - @logger.log('s', "[!] Failure logging #{path} to graphite server [#{server}:#{port}]: #{e}") end end end diff --git a/lib/vmpooler/metrics/promstats.rb b/lib/vmpooler/metrics/promstats.rb index 9a9e7a6..e385d17 100644 --- a/lib/vmpooler/metrics/promstats.rb +++ b/lib/vmpooler/metrics/promstats.rb @@ -3,304 +3,319 @@ require 'prometheus/client' module Vmpooler - class Promstats < Metrics - attr_reader :prefix, :endpoint, :metrics_prefix + class Metrics + class Promstats < Metrics + attr_reader :prefix, :endpoint, :metrics_prefix - # Constants for Metric Types - M_COUNTER = 1 - M_GAUGE = 2 - M_SUMMARY = 3 - M_HISTOGRAM = 4 + # Constants for Metric Types + M_COUNTER = 1 + M_GAUGE = 2 + M_SUMMARY = 3 + M_HISTOGRAM = 4 - # Customised Bucket set to use for the Pooler clone times set to more appropriate intervals. - POOLER_TIME_BUCKETS = [1.0, 2.5, 5.0, 10.0, 20.0, 50.0, 100.0, 200.0, 500.0, 1000.0, 2000.0].freeze - # Same for Redis connection times - this is the same as the current Prometheus Default. - # https://github.com/prometheus/client_ruby/blob/master/lib/prometheus/client/histogram.rb#L14 - REDIS_CONNECT_BUCKETS = [0.005, 0.01, 0.025, 0.05, 0.1, 0.25, 0.5, 1, 2.5, 5, 10].freeze + # Customised Bucket set to use for the Pooler clone times set to more appropriate intervals. + POOLER_TIME_BUCKETS = [1.0, 2.5, 5.0, 10.0, 20.0, 50.0, 100.0, 200.0, 500.0, 1000.0, 2000.0].freeze + # Same for redis connection times - this is the same as the current Prometheus Default. + # https://github.com/prometheus/client_ruby/blob/master/lib/prometheus/client/histogram.rb#L14 + REDIS_CONNECT_BUCKETS = [0.005, 0.01, 0.025, 0.05, 0.1, 0.25, 0.5, 1, 2.5, 5, 10].freeze - @p_metrics = {} + @p_metrics = {} - def initialize(logger, params = {}) - @prefix = params['prefix'] || 'vmpooler' - @metrics_prefix = params['metrics_prefix'] || 'vmpooler' - @endpoint = params['endpoint'] || '/prometheus' - @logger = logger + def initialize(logger, params = {}) + @prefix = params['prefix'] || 'vmpooler' + @metrics_prefix = params['metrics_prefix'] || 'vmpooler' + @endpoint = params['endpoint'] || '/prometheus' + @logger = logger - # Setup up prometheus registry and data structures - @prometheus = Prometheus::Client.registry - end + # Setup up prometheus registry and data structures + @prometheus = Prometheus::Client.registry + end - # Metrics structure used to register the metrics and also translate/interpret the incoming metrics. - def vmpooler_metrics_table - { - errors: { - mtype: M_COUNTER, - docstring: 'Count of Errors for pool', - prom_metric_prefix: "#{@metrics_prefix}_errors", - metric_suffixes: { - markedasfailed: 'Timeout waiting for instance to initialise', - duplicatehostname: 'Unable to create instance due to duplicate hostname' + # Metrics structure used to register the metrics and also translate/interpret the incoming metrics. + def vmpooler_metrics_table + { + errors: { + mtype: M_COUNTER, + docstring: 'Count of errors for pool', + prom_metric_prefix: "#{@metrics_prefix}_errors", + metric_suffixes: { + markedasfailed: 'timeout waiting for instance to initialise', + duplicatehostname: 'unable to create instance due to duplicate hostname', + staledns: 'unable to create instance due to duplicate DNS record' + }, + param_labels: %i[template_name] }, - param_labels: %i[template_name] - }, - user: { - mtype: M_COUNTER, - docstring: 'Number of Pool Instances this user created created', - prom_metric_prefix: "#{@metrics_prefix}_user", - param_labels: %i[user poolname] - }, - usage_jenkins_instance: { - mtype: M_COUNTER, - docstring: 'Pools by Jenkins Instance usage', - prom_metric_prefix: "#{@metrics_prefix}_usage_jenkins_instance", - param_labels: %i[jenkins_instance value_stream poolname] - }, - usage_branch_project: { - mtype: M_COUNTER, - docstring: 'Pools by branch/project usage', - prom_metric_prefix: "#{@metrics_prefix}_usage_branch_project", - param_labels: %i[branch project poolname] - }, - usage_job_component: { - mtype: M_COUNTER, - docstring: 'Pools by job/component usage', - prom_metric_prefix: "#{@metrics_prefix}_usage_job_component", - param_labels: %i[job_name component_to_test poolname] - }, - checkout: { - mtype: M_COUNTER, - docstring: 'Pool Checkout counts', - prom_metric_prefix: "#{@metrics_prefix}_checkout", - metric_suffixes: { - nonresponsive: 'Checkout Failed - Non Responsive Machine', - empty: 'Checkout Failed - no machine', - success: 'Successful Checkout', - invalid: 'Checkout Failed - Invalid Template' + user: { + mtype: M_COUNTER, + docstring: 'Number of pool instances this user created created', + prom_metric_prefix: "#{@metrics_prefix}_user", + param_labels: %i[user poolname] }, - param_labels: %i[poolname] - }, - config: { - mtype: M_COUNTER, - docstring: 'vmpooler Pool Configuration Request', - prom_metric_prefix: "#{@metrics_prefix}_config", - metric_suffixes: { invalid: 'Invalid' }, - param_labels: %i[poolname] - }, - poolreset: { - mtype: M_COUNTER, - docstring: 'Pool Reset Counter', - prom_metric_prefix: "#{@metrics_prefix}_poolreset", - metric_suffixes: { invalid: 'Invalid Pool' }, - param_labels: %i[poolname] - }, - connect: { - mtype: M_COUNTER, - docstring: 'vmpooler Connect (to vSphere)', - prom_metric_prefix: "#{@metrics_prefix}_connect", - metric_suffixes: { - open: 'Connect Succeeded', - fail: 'Connect Failed' + usage_litmus: { + mtype: M_COUNTER, + docstring: 'Pools by Litmus job usage', + prom_metric_prefix: "#{@metrics_prefix}_usage_litmus", + param_labels: %i[user poolname] }, - param_labels: [] - }, - migrate_from: { - mtype: M_COUNTER, - docstring: 'vmpooler Machine Migrated from', - prom_metric_prefix: "#{@metrics_prefix}_migrate_from", - param_labels: %i[host_name] - }, - migrate_to: { - mtype: M_COUNTER, - docstring: 'vmpooler Machine Migrated to', - prom_metric_prefix: "#{@metrics_prefix}_migrate_to", - param_labels: %i[host_name] - }, - ready: { - mtype: M_GAUGE, - docstring: 'vmpooler Number of Machines in Ready State', - prom_metric_prefix: "#{@metrics_prefix}_ready", - param_labels: %i[poolname] - }, - running: { - mtype: M_GAUGE, - docstring: 'vmpooler Number of Machines Running', - prom_metric_prefix: "#{@metrics_prefix}_running", - param_labels: %i[poolname] - }, - connection_available: { - mtype: M_GAUGE, - docstring: 'vmpooler Redis Connections Available', - prom_metric_prefix: "#{@metrics_prefix}_connection_available", - param_labels: %i[type provider] - }, - time_to_ready_state: { - mtype: M_HISTOGRAM, - buckets: POOLER_TIME_BUCKETS, - docstring: 'Time taken for machine to read ready state for pool', - prom_metric_prefix: "#{@metrics_prefix}_time_to_ready_state", - param_labels: %i[poolname] - }, - migrate: { - mtype: M_HISTOGRAM, - buckets: POOLER_TIME_BUCKETS, - docstring: 'vmpooler Time taken to migrate machine for pool', - prom_metric_prefix: "#{@metrics_prefix}_migrate", - param_labels: %i[poolname] - }, - clone: { - mtype: M_HISTOGRAM, - buckets: POOLER_TIME_BUCKETS, - docstring: 'vmpooler Time taken to Clone Machine', - prom_metric_prefix: "#{@metrics_prefix}_clone", - param_labels: %i[poolname] - }, - destroy: { - mtype: M_HISTOGRAM, - buckets: POOLER_TIME_BUCKETS, - docstring: 'vmpooler Time taken to Destroy Machine', - prom_metric_prefix: "#{@metrics_prefix}_destroy", - param_labels: %i[poolname] - }, - connection_waited: { - mtype: M_HISTOGRAM, - buckets: REDIS_CONNECT_BUCKETS, - docstring: 'vmpooler Redis Connection Wait Time', - prom_metric_prefix: "#{@metrics_prefix}_connection_waited", - param_labels: %i[type provider] + usage_jenkins_instance: { + mtype: M_COUNTER, + docstring: 'Pools by Jenkins instance usage', + prom_metric_prefix: "#{@metrics_prefix}_usage_jenkins_instance", + param_labels: %i[jenkins_instance value_stream poolname] + }, + usage_branch_project: { + mtype: M_COUNTER, + docstring: 'Pools by branch/project usage', + prom_metric_prefix: "#{@metrics_prefix}_usage_branch_project", + param_labels: %i[branch project poolname] + }, + usage_job_component: { + mtype: M_COUNTER, + docstring: 'Pools by job/component usage', + prom_metric_prefix: "#{@metrics_prefix}_usage_job_component", + param_labels: %i[job_name component_to_test poolname] + }, + checkout: { + mtype: M_COUNTER, + docstring: 'Pool checkout counts', + prom_metric_prefix: "#{@metrics_prefix}_checkout", + metric_suffixes: { + nonresponsive: 'checkout failed - non responsive machine', + empty: 'checkout failed - no machine', + success: 'successful checkout', + invalid: 'checkout failed - invalid template' + }, + param_labels: %i[poolname] + }, + config: { + mtype: M_COUNTER, + docstring: 'vmpooler pool configuration request', + prom_metric_prefix: "#{@metrics_prefix}_config", + metric_suffixes: { invalid: 'Invalid' }, + param_labels: %i[poolname] + }, + poolreset: { + mtype: M_COUNTER, + docstring: 'Pool reset counter', + prom_metric_prefix: "#{@metrics_prefix}_poolreset", + metric_suffixes: { invalid: 'Invalid Pool' }, + param_labels: %i[poolname] + }, + connect: { + mtype: M_COUNTER, + docstring: 'vmpooler connect (to vSphere)', + prom_metric_prefix: "#{@metrics_prefix}_connect", + metric_suffixes: { + open: 'Connect Succeeded', + fail: 'Connect Failed' + }, + param_labels: [] + }, + migrate_from: { + mtype: M_COUNTER, + docstring: 'vmpooler machine migrated from', + prom_metric_prefix: "#{@metrics_prefix}_migrate_from", + param_labels: %i[host_name] + }, + migrate_to: { + mtype: M_COUNTER, + docstring: 'vmpooler machine migrated to', + prom_metric_prefix: "#{@metrics_prefix}_migrate_to", + param_labels: %i[host_name] + }, + api_vm: { + mtype: M_COUNTER, + docstring: 'Total number of HTTP request/sub-operations handled by the Rack application under the /vm endpoint', + prom_metric_prefix: "#{@metrics_prefix}_http_requests_vm_total", + param_labels: %i[method subpath operation] + }, + ready: { + mtype: M_GAUGE, + docstring: 'vmpooler number of machines in ready State', + prom_metric_prefix: "#{@metrics_prefix}_ready", + param_labels: %i[poolname] + }, + running: { + mtype: M_GAUGE, + docstring: 'vmpooler number of machines running', + prom_metric_prefix: "#{@metrics_prefix}_running", + param_labels: %i[poolname] + }, + connection_available: { + mtype: M_GAUGE, + docstring: 'vmpooler redis connections available', + prom_metric_prefix: "#{@metrics_prefix}_connection_available", + param_labels: %i[type provider] + }, + time_to_ready_state: { + mtype: M_HISTOGRAM, + buckets: POOLER_TIME_BUCKETS, + docstring: 'Time taken for machine to read ready state for pool', + prom_metric_prefix: "#{@metrics_prefix}_time_to_ready_state", + param_labels: %i[poolname] + }, + migrate: { + mtype: M_HISTOGRAM, + buckets: POOLER_TIME_BUCKETS, + docstring: 'vmpooler time taken to migrate machine for pool', + prom_metric_prefix: "#{@metrics_prefix}_migrate", + param_labels: %i[poolname] + }, + clone: { + mtype: M_HISTOGRAM, + buckets: POOLER_TIME_BUCKETS, + docstring: 'vmpooler time taken to clone machine', + prom_metric_prefix: "#{@metrics_prefix}_clone", + param_labels: %i[poolname] + }, + destroy: { + mtype: M_HISTOGRAM, + buckets: POOLER_TIME_BUCKETS, + docstring: 'vmpooler time taken to destroy machine', + prom_metric_prefix: "#{@metrics_prefix}_destroy", + param_labels: %i[poolname] + }, + connection_waited: { + mtype: M_HISTOGRAM, + buckets: REDIS_CONNECT_BUCKETS, + docstring: 'vmpooler redis connection wait time', + prom_metric_prefix: "#{@metrics_prefix}_connection_waited", + param_labels: %i[type provider] + } } - } - end - - # Helper to add individual prom metric. - # Allow Histograms to specify the bucket size. - def add_prometheus_metric(metric_spec, name, docstring) - case metric_spec[:mtype] - when M_COUNTER - metric_class = Prometheus::Client::Counter - when M_GAUGE - metric_class = Prometheus::Client::Gauge - when M_SUMMARY - metric_class = Prometheus::Client::Summary - when M_HISTOGRAM - metric_class = Prometheus::Client::Histogram - else - raise("Unable to register metric #{name} with metric type #{metric_spec[:mtype]}") end - if (metric_spec[:mtype] == M_HISTOGRAM) && (metric_spec.key? :buckets) - prom_metric = metric_class.new( - name.to_sym, - docstring: docstring, - labels: metric_spec[:param_labels] + [:vmpooler_instance], - buckets: metric_spec[:buckets], - preset_labels: { vmpooler_instance: @prefix } - ) - else - prom_metric = metric_class.new( - name.to_sym, - docstring: docstring, - labels: metric_spec[:param_labels] + [:vmpooler_instance], - preset_labels: { vmpooler_instance: @prefix } - ) - end - @prometheus.register(prom_metric) - end - - # Top level method to register all the prometheus metrics. - - def setup_prometheus_metrics - @p_metrics = vmpooler_metrics_table - @p_metrics.each do |_name, metric_spec| - if metric_spec.key? :metric_suffixes - # Iterate thru the suffixes if provided to register multiple counters here. - metric_spec[:metric_suffixes].each do |metric_suffix| - add_prometheus_metric( - metric_spec, - "#{metric_spec[:prom_metric_prefix]}_#{metric_suffix[0]}", - "#{metric_spec[:docstring]} #{metric_suffix[1]}" - ) - end + # Helper to add individual prom metric. + # Allow Histograms to specify the bucket size. + def add_prometheus_metric(metric_spec, name, docstring) + case metric_spec[:mtype] + when M_COUNTER + metric_class = Prometheus::Client::Counter + when M_GAUGE + metric_class = Prometheus::Client::Gauge + when M_SUMMARY + metric_class = Prometheus::Client::Summary + when M_HISTOGRAM + metric_class = Prometheus::Client::Histogram else - # No Additional counter suffixes so register this as metric. - add_prometheus_metric( - metric_spec, - metric_spec[:prom_metric_prefix], - metric_spec[:docstring] + raise("Unable to register metric #{name} with metric type #{metric_spec[:mtype]}") + end + + if (metric_spec[:mtype] == M_HISTOGRAM) && (metric_spec.key? :buckets) + prom_metric = metric_class.new( + name.to_sym, + docstring: docstring, + labels: metric_spec[:param_labels] + [:vmpooler_instance], + buckets: metric_spec[:buckets], + preset_labels: { vmpooler_instance: @prefix } + ) + else + prom_metric = metric_class.new( + name.to_sym, + docstring: docstring, + labels: metric_spec[:param_labels] + [:vmpooler_instance], + preset_labels: { vmpooler_instance: @prefix } ) end - end - end - - # locate a metric and check/interpet the sub-fields. - def find_metric(label) - sublabels = label.split('.') - metric_key = sublabels.shift.to_sym - raise("Invalid Metric #{metric_key} for #{label}") unless @p_metrics.key? metric_key - - metric = @p_metrics[metric_key].clone - - if metric.key? :metric_suffixes - metric_subkey = sublabels.shift.to_sym - raise("Invalid Metric #{metric_key}_#{metric_subkey} for #{label}") unless metric[:metric_suffixes].key? metric_subkey.to_sym - - metric[:metric_name] = "#{metric[:prom_metric_prefix]}_#{metric_subkey}" - else - metric[:metric_name] = metric[:prom_metric_prefix] + @prometheus.register(prom_metric) end - # Check if we are looking for a parameter value at last element. - if metric.key? :param_labels - metric[:labels] = {} - # Special case processing here - if there is only one parameter label then make sure - # we append all of the remaining contents of the metric with "." separators to ensure - # we get full nodenames (e.g. for Migration to node operations) - if metric[:param_labels].length == 1 - metric[:labels][metric[:param_labels].first] = sublabels.join('.') - else - metric[:param_labels].reverse_each do |param_label| - metric[:labels][param_label] = sublabels.pop(1).first + # Top level method to register all the prometheus metrics. + + def setup_prometheus_metrics + @p_metrics = vmpooler_metrics_table + @p_metrics.each do |_name, metric_spec| + if metric_spec.key? :metric_suffixes + # Iterate thru the suffixes if provided to register multiple counters here. + metric_spec[:metric_suffixes].each do |metric_suffix| + add_prometheus_metric( + metric_spec, + "#{metric_spec[:prom_metric_prefix]}_#{metric_suffix[0]}", + "#{metric_spec[:docstring]} #{metric_suffix[1]}" + ) + end + else + # No Additional counter suffixes so register this as metric. + add_prometheus_metric( + metric_spec, + metric_spec[:prom_metric_prefix], + metric_spec[:docstring] + ) end end end - metric - end - # Helper to get lab metrics. - def get(label) - metric = find_metric(label) - [metric, @prometheus.get(metric[:metric_name])] - end + # locate a metric and check/interpet the sub-fields. + def find_metric(label) + sublabels = label.split('.') + metric_key = sublabels.shift.to_sym + raise("Invalid Metric #{metric_key} for #{label}") unless @p_metrics.key? metric_key - # Note - Catch and log metrics failures so they can be noted, but don't interrupt vmpooler operation. - def increment(label) - begin - counter_metric, c = get(label) - c.increment(labels: counter_metric[:labels]) - rescue StandardError => e - @logger.log('s', "[!] prometheus error logging metric #{label} increment : #{e}") - end - end + metric = @p_metrics[metric_key].clone - def gauge(label, value) - begin - unless value.nil? - gauge_metric, g = get(label) - g.set(value.to_i, labels: gauge_metric[:labels]) + if metric.key? :metric_suffixes + metric_subkey = sublabels.shift.to_sym + raise("Invalid Metric #{metric_key}_#{metric_subkey} for #{label}") unless metric[:metric_suffixes].key? metric_subkey.to_sym + + metric[:metric_name] = "#{metric[:prom_metric_prefix]}_#{metric_subkey}" + else + metric[:metric_name] = metric[:prom_metric_prefix] end - rescue StandardError => e - @logger.log('s', "[!] prometheus error logging gauge #{label}, value #{value}: #{e}") - end - end - def timing(label, duration) - begin - # https://prometheus.io/docs/practices/histograms/ - unless duration.nil? - histogram_metric, hm = get(label) - hm.observe(duration.to_f, labels: histogram_metric[:labels]) + # Check if we are looking for a parameter value at last element. + if metric.key? :param_labels + metric[:labels] = {} + # Special case processing here - if there is only one parameter label then make sure + # we append all of the remaining contents of the metric with "." separators to ensure + # we get full nodenames (e.g. for Migration to node operations) + if metric[:param_labels].length == 1 + metric[:labels][metric[:param_labels].first] = sublabels.join('.') + else + metric[:param_labels].reverse_each do |param_label| + metric[:labels][param_label] = sublabels.pop(1).first + end + end + end + metric + end + + # Helper to get lab metrics. + def get(label) + metric = find_metric(label) + [metric, @prometheus.get(metric[:metric_name])] + end + + # Note - Catch and log metrics failures so they can be noted, but don't interrupt vmpooler operation. + def increment(label) + begin + counter_metric, c = get(label) + c.increment(labels: counter_metric[:labels]) + rescue StandardError => e + @logger.log('s', "[!] prometheus error logging metric #{label} increment : #{e}") + end + end + + def gauge(label, value) + begin + unless value.nil? + gauge_metric, g = get(label) + g.set(value.to_i, labels: gauge_metric[:labels]) + end + rescue StandardError => e + @logger.log('s', "[!] prometheus error logging gauge #{label}, value #{value}: #{e}") + end + end + + def timing(label, duration) + begin + # https://prometheus.io/docs/practices/histograms/ + unless duration.nil? + histogram_metric, hm = get(label) + hm.observe(duration.to_f, labels: histogram_metric[:labels]) + end + rescue StandardError => e + @logger.log('s', "[!] prometheus error logging timing event label #{label}, duration #{duration}: #{e}") end - rescue StandardError => e - @logger.log('s', "[!] prometheus error logging timing event label #{label}, duration #{duration}: #{e}") end end end diff --git a/lib/vmpooler/metrics/statsd.rb b/lib/vmpooler/metrics/statsd.rb index def7b58..85b705d 100644 --- a/lib/vmpooler/metrics/statsd.rb +++ b/lib/vmpooler/metrics/statsd.rb @@ -4,35 +4,37 @@ require 'rubygems' unless defined?(Gem) require 'statsd' module Vmpooler - class Statsd < Metrics - attr_reader :server, :port, :prefix + class Metrics + class Statsd < Metrics + attr_reader :server, :port, :prefix - def initialize(logger, params = {}) - raise ArgumentError, "Statsd server is required. Config: #{params.inspect}" if params['server'].nil? || params['server'].empty? + def initialize(logger, params = {}) + raise ArgumentError, "Statsd server is required. Config: #{params.inspect}" if params['server'].nil? || params['server'].empty? - host = params['server'] - @port = params['port'] || 8125 - @prefix = params['prefix'] || 'vmpooler' - @server = ::Statsd.new(host, @port) - @logger = logger - end + host = params['server'] + @port = params['port'] || 8125 + @prefix = params['prefix'] || 'vmpooler' + @server = ::Statsd.new(host, @port) + @logger = logger + end - def increment(label) - server.increment(prefix + '.' + label) - rescue StandardError => e - @logger.log('s', "[!] Failure incrementing #{prefix}.#{label} on statsd server [#{server}:#{port}]: #{e}") - end + def increment(label) + server.increment(prefix + '.' + label) + rescue StandardError => e + @logger.log('s', "[!] Failure incrementing #{prefix}.#{label} on statsd server [#{server}:#{port}]: #{e}") + end - def gauge(label, value) - server.gauge(prefix + '.' + label, value) - rescue StandardError => e - @logger.log('s', "[!] Failure updating gauge #{prefix}.#{label} on statsd server [#{server}:#{port}]: #{e}") - end + def gauge(label, value) + server.gauge(prefix + '.' + label, value) + rescue StandardError => e + @logger.log('s', "[!] Failure updating gauge #{prefix}.#{label} on statsd server [#{server}:#{port}]: #{e}") + end - def timing(label, duration) - server.timing(prefix + '.' + label, duration) - rescue StandardError => e - @logger.log('s', "[!] Failure updating timing #{prefix}.#{label} on statsd server [#{server}:#{port}]: #{e}") + def timing(label, duration) + server.timing(prefix + '.' + label, duration) + rescue StandardError => e + @logger.log('s', "[!] Failure updating timing #{prefix}.#{label} on statsd server [#{server}:#{port}]: #{e}") + end end end end diff --git a/spec/integration/api/v1/config_spec.rb b/spec/integration/api/v1/config_spec.rb index 7462307..655a2cc 100644 --- a/spec/integration/api/v1/config_spec.rb +++ b/spec/integration/api/v1/config_spec.rb @@ -34,7 +34,7 @@ describe Vmpooler::API::V1 do describe '/config/pooltemplate' do let(:prefix) { '/api/v1' } - let(:metrics) { Vmpooler::DummyStatsd.new } + let(:metrics) { Vmpooler::Metrics::DummyStatsd.new } let(:current_time) { Time.now } diff --git a/spec/integration/api/v1/ondemandvm_spec.rb b/spec/integration/api/v1/ondemandvm_spec.rb index f9b31b2..6ab0076 100644 --- a/spec/integration/api/v1/ondemandvm_spec.rb +++ b/spec/integration/api/v1/ondemandvm_spec.rb @@ -16,7 +16,7 @@ describe Vmpooler::API::V1 do describe '/ondemandvm' do let(:prefix) { '/api/v1' } - let(:metrics) { Vmpooler::DummyStatsd.new } + let(:metrics) { Vmpooler::Metrics::DummyStatsd.new } let(:config) { { config: { diff --git a/spec/integration/api/v1/poolreset.rb b/spec/integration/api/v1/poolreset.rb index 043989e..35e2ba4 100644 --- a/spec/integration/api/v1/poolreset.rb +++ b/spec/integration/api/v1/poolreset.rb @@ -31,7 +31,7 @@ describe Vmpooler::API::V1 do describe '/poolreset' do let(:prefix) { '/api/v1' } - let(:metrics) { Vmpooler::DummyStatsd.new } + let(:metrics) { Vmpooler::Metrics::DummyStatsd.new } let(:current_time) { Time.now } diff --git a/spec/integration/api/v1/vm_hostname_spec.rb b/spec/integration/api/v1/vm_hostname_spec.rb index 18fcec3..25201a8 100644 --- a/spec/integration/api/v1/vm_hostname_spec.rb +++ b/spec/integration/api/v1/vm_hostname_spec.rb @@ -21,7 +21,7 @@ describe Vmpooler::API::V1 do describe '/vm/:hostname' do let(:prefix) { '/api/v1' } - let(:metrics) { Vmpooler::DummyStatsd.new } + let(:metrics) { Vmpooler::Metrics::DummyStatsd.new } let(:config) { { diff --git a/spec/integration/api/v1/vm_spec.rb b/spec/integration/api/v1/vm_spec.rb index 7f5c29a..15b6074 100644 --- a/spec/integration/api/v1/vm_spec.rb +++ b/spec/integration/api/v1/vm_spec.rb @@ -17,7 +17,7 @@ describe Vmpooler::API::V1 do describe '/vm' do let(:prefix) { '/api/v1' } - let(:metrics) { Vmpooler::DummyStatsd.new } + let(:metrics) { Vmpooler::Metrics::DummyStatsd.new } let(:config) { { config: { diff --git a/spec/integration/api/v1/vm_template_spec.rb b/spec/integration/api/v1/vm_template_spec.rb index da83116..839b427 100644 --- a/spec/integration/api/v1/vm_template_spec.rb +++ b/spec/integration/api/v1/vm_template_spec.rb @@ -17,7 +17,7 @@ describe Vmpooler::API::V1 do describe '/vm/:template' do let(:prefix) { '/api/v1' } - let(:metrics) { Vmpooler::DummyStatsd.new } + let(:metrics) { Vmpooler::Metrics::DummyStatsd.new } let(:config) { { config: { diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb index ed7d0ae..446d1ba 100644 --- a/spec/unit/pool_manager_spec.rb +++ b/spec/unit/pool_manager_spec.rb @@ -10,7 +10,7 @@ RSpec::Matchers.define :a_pool_with_name_of do |value| end describe 'Pool Manager' do let(:logger) { MockLogger.new } - let(:metrics) { Vmpooler::DummyStatsd.new } + let(:metrics) { Vmpooler::Metrics::DummyStatsd.new } let(:pool) { 'pool1' } let(:vm) { 'vm1' } let(:timeout) { 5 } diff --git a/spec/unit/promstats_spec.rb b/spec/unit/promstats_spec.rb index d721cce..62c8f4a 100644 --- a/spec/unit/promstats_spec.rb +++ b/spec/unit/promstats_spec.rb @@ -5,12 +5,12 @@ require 'spec_helper' describe 'prometheus' do logger = MockLogger.new params = { 'prefix': 'test', 'metrics_prefix': 'mtest', 'endpoint': 'eptest' } - subject = Vmpooler::Promstats.new(logger, params) + subject = Vmpooler::Metrics::Promstats.new(logger, params) let(:logger) { MockLogger.new } describe '#initialise' do it 'returns a Metrics object' do - expect(Vmpooler::Promstats.new(logger)).to be_a(Vmpooler::Metrics) + expect(Vmpooler::Metrics::Promstats.new(logger)).to be_a(Vmpooler::Metrics) end end @@ -142,6 +142,13 @@ describe 'prometheus' do po.get(labels: metric[:labels]) }.by(1) end + it 'Increments errors.staledns.#{pool_name}' do + pool_name = 'test-pool' + expect { subject.increment("errors.staledns.#{pool_name}") }.to change { + metric, po = subject.get("errors.staledns.#{pool_name}") + po.get(labels: metric[:labels]) + }.by(1) + end it 'Increments user.#{user}.#{poolname}' do user = 'myuser' poolname = 'test-pool' @@ -150,6 +157,14 @@ describe 'prometheus' do po.get(labels: metric[:labels]) }.by(1) end + it 'Increments usage_litmus.#{user}.#{poolname}' do + user = 'myuser' + poolname = 'test-pool' + expect { subject.increment("usage_litmus.#{user}.#{poolname}") }.to change { + metric, po = subject.get("usage_litmus.#{user}.#{poolname}") + po.get(labels: metric[:labels]) + }.by(1) + end it 'Increments label usage_jenkins_instance.#{jenkins_instance}.#{value_stream}.#{poolname}' do jenkins_instance = 'jenkins_test_instance' value_stream = 'notional_value' @@ -177,7 +192,6 @@ describe 'prometheus' do po.get(labels: metric[:labels]) }.by(1) end - it 'Increments connect.open' do expect { subject.increment('connect.open') }.to change { metric, po = subject.get('connect.open') @@ -204,6 +218,16 @@ describe 'prometheus' do po.get(labels: metric[:labels]) }.by(1) end + it 'Increments label api_vm.#{method}.#{subpath}.#{operation}' do + method = 'get' + subpath = 'template' + operation = 'something' + expect { subject.increment("api_vm.#{method}.#{subpath}.#{operation}") }.to change { + metric, po = subject.get("api_vm.#{method}.#{subpath}.#{operation}") + po.get(labels: metric[:labels]) + }.by(1) + end + end describe '#gauge' do diff --git a/spec/unit/providers/base_spec.rb b/spec/unit/providers/base_spec.rb index 27b3d94..1cc2090 100644 --- a/spec/unit/providers/base_spec.rb +++ b/spec/unit/providers/base_spec.rb @@ -6,7 +6,7 @@ require 'vmpooler/providers/base' describe 'Vmpooler::PoolManager::Provider::Base' do let(:logger) { MockLogger.new } - let(:metrics) { Vmpooler::DummyStatsd.new } + let(:metrics) { Vmpooler::Metrics::DummyStatsd.new } let(:config) { {} } let(:provider_name) { 'base' } let(:provider_options) { { 'param' => 'value' } } diff --git a/spec/unit/providers/dummy_spec.rb b/spec/unit/providers/dummy_spec.rb index a22d2e6..f4e4849 100644 --- a/spec/unit/providers/dummy_spec.rb +++ b/spec/unit/providers/dummy_spec.rb @@ -3,7 +3,7 @@ require 'vmpooler/providers/dummy' describe 'Vmpooler::PoolManager::Provider::Dummy' do let(:logger) { MockLogger.new } - let(:metrics) { Vmpooler::DummyStatsd.new } + let(:metrics) { Vmpooler::Metrics::DummyStatsd.new } let(:pool_name) { 'pool1' } let(:other_pool_name) { 'pool2' } let(:vm_name) { 'vm1' } diff --git a/spec/unit/providers/vsphere_spec.rb b/spec/unit/providers/vsphere_spec.rb index 90146b9..a2970ac 100644 --- a/spec/unit/providers/vsphere_spec.rb +++ b/spec/unit/providers/vsphere_spec.rb @@ -41,7 +41,7 @@ end describe 'Vmpooler::PoolManager::Provider::VSphere' do let(:logger) { MockLogger.new } - let(:metrics) { Vmpooler::DummyStatsd.new } + let(:metrics) { Vmpooler::Metrics::DummyStatsd.new } let(:poolname) { 'pool1'} let(:provider_options) { { 'param' => 'value' } } let(:datacenter_name) { 'MockDC' } From a21d8c56425ddaf7a2f8ceb1b26887e246caa8ad Mon Sep 17 00:00:00 2001 From: John O'Connor Date: Thu, 25 Jun 2020 19:44:26 +0100 Subject: [PATCH 10/12] (POOLER-178) Target Stats for api & manager Ensure that the correct stats are registered for the Manager and the api respectively. E.g. all checkout counters are for the api only, whereas clone times belong to the manager. Also new ondemand functionality stats weren't registered, so add these along with missing delete stats. --- bin/vmpooler | 10 ++-- lib/vmpooler/api.rb | 4 +- lib/vmpooler/api/v1.rb | 17 +++--- lib/vmpooler/metrics/promstats.rb | 59 ++++++++++++++++++++- spec/integration/api/v1/config_spec.rb | 2 +- spec/integration/api/v1/ondemandvm_spec.rb | 2 +- spec/integration/api/v1/poolreset.rb | 2 +- spec/integration/api/v1/status_spec.rb | 2 +- spec/integration/api/v1/token_spec.rb | 4 +- spec/integration/api/v1/vm_hostname_spec.rb | 2 +- spec/integration/api/v1/vm_spec.rb | 2 +- spec/integration/api/v1/vm_template_spec.rb | 2 +- spec/integration/dashboard_spec.rb | 2 +- spec/unit/promstats_spec.rb | 46 +++++++++++++++- 14 files changed, 128 insertions(+), 28 deletions(-) diff --git a/bin/vmpooler b/bin/vmpooler index 214480e..44922ab 100755 --- a/bin/vmpooler +++ b/bin/vmpooler @@ -16,15 +16,15 @@ metrics = Vmpooler::Metrics.init(logger, config) torun_threads = [] if ARGV.count == 0 - torun = ['api', 'manager'] + torun = %i[api manager] else torun = [] - torun << 'api' if ARGV.include? 'api' - torun << 'manager' if ARGV.include? 'manager' + torun << :api if ARGV.include? 'api' + torun << :manager if ARGV.include? 'manager' exit(2) if torun.empty? end -if torun.include? 'api' +if torun.include? :api api = Thread.new do redis = Vmpooler.new_redis(redis_host, redis_port, redis_password) Vmpooler::API.execute(torun, config, redis, metrics) @@ -38,7 +38,7 @@ elsif metrics.respond_to?(:setup_prometheus_metrics) torun_threads << prometheus_only_api end -if torun.include? 'manager' +if torun.include? :manager manager = Thread.new do Vmpooler::PoolManager.new( config, diff --git a/lib/vmpooler/api.rb b/lib/vmpooler/api.rb index cf516a7..dd527ac 100644 --- a/lib/vmpooler/api.rb +++ b/lib/vmpooler/api.rb @@ -33,7 +33,7 @@ module Vmpooler if metrics.respond_to?(:setup_prometheus_metrics) # Prometheus metrics are only setup if actually specified # in the config file. - metrics.setup_prometheus_metrics + metrics.setup_prometheus_metrics(torun) # Using customised collector that filters out hostnames on API paths require 'vmpooler/metrics/promstats/collector_middleware' @@ -42,7 +42,7 @@ module Vmpooler use Prometheus::Middleware::Exporter, path: metrics.endpoint end - if torun.include? 'api' + if torun.include? :api use Vmpooler::Dashboard use Vmpooler::API::Dashboard use Vmpooler::API::Reroute diff --git a/lib/vmpooler/api/v1.rb b/lib/vmpooler/api/v1.rb index c5cb0bf..c957f28 100644 --- a/lib/vmpooler/api/v1.rb +++ b/lib/vmpooler/api/v1.rb @@ -339,7 +339,7 @@ module Vmpooler payload&.each do |poolname, count| next unless count.to_i > config['max_ondemand_instances_per_request'] - metrics.increment('ondemandrequest.toomanyrequests.' + poolname) + metrics.increment('ondemandrequest_fail.toomanyrequests.' + poolname) return true end false @@ -363,7 +363,7 @@ module Vmpooler if backend.exists?("vmpooler__odrequest__#{request_id}") result['message'] = "request_id '#{request_id}' has already been created" status 409 - metrics.increment('ondemandrequest.generate.duplicaterequests') + metrics.increment('ondemandrequest_generate.duplicaterequests') return result end @@ -387,7 +387,7 @@ module Vmpooler result['domain'] = config['domain'] if config['domain'] result[:ok] = true - metrics.increment('ondemandrequest.generate.success') + metrics.increment('ondemandrequest_generate.success') result end @@ -825,12 +825,12 @@ module Vmpooler else result[:bad_templates] = invalid invalid.each do |bad_template| - metrics.increment('ondemandrequest.invalid.' + bad_template) + metrics.increment('ondemandrequest_fail.invalid.' + bad_template) end status 404 end else - metrics.increment('ondemandrequest.invalid.unknown') + metrics.increment('ondemandrequest_fail.invalid.unknown') status 404 end rescue JSON::ParserError @@ -860,12 +860,12 @@ module Vmpooler else result[:bad_templates] = invalid invalid.each do |bad_template| - metrics.increment('ondemandrequest.invalid.' + bad_template) + metrics.increment('ondemandrequest_fail.invalid.' + bad_template) end status 404 end else - metrics.increment('ondemandrequest.invalid.unknown') + metrics.increment('ondemandrequest_fail.invalid.unknown') status 404 end @@ -1154,8 +1154,9 @@ module Vmpooler status 200 result['ok'] = true + metrics.increment('delete.success') else - metrics.increment('delete.srem.failed') + metrics.increment('delete.failed') end end diff --git a/lib/vmpooler/metrics/promstats.rb b/lib/vmpooler/metrics/promstats.rb index e385d17..3be0726 100644 --- a/lib/vmpooler/metrics/promstats.rb +++ b/lib/vmpooler/metrics/promstats.rb @@ -36,6 +36,7 @@ module Vmpooler { errors: { mtype: M_COUNTER, + torun: %i[manager], docstring: 'Count of errors for pool', prom_metric_prefix: "#{@metrics_prefix}_errors", metric_suffixes: { @@ -47,36 +48,42 @@ module Vmpooler }, user: { mtype: M_COUNTER, + torun: %i[manager], docstring: 'Number of pool instances this user created created', prom_metric_prefix: "#{@metrics_prefix}_user", param_labels: %i[user poolname] }, usage_litmus: { mtype: M_COUNTER, + torun: %i[manager], docstring: 'Pools by Litmus job usage', prom_metric_prefix: "#{@metrics_prefix}_usage_litmus", param_labels: %i[user poolname] }, usage_jenkins_instance: { mtype: M_COUNTER, + torun: %i[manager], docstring: 'Pools by Jenkins instance usage', prom_metric_prefix: "#{@metrics_prefix}_usage_jenkins_instance", param_labels: %i[jenkins_instance value_stream poolname] }, usage_branch_project: { mtype: M_COUNTER, + torun: %i[manager], docstring: 'Pools by branch/project usage', prom_metric_prefix: "#{@metrics_prefix}_usage_branch_project", param_labels: %i[branch project poolname] }, usage_job_component: { mtype: M_COUNTER, + torun: %i[manager], docstring: 'Pools by job/component usage', prom_metric_prefix: "#{@metrics_prefix}_usage_job_component", param_labels: %i[job_name component_to_test poolname] }, checkout: { mtype: M_COUNTER, + torun: %i[api], docstring: 'Pool checkout counts', prom_metric_prefix: "#{@metrics_prefix}_checkout", metric_suffixes: { @@ -87,8 +94,42 @@ module Vmpooler }, param_labels: %i[poolname] }, + delete: { + mtype: M_COUNTER, + torun: %i[api], + docstring: 'Delete machine', + prom_metric_prefix: "#{@metrics_prefix}_delete", + metric_suffixes: { + success: 'succeeded', + failed: 'failed' + }, + param_labels: [] + }, + ondemandrequest_generate: { + mtype: M_COUNTER, + torun: %i[api], + docstring: 'Ondemand request', + prom_metric_prefix: "#{@metrics_prefix}_ondemandrequest_generate", + metric_suffixes: { + duplicaterequests: 'failed duplicate request', + success: 'succeeded' + }, + param_labels: [] + }, + ondemandrequest_fail: { + mtype: M_COUNTER, + torun: %i[api], + docstring: 'Ondemand request failure', + prom_metric_prefix: "#{@metrics_prefix}_ondemandrequest_fail", + metric_suffixes: { + toomanyrequests: 'too many requests', + invalid: 'invalid poolname' + }, + param_labels: %i[poolname] + }, config: { mtype: M_COUNTER, + torun: %i[api], docstring: 'vmpooler pool configuration request', prom_metric_prefix: "#{@metrics_prefix}_config", metric_suffixes: { invalid: 'Invalid' }, @@ -96,6 +137,7 @@ module Vmpooler }, poolreset: { mtype: M_COUNTER, + torun: %i[api], docstring: 'Pool reset counter', prom_metric_prefix: "#{@metrics_prefix}_poolreset", metric_suffixes: { invalid: 'Invalid Pool' }, @@ -103,6 +145,7 @@ module Vmpooler }, connect: { mtype: M_COUNTER, + torun: %i[manager], docstring: 'vmpooler connect (to vSphere)', prom_metric_prefix: "#{@metrics_prefix}_connect", metric_suffixes: { @@ -113,42 +156,49 @@ module Vmpooler }, migrate_from: { mtype: M_COUNTER, + torun: %i[manager], docstring: 'vmpooler machine migrated from', prom_metric_prefix: "#{@metrics_prefix}_migrate_from", param_labels: %i[host_name] }, migrate_to: { mtype: M_COUNTER, + torun: %i[manager], docstring: 'vmpooler machine migrated to', prom_metric_prefix: "#{@metrics_prefix}_migrate_to", param_labels: %i[host_name] }, api_vm: { mtype: M_COUNTER, + torun: %i[api], docstring: 'Total number of HTTP request/sub-operations handled by the Rack application under the /vm endpoint', prom_metric_prefix: "#{@metrics_prefix}_http_requests_vm_total", param_labels: %i[method subpath operation] }, ready: { mtype: M_GAUGE, + torun: %i[manager], docstring: 'vmpooler number of machines in ready State', prom_metric_prefix: "#{@metrics_prefix}_ready", param_labels: %i[poolname] }, running: { mtype: M_GAUGE, + torun: %i[manager], docstring: 'vmpooler number of machines running', prom_metric_prefix: "#{@metrics_prefix}_running", param_labels: %i[poolname] }, connection_available: { mtype: M_GAUGE, + torun: %i[manager], docstring: 'vmpooler redis connections available', prom_metric_prefix: "#{@metrics_prefix}_connection_available", param_labels: %i[type provider] }, time_to_ready_state: { mtype: M_HISTOGRAM, + torun: %i[manager], buckets: POOLER_TIME_BUCKETS, docstring: 'Time taken for machine to read ready state for pool', prom_metric_prefix: "#{@metrics_prefix}_time_to_ready_state", @@ -156,6 +206,7 @@ module Vmpooler }, migrate: { mtype: M_HISTOGRAM, + torun: %i[manager], buckets: POOLER_TIME_BUCKETS, docstring: 'vmpooler time taken to migrate machine for pool', prom_metric_prefix: "#{@metrics_prefix}_migrate", @@ -163,6 +214,7 @@ module Vmpooler }, clone: { mtype: M_HISTOGRAM, + torun: %i[manager], buckets: POOLER_TIME_BUCKETS, docstring: 'vmpooler time taken to clone machine', prom_metric_prefix: "#{@metrics_prefix}_clone", @@ -170,6 +222,7 @@ module Vmpooler }, destroy: { mtype: M_HISTOGRAM, + torun: %i[manager], buckets: POOLER_TIME_BUCKETS, docstring: 'vmpooler time taken to destroy machine', prom_metric_prefix: "#{@metrics_prefix}_destroy", @@ -177,6 +230,7 @@ module Vmpooler }, connection_waited: { mtype: M_HISTOGRAM, + torun: %i[manager], buckets: REDIS_CONNECT_BUCKETS, docstring: 'vmpooler redis connection wait time', prom_metric_prefix: "#{@metrics_prefix}_connection_waited", @@ -222,9 +276,12 @@ module Vmpooler # Top level method to register all the prometheus metrics. - def setup_prometheus_metrics + def setup_prometheus_metrics(torun) @p_metrics = vmpooler_metrics_table @p_metrics.each do |_name, metric_spec| + # Only register metrics appropriate to api or manager + next if (torun & metric_spec[:torun]).empty? + if metric_spec.key? :metric_suffixes # Iterate thru the suffixes if provided to register multiple counters here. metric_spec[:metric_suffixes].each do |metric_suffix| diff --git a/spec/integration/api/v1/config_spec.rb b/spec/integration/api/v1/config_spec.rb index 655a2cc..6bcd385 100644 --- a/spec/integration/api/v1/config_spec.rb +++ b/spec/integration/api/v1/config_spec.rb @@ -40,7 +40,7 @@ describe Vmpooler::API::V1 do before(:each) do expect(app).to receive(:run!).once - app.execute(['api'], config, redis, metrics) + app.execute([:api], config, redis, metrics) app.settings.set :config, auth: false create_token('abcdefghijklmnopqrstuvwxyz012345', 'jdoe', current_time) end diff --git a/spec/integration/api/v1/ondemandvm_spec.rb b/spec/integration/api/v1/ondemandvm_spec.rb index 6ab0076..0af5bb1 100644 --- a/spec/integration/api/v1/ondemandvm_spec.rb +++ b/spec/integration/api/v1/ondemandvm_spec.rb @@ -43,7 +43,7 @@ describe Vmpooler::API::V1 do before(:each) do expect(app).to receive(:run!).once - app.execute(['api'], config, redis, metrics) + app.execute([:api], config, redis, metrics) app.settings.set :config, auth: false app.settings.set :checkoutlock, checkoutlock create_token('abcdefghijklmnopqrstuvwxyz012345', 'jdoe', current_time) diff --git a/spec/integration/api/v1/poolreset.rb b/spec/integration/api/v1/poolreset.rb index 35e2ba4..c393138 100644 --- a/spec/integration/api/v1/poolreset.rb +++ b/spec/integration/api/v1/poolreset.rb @@ -37,7 +37,7 @@ describe Vmpooler::API::V1 do before(:each) do expect(app).to receive(:run!).once - app.execute(['api'], config, redis, metrics) + app.execute([:api], config, redis, metrics) app.settings.set :config, auth: false create_token('abcdefghijklmnopqrstuvwxyz012345', 'jdoe', current_time) end diff --git a/spec/integration/api/v1/status_spec.rb b/spec/integration/api/v1/status_spec.rb index 486c263..7c22ebf 100644 --- a/spec/integration/api/v1/status_spec.rb +++ b/spec/integration/api/v1/status_spec.rb @@ -40,7 +40,7 @@ describe Vmpooler::API::V1 do before(:each) do expect(app).to receive(:run!).once - app.execute(['api'], config, redis, nil) + app.execute([:api], config, redis, nil) app.settings.set :config, auth: false create_token('abcdefghijklmnopqrstuvwxyz012345', 'jdoe', current_time) end diff --git a/spec/integration/api/v1/token_spec.rb b/spec/integration/api/v1/token_spec.rb index 72e60bd..d4f33f4 100644 --- a/spec/integration/api/v1/token_spec.rb +++ b/spec/integration/api/v1/token_spec.rb @@ -22,7 +22,7 @@ describe Vmpooler::API::V1 do before(:each) do expect(app).to receive(:run!).once - app.execute(['api'], config, redis, nil) + app.execute([:api], config, redis, nil) end describe 'GET /token' do @@ -106,7 +106,7 @@ describe Vmpooler::API::V1 do before(:each) do expect(app).to receive(:run!).once - app.execute(['api'], config, redis, nil) + app.execute([:api], config, redis, nil) app.settings.set :config, config app.settings.set :redis, redis end diff --git a/spec/integration/api/v1/vm_hostname_spec.rb b/spec/integration/api/v1/vm_hostname_spec.rb index 25201a8..ba2b460 100644 --- a/spec/integration/api/v1/vm_hostname_spec.rb +++ b/spec/integration/api/v1/vm_hostname_spec.rb @@ -43,7 +43,7 @@ describe Vmpooler::API::V1 do before(:each) do expect(app).to receive(:run!).once - app.execute(['api'], config, redis, metrics) + app.execute([:api], config, redis, metrics) create_token('abcdefghijklmnopqrstuvwxyz012345', 'jdoe', current_time) end diff --git a/spec/integration/api/v1/vm_spec.rb b/spec/integration/api/v1/vm_spec.rb index 15b6074..f6b2cc9 100644 --- a/spec/integration/api/v1/vm_spec.rb +++ b/spec/integration/api/v1/vm_spec.rb @@ -40,7 +40,7 @@ describe Vmpooler::API::V1 do before(:each) do expect(app).to receive(:run!).once - app.execute(['api'], config, redis, metrics) + app.execute([:api], config, redis, metrics) app.settings.set :config, auth: false app.settings.set :checkoutlock, checkoutlock create_token('abcdefghijklmnopqrstuvwxyz012345', 'jdoe', current_time) diff --git a/spec/integration/api/v1/vm_template_spec.rb b/spec/integration/api/v1/vm_template_spec.rb index 839b427..a306d87 100644 --- a/spec/integration/api/v1/vm_template_spec.rb +++ b/spec/integration/api/v1/vm_template_spec.rb @@ -41,7 +41,7 @@ describe Vmpooler::API::V1 do before(:each) do expect(app).to receive(:run!).once - app.execute(['api'], config, redis, metrics) + app.execute([:api], config, redis, metrics) app.settings.set :config, auth: false app.settings.set :checkoutlock, checkoutlock create_token('abcdefghijklmnopqrstuvwxyz012345', 'jdoe', current_time) diff --git a/spec/integration/dashboard_spec.rb b/spec/integration/dashboard_spec.rb index 32929b7..8d87ac4 100644 --- a/spec/integration/dashboard_spec.rb +++ b/spec/integration/dashboard_spec.rb @@ -26,7 +26,7 @@ describe Vmpooler::API do before(:each) do expect(app).to receive(:run!) - app.execute(['api'], config, redis, nil) + app.execute([:api], config, redis, nil) app.settings.set :config, auth: false end diff --git a/spec/unit/promstats_spec.rb b/spec/unit/promstats_spec.rb index 62c8f4a..eb66c5f 100644 --- a/spec/unit/promstats_spec.rb +++ b/spec/unit/promstats_spec.rb @@ -55,7 +55,7 @@ describe 'prometheus' do context 'setup_prometheus_metrics' do before(:all) do Prometheus::Client.config.data_store = Prometheus::Client::DataStores::Synchronized.new - subject.setup_prometheus_metrics + subject.setup_prometheus_metrics(%i[api manager]) end let(:MCOUNTER) { 1 } @@ -63,7 +63,7 @@ describe 'prometheus' do it 'calls add_prometheus_metric for each item in list' do Prometheus::Client.config.data_store = Prometheus::Client::DataStores::Synchronized.new expect(subject).to receive(:add_prometheus_metric).at_least(subject.vmpooler_metrics_table.size).times - subject.setup_prometheus_metrics + subject.setup_prometheus_metrics(%i[api manager]) end end @@ -102,6 +102,48 @@ describe 'prometheus' do po.get(labels: metric[:labels]) }.by(1) end + it 'Increments delete.failed' do + bad_template = 'test-template' + expect { subject.increment('delete.failed') }.to change { + metric, po = subject.get('delete.failed') + po.get(labels: metric[:labels]) + }.by(1) + end + it 'Increments delete.success' do + bad_template = 'test-template' + expect { subject.increment('delete.success') }.to change { + metric, po = subject.get('delete.success') + po.get(labels: metric[:labels]) + }.by(1) + end + it 'Increments ondemandrequest_generate.duplicaterequests' do + bad_template = 'test-template' + expect { subject.increment('ondemandrequest_generate.duplicaterequests') }.to change { + metric, po = subject.get('ondemandrequest_generate.duplicaterequests') + po.get(labels: metric[:labels]) + }.by(1) + end + it 'Increments ondemandrequest_generate.success' do + bad_template = 'test-template' + expect { subject.increment('ondemandrequest_generate.success') }.to change { + metric, po = subject.get('ondemandrequest_generate.success') + po.get(labels: metric[:labels]) + }.by(1) + end + it 'Increments ondemandrequest_fail.toomanyrequests.#{bad_template}' do + bad_template = 'test-template' + expect { subject.increment("ondemandrequest_fail.toomanyrequests.#{bad_template}") }.to change { + metric, po = subject.get("ondemandrequest_fail.toomanyrequests.#{bad_template}") + po.get(labels: metric[:labels]) + }.by(1) + end + it 'Increments ondemandrequest_fail.invalid.#{bad_template}' do + bad_template = 'test-template' + expect { subject.increment("ondemandrequest_fail.invalid.#{bad_template}") }.to change { + metric, po = subject.get("ondemandrequest_fail.invalid.#{bad_template}") + po.get(labels: metric[:labels]) + }.by(1) + end it 'Increments config.invalid.#{bad_template}' do bad_template = 'test-template' expect { subject.increment("config.invalid.#{bad_template}") }.to change { From 85ff3f7022e52465b24a0e8ca34f74b15b6e2ac5 Mon Sep 17 00:00:00 2001 From: John O'Connor Date: Fri, 26 Jun 2020 20:49:17 +0100 Subject: [PATCH 11/12] (MAINT) Add optional API Request Logging This was partially an exercise to use middleware, but also to enable basic logging for the API by logging all the API calls to the logger. --- bin/vmpooler | 4 +-- docs/configuration.md | 7 ++++ lib/vmpooler.rb | 1 + lib/vmpooler/api.rb | 5 ++- lib/vmpooler/api/request_logger.rb | 20 ++++++++++++ .../metrics/promstats/collector_middleware.rb | 14 ++++---- spec/integration/api/v1/config_spec.rb | 2 +- spec/integration/api/v1/ondemandvm_spec.rb | 2 +- spec/integration/api/v1/poolreset.rb | 2 +- spec/integration/api/v1/status_spec.rb | 2 +- spec/integration/api/v1/token_spec.rb | 32 +++++++++++++++---- spec/integration/api/v1/vm_hostname_spec.rb | 2 +- spec/integration/api/v1/vm_spec.rb | 2 +- spec/integration/api/v1/vm_template_spec.rb | 2 +- spec/integration/dashboard_spec.rb | 5 ++- spec/unit/env_config.rb | 1 + vmpooler.yaml.example | 6 ++++ 17 files changed, 85 insertions(+), 24 deletions(-) create mode 100644 lib/vmpooler/api/request_logger.rb diff --git a/bin/vmpooler b/bin/vmpooler index 44922ab..139d79c 100755 --- a/bin/vmpooler +++ b/bin/vmpooler @@ -27,13 +27,13 @@ end if torun.include? :api api = Thread.new do redis = Vmpooler.new_redis(redis_host, redis_port, redis_password) - Vmpooler::API.execute(torun, config, redis, metrics) + Vmpooler::API.execute(torun, config, redis, metrics, logger) end torun_threads << api elsif metrics.respond_to?(:setup_prometheus_metrics) # Run the cut down API - Prometheus Metrics only. prometheus_only_api = Thread.new do - Vmpooler::API.execute(torun, config, nil, metrics) + Vmpooler::API.execute(torun, config, nil, metrics, logger) end torun_threads << prometheus_only_api end diff --git a/docs/configuration.md b/docs/configuration.md index 4846b94..d49debb 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -191,6 +191,13 @@ https://jenkins.example.com/job/platform\_puppet-agent-extra\_puppet-agent-integ Expects a boolean value (optional; default: false) +### REQUEST\_LOGGER + +Enable API request logging to the logger +When enabled all requests to the API are written to the standard logger. +Expects a boolean value +(optional; default: false) + ### EXTRA\_CONFIG Specify additional application configuration files diff --git a/lib/vmpooler.rb b/lib/vmpooler.rb index 20eb26b..276bfa7 100644 --- a/lib/vmpooler.rb +++ b/lib/vmpooler.rb @@ -84,6 +84,7 @@ module Vmpooler parsed_config[:config]['experimental_features'] = ENV['EXPERIMENTAL_FEATURES'] if ENV['EXPERIMENTAL_FEATURES'] parsed_config[:config]['purge_unconfigured_folders'] = ENV['PURGE_UNCONFIGURED_FOLDERS'] if ENV['PURGE_UNCONFIGURED_FOLDERS'] parsed_config[:config]['usage_stats'] = ENV['USAGE_STATS'] if ENV['USAGE_STATS'] + parsed_config[:config]['request_logger'] = ENV['REQUEST_LOGGER'] if ENV['REQUEST_LOGGER'] parsed_config[:redis] = parsed_config[:redis] || {} parsed_config[:redis]['server'] = ENV['REDIS_SERVER'] || parsed_config[:redis]['server'] || 'localhost' diff --git a/lib/vmpooler/api.rb b/lib/vmpooler/api.rb index dd527ac..9ffe386 100644 --- a/lib/vmpooler/api.rb +++ b/lib/vmpooler/api.rb @@ -9,7 +9,7 @@ module Vmpooler # Load dashboard components require 'vmpooler/dashboard' - def self.execute(torun, config, redis, metrics) + def self.execute(torun, config, redis, metrics, logger) self.settings.set :config, config self.settings.set :redis, redis unless redis.nil? self.settings.set :metrics, metrics @@ -43,6 +43,9 @@ module Vmpooler end if torun.include? :api + # Enable API request logging only if required + use Vmpooler::API::RequestLogger, logger: logger if config[:config]['request_logger'] + use Vmpooler::Dashboard use Vmpooler::API::Dashboard use Vmpooler::API::Reroute diff --git a/lib/vmpooler/api/request_logger.rb b/lib/vmpooler/api/request_logger.rb new file mode 100644 index 0000000..d31c190 --- /dev/null +++ b/lib/vmpooler/api/request_logger.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +module Vmpooler + class API + class RequestLogger + attr_reader :app + + def initialize(app, options = {}) + @app = app + @logger = options[:logger] + end + + def call(env) + status, headers, body = @app.call(env) + @logger.log('s', "[ ] API: Method: #{env['REQUEST_METHOD']}, Status: #{status}, Path: #{env['PATH_INFO']}, Body: #{body}") + [status, headers, body] + end + end + end +end diff --git a/lib/vmpooler/metrics/promstats/collector_middleware.rb b/lib/vmpooler/metrics/promstats/collector_middleware.rb index c05c866..bfaa7a0 100644 --- a/lib/vmpooler/metrics/promstats/collector_middleware.rb +++ b/lib/vmpooler/metrics/promstats/collector_middleware.rb @@ -6,9 +6,10 @@ # # The code was also failing Rubocop on PR check, so have addressed all the offenses. # -# The method strip_ids_from_path has been adapted to add a match for hostnames in paths -# to replace with a single ":hostname" string to avoid proliferation of stat lines for -# each new vm hostname deleted, modified or otherwise queried. +# The method strip_hostnames_from_path (originally strip_ids_from_path) has been adapted +# to add a match for hostnames in paths # to replace with a single ":hostname" string to +# avoid # proliferation of stat lines for # each new vm hostname deleted, modified or +# otherwise queried. require 'benchmark' require 'prometheus/client' @@ -91,12 +92,12 @@ module Vmpooler counter_labels = { code: code, method: env['REQUEST_METHOD'].downcase, - path: strip_ids_from_path(env['PATH_INFO']) + path: strip_hostnames_from_path(env['PATH_INFO']) } duration_labels = { method: env['REQUEST_METHOD'].downcase, - path: strip_ids_from_path(env['PATH_INFO']) + path: strip_hostnames_from_path(env['PATH_INFO']) } @requests.increment(labels: counter_labels) @@ -105,10 +106,11 @@ module Vmpooler nil end - def strip_ids_from_path(path) + def strip_hostnames_from_path(path) # Custom for /vm path - so we just collect aggrate stats for all usage along this one # path. Custom counters are then added more specific endpoints in v1.rb # Since we aren't parsing UID/GIDs as in the original example, these are removed. + # Similarly, request IDs are also stripped from the /ondemand path. path .gsub(%r{/vm/.+$}, '/vm') .gsub(%r{/ondemand/.+$}, '/ondemand') diff --git a/spec/integration/api/v1/config_spec.rb b/spec/integration/api/v1/config_spec.rb index 6bcd385..79874c8 100644 --- a/spec/integration/api/v1/config_spec.rb +++ b/spec/integration/api/v1/config_spec.rb @@ -40,7 +40,7 @@ describe Vmpooler::API::V1 do before(:each) do expect(app).to receive(:run!).once - app.execute([:api], config, redis, metrics) + app.execute([:api], config, redis, metrics, nil) app.settings.set :config, auth: false create_token('abcdefghijklmnopqrstuvwxyz012345', 'jdoe', current_time) end diff --git a/spec/integration/api/v1/ondemandvm_spec.rb b/spec/integration/api/v1/ondemandvm_spec.rb index 0af5bb1..37b315c 100644 --- a/spec/integration/api/v1/ondemandvm_spec.rb +++ b/spec/integration/api/v1/ondemandvm_spec.rb @@ -43,7 +43,7 @@ describe Vmpooler::API::V1 do before(:each) do expect(app).to receive(:run!).once - app.execute([:api], config, redis, metrics) + app.execute([:api], config, redis, metrics, nil) app.settings.set :config, auth: false app.settings.set :checkoutlock, checkoutlock create_token('abcdefghijklmnopqrstuvwxyz012345', 'jdoe', current_time) diff --git a/spec/integration/api/v1/poolreset.rb b/spec/integration/api/v1/poolreset.rb index c393138..dfb7f18 100644 --- a/spec/integration/api/v1/poolreset.rb +++ b/spec/integration/api/v1/poolreset.rb @@ -37,7 +37,7 @@ describe Vmpooler::API::V1 do before(:each) do expect(app).to receive(:run!).once - app.execute([:api], config, redis, metrics) + app.execute([:api], config, redis, metrics, nil) app.settings.set :config, auth: false create_token('abcdefghijklmnopqrstuvwxyz012345', 'jdoe', current_time) end diff --git a/spec/integration/api/v1/status_spec.rb b/spec/integration/api/v1/status_spec.rb index 7c22ebf..76e65df 100644 --- a/spec/integration/api/v1/status_spec.rb +++ b/spec/integration/api/v1/status_spec.rb @@ -40,7 +40,7 @@ describe Vmpooler::API::V1 do before(:each) do expect(app).to receive(:run!).once - app.execute([:api], config, redis, nil) + app.execute([:api], config, redis, nil, nil) app.settings.set :config, auth: false create_token('abcdefghijklmnopqrstuvwxyz012345', 'jdoe', current_time) end diff --git a/spec/integration/api/v1/token_spec.rb b/spec/integration/api/v1/token_spec.rb index d4f33f4..72ab060 100644 --- a/spec/integration/api/v1/token_spec.rb +++ b/spec/integration/api/v1/token_spec.rb @@ -18,16 +18,21 @@ describe Vmpooler::API::V1 do describe '/token' do let(:prefix) { '/api/v1' } let(:current_time) { Time.now } - let(:config) { { } } + let(:config) { { + config: {} + } } before(:each) do expect(app).to receive(:run!).once - app.execute([:api], config, redis, nil) + app.execute([:api], config, redis, nil, nil) end describe 'GET /token' do context '(auth not configured)' do - let(:config) { { auth: false } } + let(:config) { { + config: {}, + auth: false + } } it 'returns a 404' do get "#{prefix}/token" @@ -38,6 +43,7 @@ describe Vmpooler::API::V1 do context '(auth configured)' do let(:config) { { + config: {}, auth: { 'provider' => 'dummy' } @@ -65,7 +71,10 @@ describe Vmpooler::API::V1 do describe 'POST /token' do context '(auth not configured)' do - let(:config) { { auth: false } } + let(:config) { { + config: {}, + auth: false + } } it 'returns a 404' do post "#{prefix}/token" @@ -76,6 +85,7 @@ describe Vmpooler::API::V1 do context '(auth configured)' do let(:config) { { + config: {}, auth: { 'provider' => 'dummy' } @@ -106,7 +116,7 @@ describe Vmpooler::API::V1 do before(:each) do expect(app).to receive(:run!).once - app.execute([:api], config, redis, nil) + app.execute([:api], config, redis, nil, nil) app.settings.set :config, config app.settings.set :redis, redis end @@ -118,7 +128,10 @@ describe Vmpooler::API::V1 do describe 'GET /token/:token' do context '(auth not configured)' do - let(:config) { { auth: false } } + let(:config) { { + config: {}, + auth: false + } } it 'returns a 404' do get "#{prefix}/token/this" @@ -128,6 +141,7 @@ describe Vmpooler::API::V1 do context '(auth configured)' do let(:config) { { + config: {}, auth: true, pools: [ {'name' => 'pool1', 'size' => 5} @@ -150,7 +164,10 @@ describe Vmpooler::API::V1 do describe 'DELETE /token/:token' do context '(auth not configured)' do - let(:config) { { auth: false } } + let(:config) { { + config: {}, + auth: false + } } it 'returns a 404' do delete "#{prefix}/token/this" @@ -161,6 +178,7 @@ describe Vmpooler::API::V1 do context '(auth configured)' do let(:config) { { + config: {}, auth: { 'provider' => 'dummy' } diff --git a/spec/integration/api/v1/vm_hostname_spec.rb b/spec/integration/api/v1/vm_hostname_spec.rb index ba2b460..ff5f7a3 100644 --- a/spec/integration/api/v1/vm_hostname_spec.rb +++ b/spec/integration/api/v1/vm_hostname_spec.rb @@ -43,7 +43,7 @@ describe Vmpooler::API::V1 do before(:each) do expect(app).to receive(:run!).once - app.execute([:api], config, redis, metrics) + app.execute([:api], config, redis, metrics, nil) create_token('abcdefghijklmnopqrstuvwxyz012345', 'jdoe', current_time) end diff --git a/spec/integration/api/v1/vm_spec.rb b/spec/integration/api/v1/vm_spec.rb index f6b2cc9..0d06585 100644 --- a/spec/integration/api/v1/vm_spec.rb +++ b/spec/integration/api/v1/vm_spec.rb @@ -40,7 +40,7 @@ describe Vmpooler::API::V1 do before(:each) do expect(app).to receive(:run!).once - app.execute([:api], config, redis, metrics) + app.execute([:api], config, redis, metrics, nil) app.settings.set :config, auth: false app.settings.set :checkoutlock, checkoutlock create_token('abcdefghijklmnopqrstuvwxyz012345', 'jdoe', current_time) diff --git a/spec/integration/api/v1/vm_template_spec.rb b/spec/integration/api/v1/vm_template_spec.rb index a306d87..25c0fbd 100644 --- a/spec/integration/api/v1/vm_template_spec.rb +++ b/spec/integration/api/v1/vm_template_spec.rb @@ -41,7 +41,7 @@ describe Vmpooler::API::V1 do before(:each) do expect(app).to receive(:run!).once - app.execute([:api], config, redis, metrics) + app.execute([:api], config, redis, metrics, nil) app.settings.set :config, auth: false app.settings.set :checkoutlock, checkoutlock create_token('abcdefghijklmnopqrstuvwxyz012345', 'jdoe', current_time) diff --git a/spec/integration/dashboard_spec.rb b/spec/integration/dashboard_spec.rb index 8d87ac4..b0abbea 100644 --- a/spec/integration/dashboard_spec.rb +++ b/spec/integration/dashboard_spec.rb @@ -18,6 +18,7 @@ describe Vmpooler::API do describe 'Dashboard' do let(:config) { { + config: {}, pools: [ {'name' => 'pool1', 'size' => 5} ], @@ -26,7 +27,7 @@ describe Vmpooler::API do before(:each) do expect(app).to receive(:run!) - app.execute([:api], config, redis, nil) + app.execute([:api], config, redis, nil, nil) app.settings.set :config, auth: false end @@ -64,6 +65,7 @@ describe Vmpooler::API do describe '/dashboard/stats/vmpooler/pool' do let(:config) { { + config: {}, pools: [ {'name' => 'pool1', 'size' => 5}, {'name' => 'pool2', 'size' => 1} @@ -134,6 +136,7 @@ describe Vmpooler::API do describe '/dashboard/stats/vmpooler/running' do let(:config) { { + config: {}, pools: [ {'name' => 'pool-1', 'size' => 5}, {'name' => 'pool-2', 'size' => 1}, diff --git a/spec/unit/env_config.rb b/spec/unit/env_config.rb index 91d79f6..0eba2fb 100644 --- a/spec/unit/env_config.rb +++ b/spec/unit/env_config.rb @@ -30,6 +30,7 @@ describe 'Vmpooler' do ['experimental_features', test_bool, nil], ['purge_unconfigured_folders', test_bool, nil], ['usage_stats', test_bool, nil], + ['request_logger', test_bool, nil], ['extra_config', test_string, nil], ] diff --git a/vmpooler.yaml.example b/vmpooler.yaml.example index d596724..6120c50 100644 --- a/vmpooler.yaml.example +++ b/vmpooler.yaml.example @@ -509,6 +509,12 @@ # Expects a boolean value # (optional; default: false) # +# - request_logger +# Enable API Request logging to the logger +# When enabled all requests to the API are written to the standard logger. +# Expects a boolean value +# (optional; default: false) +# # - max_lifetime_upper_limit # Sets a lifetime upper limit (in hours) for how long the vm lifetime can be set via the API. Lifetime can be set and extended # so this configuration is used to enforce an upper limit to both the initial lifetime request and/or the extended From 416f1f6e7b8631ca5af36318d0b2c884737d59c1 Mon Sep 17 00:00:00 2001 From: John O'Connor Date: Tue, 30 Jun 2020 17:49:42 +0100 Subject: [PATCH 12/12] (POOLER-160) Revise Redis buckets Output from our discussion on histogram buckets for the Clone, Time to ready and Redis connection buckets. --- lib/vmpooler/metrics/promstats.rb | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/lib/vmpooler/metrics/promstats.rb b/lib/vmpooler/metrics/promstats.rb index 3be0726..c7aeb6d 100644 --- a/lib/vmpooler/metrics/promstats.rb +++ b/lib/vmpooler/metrics/promstats.rb @@ -14,10 +14,11 @@ module Vmpooler M_HISTOGRAM = 4 # Customised Bucket set to use for the Pooler clone times set to more appropriate intervals. - POOLER_TIME_BUCKETS = [1.0, 2.5, 5.0, 10.0, 20.0, 50.0, 100.0, 200.0, 500.0, 1000.0, 2000.0].freeze + POOLER_CLONE_TIME_BUCKETS = [10.0, 20.0, 30.0, 40.0, 50.0, 60.0, 120.0, 180.0, 240.0, 300.0, 600.0].freeze + POOLER_READY_TIME_BUCKETS = [30.0, 60.0, 120.0, 180.0, 240.0, 300.0, 500.0, 800.0, 1200.0, 1600.0].freeze # Same for redis connection times - this is the same as the current Prometheus Default. # https://github.com/prometheus/client_ruby/blob/master/lib/prometheus/client/histogram.rb#L14 - REDIS_CONNECT_BUCKETS = [0.005, 0.01, 0.025, 0.05, 0.1, 0.25, 0.5, 1, 2.5, 5, 10].freeze + REDIS_CONNECT_BUCKETS = [1.0, 2.0, 3.0, 5.0, 8.0, 13.0, 18.0, 23.0].freeze @p_metrics = {} @@ -199,7 +200,7 @@ module Vmpooler time_to_ready_state: { mtype: M_HISTOGRAM, torun: %i[manager], - buckets: POOLER_TIME_BUCKETS, + buckets: POOLER_READY_TIME_BUCKETS, docstring: 'Time taken for machine to read ready state for pool', prom_metric_prefix: "#{@metrics_prefix}_time_to_ready_state", param_labels: %i[poolname] @@ -207,7 +208,7 @@ module Vmpooler migrate: { mtype: M_HISTOGRAM, torun: %i[manager], - buckets: POOLER_TIME_BUCKETS, + buckets: POOLER_CLONE_TIME_BUCKETS, docstring: 'vmpooler time taken to migrate machine for pool', prom_metric_prefix: "#{@metrics_prefix}_migrate", param_labels: %i[poolname] @@ -215,7 +216,7 @@ module Vmpooler clone: { mtype: M_HISTOGRAM, torun: %i[manager], - buckets: POOLER_TIME_BUCKETS, + buckets: POOLER_CLONE_TIME_BUCKETS, docstring: 'vmpooler time taken to clone machine', prom_metric_prefix: "#{@metrics_prefix}_clone", param_labels: %i[poolname] @@ -223,7 +224,7 @@ module Vmpooler destroy: { mtype: M_HISTOGRAM, torun: %i[manager], - buckets: POOLER_TIME_BUCKETS, + buckets: POOLER_CLONE_TIME_BUCKETS, docstring: 'vmpooler time taken to destroy machine', prom_metric_prefix: "#{@metrics_prefix}_destroy", param_labels: %i[poolname]