From a21d8c56425ddaf7a2f8ceb1b26887e246caa8ad Mon Sep 17 00:00:00 2001 From: John O'Connor Date: Thu, 25 Jun 2020 19:44:26 +0100 Subject: [PATCH] (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 {