From 0c08c2e2b375d783965e700e7b964ab9c6f9bfb4 Mon Sep 17 00:00:00 2001
From: Emma Foley <emma.l.foley@intel.com>
Date: Wed, 30 Aug 2017 13:33:09 +0000
Subject: [PATCH] Update units for new virt plugin metrics

- Rename libvirt to virt in units.py
- Add new metrics for virt/perf
  - this requires three parameters to map correctly
- Updated unit() to use an optional pltype_instance parameter
- Add unit test to test behaviour with different numbers of parameters

Change-Id: I94437aab309fa6c4d70e52e93d8858b54dcfeedb
Co-Authored-By: Mark O'Neill <mark1.oneill@intel.com>
Closes-Bug: #1714996
---
 collectd_openstack/common/settings.py         | 10 ++++-
 collectd_openstack/common/units.py            | 25 +++++++++++++
 .../tests/common/test_config.py               | 37 +++++++++++++++++++
 3 files changed, 70 insertions(+), 2 deletions(-)

diff --git a/collectd_openstack/common/settings.py b/collectd_openstack/common/settings.py
index 7199413..e78caa4 100644
--- a/collectd_openstack/common/settings.py
+++ b/collectd_openstack/common/settings.py
@@ -102,10 +102,16 @@ class Config(object):
         else:
             LOGGER.info('Configuration OK')
 
-    def unit(self, plugin, pltype):
+    def unit(self, plugin, pltype, pltype_instance=None):
         """Get unit for plugin and type"""
 
-        if pltype:
+        if plugin == "virt" and pltype == "perf":
+            unit = self._units.get(
+                "%s.%s.%s" % (plugin, pltype, pltype_instance))
+            if unit:
+                return unit
+
+        elif pltype:
             unit = self._units.get('%s.%s' % (plugin, pltype))
             if unit:
                 return unit
diff --git a/collectd_openstack/common/units.py b/collectd_openstack/common/units.py
index 5f2b89b..42945e4 100644
--- a/collectd_openstack/common/units.py
+++ b/collectd_openstack/common/units.py
@@ -119,6 +119,8 @@ UNITS = {
 
     'irq': 'Irq/s',
 
+    # DO NOT UPDATE, modify 'virt' plugin instead
+    # These mappings are to support older versions of collectd
     'libvirt.if_octets':        'B/s',
     'libvirt.virt_cpu_total':   'ms',
     'libvirt.disk_octets':      'B/s',
@@ -276,6 +278,29 @@ UNITS = {
     'varnish.total_requests':   'Requests',
     'varnish.total_operations': 'Operations',
 
+    'virt.cpu_affinity':     '',
+    'virt.disk_error':       '',
+    'virt.disk_octets':      'B/s',
+    'virt.disk_ops':         'Ops/s',
+    'virt.disk_time':        'ns',
+    'virt.if_dropped':       'Packets/s',
+    'virt.if_errors':        'Errors/s',
+    'virt.if_octets':        'B/s',
+    'virt.if_packets':       'Packets/s',
+    'virt.job_stats':        '',
+    'virt.memory':           'kb',
+    'virt.percent':          '%',
+    'virt.virt_cpu_total':   'ns',
+    'virt.virt_vcpu':        'ns/cpu',
+
+    'virt.perf.perf_cmt':          'B',
+    'virt.perf.perf_mbmt':         'B',
+    'virt.perf.mbml':              'B',
+    'virt.perf.perf_cpu_cycles':   'Cycles',
+    'virt.perf.instructions':      'Instructions',
+    'virt.perf.cache_references':  'Hits',
+    'virt.perf.perf_cache_misses': 'Misses',
+
     'vmem.vmpage_action':   'Actions',
     'vmem.vmpage_faults':   'Faults/s',
     'vmem.vmpage_io':       'Pages/s',
diff --git a/collectd_openstack/tests/common/test_config.py b/collectd_openstack/tests/common/test_config.py
index 797816f..abf714c 100644
--- a/collectd_openstack/tests/common/test_config.py
+++ b/collectd_openstack/tests/common/test_config.py
@@ -177,6 +177,43 @@ class TestConfig(TestCase):
             mock.call('Configuration parameter %s not set.', "OS_AUTH_URL"),
             mock.call('Collectd plugin will not work properly')])
 
+    def test_unit_libvirt(self):
+        """Test that unit uses three params only when pl=libvirt.
+
+        Set-up: Define a subset of unit mappings
+        Test: get unit mapping for:
+          * libvirt (three params)
+          * some other plugin (two params)
+          * some other plugin (three params)
+        Expected behaviour:
+          * libvirt.perf unit mappings should use three params
+          * other plugins are mapped using two params
+          * using three params to access other plugins fails
+        """
+        # Define a sub-set of unit-mappings
+        self.config._units = {"virt.type.type_instance": "unreachable_unit",
+                              "virt.type": "virt_unit",
+                              "virt.perf.type": "perf_type_unit",
+                              "virt.perf": "unreachable_unit",
+                              "other.type": "other_unit",
+                              "other.type.not_instance": "unreachable_unit",
+                             }
+
+        # Try and get the units
+        self.assertNotEqual("unreachable_unit",
+                            self.config.unit("virt", "type", pltype_instance="type"))
+        self.assertEqual("virt_unit",
+                         self.config.unit("virt", "type"))
+        self.assertNotEqual("unreachable_unit",
+                            self.config.unit("virt", "perf"))
+        self.assertEqual("other_unit",
+                         self.config.unit("other", "type"))
+        self.assertNotEqual("unreachable_unit",
+                            self.config.unit("other", "type",
+                                             pltype_instance="not_instance"))
+        self.assertEqual("perf_type_unit",
+                         self.config.unit("virt", "perf", pltype_instance="type"))
+
     @mock.patch.object(settings, 'LOGGER', autospec=True)
     def test_user_units(self, LOGGER):
         """Test configuration with user defined units"""