From 72564de4b42821c88d1a3486d1c8d096892a871f Mon Sep 17 00:00:00 2001 From: John O'Connor Date: Fri, 22 May 2020 19:21:15 +0100 Subject: [PATCH] (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)