From 2de4a6db0b15d775f8166be3a034a7a18911b1ae Mon Sep 17 00:00:00 2001
From: "kirby@puppetlabs.com" <mk.kirby@gmail.com>
Date: Wed, 19 Dec 2018 12:45:50 -0800
Subject: [PATCH] Ensure nodes are consistent for usage stats

This commit updates vm usage stats collection to replace all instances of '.' characters within node strings. Without this change the node string containing a '.' character causes the metric to be interpreted as containing another node.
---
 CHANGELOG.md                   |  3 +++
 lib/vmpooler/pool_manager.rb   |  4 +++-
 spec/unit/pool_manager_spec.rb | 43 +++++++++++++++++++++++++++++++---
 3 files changed, 46 insertions(+), 4 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 12646ef..4d0d0fc 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -12,6 +12,9 @@ If you're looking for changes from before this, refer to the project's
 git logs & PR history.
 # [Unreleased](https://github.com/puppetlabs/vmpooler/compare/0.2.3...master)
 
+### Fixed
+- Ensure that metric nodes for vm usage stats are consistent
+
 # [0.2.3](https://github.com/puppetlabs/vmpooler/compare/0.2.2...0.2.3)
 
 ### Fixed
diff --git a/lib/vmpooler/pool_manager.rb b/lib/vmpooler/pool_manager.rb
index 95f8bd8..011e0a6 100644
--- a/lib/vmpooler/pool_manager.rb
+++ b/lib/vmpooler/pool_manager.rb
@@ -333,12 +333,13 @@ module Vmpooler
       poolname = $redis.hget("vmpooler__vm__#{vm}", "template")
 
       unless jenkins_build_url
+        user = user.gsub('.', '_')
         $metrics.increment("usage.#{user}.#{poolname}")
         return
       end
 
       url_parts = jenkins_build_url.split('/')[2..-1]
-      instance = url_parts[0].gsub('.', '_')
+      instance = url_parts[0]
       value_stream_parts = url_parts[2].split('_')
       value_stream = value_stream_parts.shift
       branch = value_stream_parts.pop
@@ -360,6 +361,7 @@ module Vmpooler
       ]
 
       metric_parts = metric_parts.reject { |s| s.nil? }
+      metric_parts = metric_parts.map { |s| s.gsub('.', '_') }
 
       $metrics.increment(metric_parts.join('.'))
     rescue => err
diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
index 995798b..7cef5a4 100644
--- a/spec/unit/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -867,22 +867,53 @@ EOT
             subject.get_vm_usage_labels(vm)
           end
 
+          context 'with a user with period in name' do
+            let(:user) { 'test.user'.gsub('.', '_') }
+            let(:metric_string) { "usage.#{user}.#{template}" }
+            let(:metric_nodes) { metric_string.split('.') }
+
+            before(:each) do
+              create_running_vm(template, vm)
+            end
+
+            it 'should emit a metric with the character replaced' do
+              expect(metrics).to receive(:increment).with(metric_string)
+
+              subject.get_vm_usage_labels(vm)
+            end
+
+            it 'should include three nodes' do
+              expect(metric_nodes.count).to eq(3)
+            end
+
+          end
+
           context 'with a jenkins_build_url label' do
             let(:jenkins_build_url) { 'https://jenkins.example.com/job/enterprise_pe-acceptance-tests_integration-system_pe_full-agent-upgrade_weekend_2018.1.x/LAYOUT=centos6-64mcd-ubuntu1404-32f-64f,LEGACY_AGENT_VERSION=NONE,PLATFORM=NOTUSED,SCM_BRANCH=2018.1.x,UPGRADE_FROM=2018.1.0,UPGRADE_TO_VERSION=NONE,label=beaker/222/' }
             let(:url_parts) { jenkins_build_url.split('/')[2..-1] }
-            let(:instance) { url_parts[0].gsub('.', '_') }
+            let(:instance) { url_parts[0] }
             let(:value_stream_parts) { url_parts[2].split('_') }
             let(:value_stream) { value_stream_parts.shift }
             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('.') }
 
             before(:each) do
               create_tag(vm, 'jenkins_build_url', jenkins_build_url)
             end
 
             it 'should emit a metric with information from the URL' do
-              expect(metrics).to receive(:increment).with("usage.#{user}.#{instance}.#{value_stream}.#{branch}.#{project}.#{job_name}.#{template}")
+              expect(metrics).to receive(:increment).with(metric_string)
 
               subject.get_vm_usage_labels(vm)
             end
@@ -899,17 +930,23 @@ EOT
             let(:job_name) { value_stream_parts.join('_') }
             let(:build_metadata) { url_parts[3] }
             let(:build_component) { subject.component_to_test('RMM_COMPONENT_TO_TEST_NAME', build_metadata) }
+            let(:expected_string) { "usage.#{user}.#{instance}.#{value_stream}.#{branch}.#{project}.#{job_name}.#{build_component}.#{template}" }
+            let(:metric_nodes) { expected_string.split('.') }
 
             before(:each) do
               create_tag(vm, 'jenkins_build_url', jenkins_build_url)
             end
 
             it 'should emit a metric with information from the URL' do
-              expect(metrics).to receive(:increment).with("usage.#{user}.#{instance}.#{value_stream}.#{branch}.#{project}.#{job_name}.#{build_component}.#{template}")
+              expect(metrics).to receive(:increment).with(expected_string)
 
               subject.get_vm_usage_labels(vm)
             end
 
+            it 'should contain exactly nine nodes' do
+              expect(metric_nodes.count).to eq(9)
+            end
+
             context 'when there is no matrix job information' do
 
               let(:jenkins_build_url) { 'https://jenkins.example.com/job/platform_puppet-agent-extra_puppet-agent-integration-suite_pr/824/' }