From 09d6a7bc41e284ff67ece4eadededd369eb709ca Mon Sep 17 00:00:00 2001
From: Shaun Duncan <shaun.duncan@gmail.com>
Date: Wed, 14 Jan 2015 09:45:38 -0500
Subject: [PATCH 1/9] Enhancements to SNMPCollector and many new tests.

Modified the base SNMPCollector to do a lot of what the SNMPRawCollector
does. This means that it can handle both individual GETs and WALKs via
configuration. Added tests for all functionality in this class.
---
 src/collectors/snmp/snmp.py          | 315 +++++++++++++++++++++------
 src/collectors/snmp/test/testsnmp.py | 273 ++++++++++++++++++++++-
 2 files changed, 518 insertions(+), 70 deletions(-)

diff --git a/src/collectors/snmp/snmp.py b/src/collectors/snmp/snmp.py
index 1ef876226..98fad2607 100644
--- a/src/collectors/snmp/snmp.py
+++ b/src/collectors/snmp/snmp.py
@@ -1,15 +1,61 @@
 # coding=utf-8
 
 """
-SNMPCollector is a special collector for collecting data from using SNMP
+The SNMPCollector is designed for collecting data from SNMP-enables devices,
+using a set of specified OIDs
+
+#### Configuration
+
+Below is an example configuration for the SNMPCollector. The collector
+can collect data any number of devices by adding configuration sections
+under the *devices* header. By default the collector will collect every 60
+seconds. This might be a bit excessive and put unnecessary load on the
+devices being polled. You may wish to change this to every 300 seconds. However
+you need modify your graphite data retentions to handle this properly.
+
+```
+    [[SNMPCollector]]
+    enabled = True
+    interval = 60
+
+    [[[devices]]]
+
+    # Start the device configuration
+    # Note: this name will be used in the metric path.
+    [[[[my-identification-for-this-host]]]]
+    host = localhost
+    port = 161
+    community = public
+
+    # Start the OID list for this device
+    # Note: the value part will be used in the metric path.
+    [[[[[oids]]]]]
+    1.3.6.1.4.1.2021.10.1.3.1 = cpu.load.1min
+    1.3.6.1.4.1.2021.10.1.3.2 = cpu.load.5min
+    1.3.6.1.4.1.2021.10.1.3.3 = cpu.load.15min
+
+    # If you want another host, you can. But you probably won't need it.
+    [[[[another-identification]]]]
+    host = router1.example.com
+    port = 161
+    community = public
+    [[[[[oids]]]]]
+    oid = metric.path
+    oid = metric.path
+```
+
+Note: If you modify the SNMPRawCollector configuration, you will need to
+restart diamond.
 
 #### Dependencies
 
- * pysnmp
+ * pysmnp (which depends on pyasn1 0.1.7 and pycrypto)
 
 """
 
+import re
 import socket
+import time
 
 import warnings
 
@@ -20,17 +66,19 @@
 warnings.filterwarnings("ignore", category=DeprecationWarning)
 
 cmdgen = None
+IntegerType = None
 
 try:
+    # Note only here for safety. The collector will log if this fails to load
     import pysnmp.entity.rfc3413.oneliner.cmdgen as cmdgen
-    import pysnmp.debug
+    from pyasn1.type.univ import Integer as IntegerType
 except ImportError:
-    pysnmp = None
-    cmdgen = None
+    pass
 
 warnings.showwarning = old_showwarning
 
 import diamond.collector
+from diamond.metric import Metric
 
 
 class SNMPCollector(diamond.collector.Collector):
@@ -46,87 +94,220 @@ def get_default_config_help(self):
     def get_default_config(self):
         # Initialize default config
         default_config = super(SNMPCollector, self).get_default_config()
-        default_config['path_suffix'] = ''
-        default_config['path_prefix'] = 'systems'
-        default_config['timeout'] = 5
-        default_config['retries'] = 3
-        # Return default config
+        default_config.update({
+            'path_suffix': '',
+            'path_prefix': 'devices',
+            'timeout': 5,
+            'retries': 3,
+            'devices': {},
+        })
         return default_config
 
-    def _convert_to_oid(self, s):
-        d = s.split(".")
-        return tuple([int(x) for x in d])
-
-    def _convert_from_oid(self, oid):
-        return ".".join([str(x) for x in oid])
+    def _to_oid_tuple(self, s):
+        """
+        Convert an OID string into a tuple of integers
 
-    def get(self, oid, host, port, community):
+        :param s: an OID string
+        :returns: A tuple of integers
         """
-        Perform SNMP get for a given OID
+        if not isinstance(s, (tuple, list)):
+            s = s.split('.')
+        return tuple(map(int, s))
+
+    def _from_oid_tuple(self, oid):
         """
-        # Initialize return value
-        ret = {}
+        Convert a tuple of integers into an OID string
 
-        # Convert OID to tuple if necessary
-        if not isinstance(oid, tuple):
-            oid = self._convert_to_oid(oid)
+        :param oid: a tuple of integers
+        :returns: a period delimited string of integers
+        """
+        if not isinstance(oid, (tuple, list)):
+            return oid
+        return '.'.join(map(str, oid))
 
-        # Convert Host to IP if necessary
-        host = socket.gethostbyname(host)
+    def _precision(self, value):
+        """
+        Return the precision of the number
+        """
+        value = str(value)
+        decimal = value.rfind('.')
+        if decimal == -1:
+            return 0
+        return len(value) - decimal - 1
 
-        # Assemble SNMP Auth Data
-        snmpAuthData = cmdgen.CommunityData('agent', community)
+    def create_metric(self, oid, basename, name, value):
+        """
+        Creates a :class:`Metric` from a name and value. Care is taken to ensure
+        that this data can be represented in graphite as a GAUGE type, so non-numeric datatypes
+        are excluded. This will also replace a root OID with a basename. For example,
+        if given a root oid '1.2.3.4.5', basename 'foo.bar', and name '1.2.3.4.5.6.7.8.9',
+        the result would be 'foo.bar.6.7.8.9'
+
+        :param oid: Root OID string
+        :param basename: The replacement string of the OID root string
+        :param name: An instance of pysnmp.proto.rfc1902.ObjectName
+        :param value: Some form of subclass instance of pyasn1.type.base.AbstractSimpleAsn1Item
+        :returns: None or Metric
+        """
+        # If metric value is 'empty'
+        if not value:
+            self.log.debug("Metric '{0}' has no value".format(name))
+            return None
+
+        # If metric value is a non-integer type
+        if not isinstance(value, IntegerType):
+            self.log.debug("Metric '{0}' is not an Integer type".format(name))
+            return None
+
+        # Convert to a simple readable format
+        name = name.prettyPrint()
+        name = re.sub(r'^{0}'.format(oid), basename, name)
+        value = value.prettyPrint()
+
+        return Metric(path=name,
+                      value=value,
+                      timestamp=time.time(),
+                      precision=self._precision(value),
+                      metric_type='GAUGE')
+
+    def snmp_get(self, oid, auth, transport):
+        """
+        Perform SNMP get for a given OID
 
-        # Assemble SNMP Transport Data
-        snmpTransportData = cmdgen.UdpTransportTarget(
-            (host, port),
-            int(self.config['timeout']),
-            int(self.config['retries']))
+        :param oid: An OID string or tuple to query
+        :param auth: a CommunityData instance for authentication
+        :param transport: an SNMP transport target (UdpTransportTarget)
+        :returns: list of SNMP (name, value) tuples
+        """
+        # Run the SNMP GET query
+        result = self.cmdgen.getCmd(auth, transport, self._to_oid_tuple(oid))
+
+        try:
+            return result[3]
+        except (ValueError, IndexError):
+            self.log.debug(
+                "SNMP GET '{0}' on host '{1}' returned no data".format(
+                    oid, transport.transportAddr[0]
+                )
+            )
+        return []
+
+    def snmp_walk(self, oid, auth, transport):
+        """
+        Perform an SNMP walk on a given OID
 
-        # Assemble SNMP Next Command
-        result = self.snmpCmdGen.getCmd(snmpAuthData, snmpTransportData, oid)
-        varBind = result[3]
+        :param oid: An OID string or tuple to query
+        :param auth: a CommunityData instance for authentication
+        :param transport: an SNMP transport target (UdpTransportTarget)
+        :returns: list of SNMP (name, value) tuples
+        """
+        # Run the SNMP WALK query
+        result = self.cmdgen.nextCmd(auth, transport, self._to_oid_tuple(oid))
+
+        try:
+            return result[3]
+        except IndexError:
+            self.log.debug(
+                "SNMP WALK '{0}' on host '{1}' returned no data".format(
+                    oid, transport.transportAddr[0]
+                )
+            )
+        return []
+
+    def create_transport(self, host, port):
+        """
+        Create a pysnmp UDP transport target with the given host and port
 
-        # TODO: Error check
+        :param host: hostname string
+        :param port: integer port number
+        :return: pysnmp UdpTransportTarget
+        """
+        # Get the IP addr of the host
+        host = socket.gethostbyname(host)
 
-        for o, v in varBind:
-            ret[o.prettyPrint()] = v.prettyPrint()
+        timeout = int(self.config['timeout'])
+        retries = int(self.config['retries'])
 
-        return ret
+        return cmdgen.UdpTransportTarget((host, port), timeout, retries)
 
-    def walk(self, oid, host, port, community):
-        """
-        Perform an SNMP walk on a given OID
+    def create_auth(self, community):
         """
-        # Initialize return value
-        ret = {}
+        Create a pysnmp CommunityData object
 
-        # Convert OID to tuple if necessary
-        if not isinstance(oid, tuple):
-            oid = self._convert_to_oid(oid)
-
-        # Convert Host to IP if necessary
-        host = socket.gethostbyname(host)
+        :param community: community auth string
+        :returns: pysnmp CommunityData
+        """
+        return cmdgen.CommunityData('agent', community)
 
-        # Assemble SNMP Auth Data
-        snmpAuthData = cmdgen.CommunityData('agent', community)
+    def collect_snmp(self, device, auth, transport, oids):
+        """
+        Collect SNMP interface data from a device. Devices should be configured with
+        an [oids] section that includes name-value pairs for data to gather. For example:
 
-        # Assemble SNMP Transport Data
-        snmpTransportData = cmdgen.UdpTransportTarget(
-            (host, port),
-            int(self.config['timeout']),
-            int(self.config['retries']))
+            [oids]
+            1.3.6.1.4.1.1111 = my.metric.name
 
-        # Assemble SNMP Next Command
-        resultTable = self.snmpCmdGen.nextCmd(snmpAuthData,
-                                              snmpTransportData,
-                                              oid)
-        varBindTable = resultTable[3]
+        There are special circumstances where one could obtain large swaths of data using
+        an SNMP walk. In this situation, metrics aren't named, but namespaced by OID value
+        in graphite:
 
-        # TODO: Error Check
+            [oids]
+            1.2.6.1.4.1.1111.* = my.metric.name
 
-        for varBindTableRow in varBindTable:
-            for o, v in varBindTableRow:
-                ret[o.prettyPrint()] = v.prettyPrint()
+        Note, this will replace anything preceding .* with the name on the right hand side.
+        Anything obtained from this walk will be namespaced according to the remaining portion
+        of the OID. For example:
 
-        return ret
+            my.metric.name.1
+            my.metric.name.2.1
+            my.metric.name.2.2
+            my.metric.name.2.3
+        """
+        self.log.debug("Collecting SNMP data from device '{0}'".format(device))
+        path_prefix = '.'.join([
+            self.config['path_prefix'],
+            device,
+            self.config['path_suffix']
+        ]).rstrip('.')
+
+        for oid, basename in oids.items():
+            if oid.endswith('.*'):  # Walk
+                oid = oid[:-2]
+                fn = self.snmp_walk
+            else:
+                fn = self.snmp_get
+
+            for metric_name, metric_value in fn(oid, auth, transport):
+                metric = self.create_metric(oid, basename, metric_name, metric_value)
+
+                self.log.debug(
+                    "'{0}' on device '{1}' - value=[{2}]".format(
+                        metric.path, device, metric.value
+                    )
+                )
+
+                metric.path = '.'.join([path_prefix, metric.path])
+                self.publish_metric(metric)
+
+    def collect(self):
+        """
+        Collect stats via SNMP
+        """
+        if not cmdgen:
+            self.log.error('pysnmp.entity.rfc3413.oneliner.cmdgen failed to load')
+            return
+
+        # If there are no devices, nothing can be collected
+        if 'devices' not in self.config:
+            self.log.error('No devices configured for this collector')
+            return
+
+        # Initialize SNMP Command Generator
+        self.cmdgen = cmdgen.CommandGenerator()
+
+        # Collect SNMP data from each device
+        for device, config in self.config['devices'].items():
+            # Create auth and transport for this device
+            auth = self.create_auth(config.get('community', 'public'))
+            transport = self.create_transport(config['host'], int(config.get('port', 161)))
+            self.collect_snmp(device, auth, transport, config.get('oids', {}))
diff --git a/src/collectors/snmp/test/testsnmp.py b/src/collectors/snmp/test/testsnmp.py
index 0ae0e83d9..270f85a00 100644
--- a/src/collectors/snmp/test/testsnmp.py
+++ b/src/collectors/snmp/test/testsnmp.py
@@ -1,22 +1,289 @@
 #!/usr/bin/python
 # coding=utf-8
 ################################################################################
-
-from test import CollectorTestCase
-from test import get_collector_config
+from mock import Mock, patch
 
 from snmp import SNMPCollector
+from test import CollectorTestCase, get_collector_config
 
 
 class TestSNMPCollector(CollectorTestCase):
+
     def setUp(self, allowed_names=None):
         if not allowed_names:
             allowed_names = []
+
         config = get_collector_config('SNMPCollector', {
             'allowed_names': allowed_names,
             'interval': 1
         })
+
         self.collector = SNMPCollector(config, None)
 
     def test_import(self):
         self.assertTrue(SNMPCollector)
+
+    def test_default_config(self):
+        config = self.collector.get_default_config()
+        self.assertEqual(config['path_prefix'], 'devices')
+        self.assertEqual(config['path_suffix'], '')
+        self.assertEqual(config['timeout'], 5)
+        self.assertEqual(config['retries'], 3)
+        self.assertEqual(config['devices'], {})
+
+    def test_to_oid_tuple(self):
+        self.assertEqual((1, 2, 3), self.collector._to_oid_tuple('1.2.3'))
+
+    def test_to_oid_tuple_handles_tuple(self):
+        tup = (1, 2, 3)
+        self.assertEqual(tup, self.collector._to_oid_tuple(tup))
+
+    def test_from_oid_tuple(self):
+        self.assertEqual('1.2.3', self.collector._from_oid_tuple((1, 2, 3)))
+
+    def test_from_oid_tuple_handles_string(self):
+        string = '1.2.3'
+        self.assertEqual(string, self.collector._from_oid_tuple(string))
+
+    def test_create_metric_empty_value(self):
+        self.assertIsNone(self.collector.create_metric('x', 'y', 'name', None))
+
+    @patch('snmp.IntegerType', Mock)
+    def test_create_metric_path_replacement(self):
+        name = Mock(prettyPrint=lambda: '1.2.3.1.2.3')
+        value = Mock(prettyPrint=lambda: '42')
+
+        metric = self.collector.create_metric('1.2', 'foo', name, value)
+
+        self.assertEqual(metric.path, 'foo.3.1.2.3')
+
+    @patch('snmp.IntegerType', int)
+    def test_create_metric_non_integer_type(self):
+        self.assertIsNone(self.collector.create_metric('x', 'y', 'name', 'value'))
+
+    @patch('snmp.IntegerType', Mock)
+    @patch('snmp.time')
+    def test_create_metric(self, time):
+        time.time.return_value = 100
+        name = Mock(prettyPrint=lambda: '1.2.3')
+        value = Mock(prettyPrint=lambda: '42')
+
+        metric = self.collector.create_metric('1.2', 'foo', name, value)
+
+        self.assertEqual(metric.path, 'foo.3')
+        self.assertEqual(metric.value, 42.0)
+        self.assertEqual(metric.precision, 0)
+        self.assertEqual(metric.timestamp, 100)
+        self.assertEqual(metric.metric_type, 'GAUGE')
+
+    def test_snmp_get_no_metrics(self):
+        retvals = [
+            [],  # IndexError
+            (None, None, None, []),  # IndexError
+            (None, None, None, ['foo']),  # ValueError
+        ]
+
+        for retval in retvals:
+            self.collector.cmdgen = Mock()
+            self.collector.cmdgen.getCmd.return_value = []
+
+            auth = Mock()
+            transport = Mock(transportAddr=('localhost', 161))
+            metrics = self.collector.snmp_get('1.2', auth, transport)
+
+            self.assertEqual([], metrics)
+
+    def test_snmp_get(self):
+        name = Mock()
+        value = Mock()
+
+        self.collector.cmdgen = Mock()
+        self.collector.cmdgen.getCmd.return_value = (None, None, None, [(name, value)])
+
+        auth = Mock()
+        transport = Mock()
+
+        with patch.object(self.collector, 'create_metric'):
+            self.collector.create_metric.return_value = 'bar'
+            metrics = self.collector.snmp_get('1.2', auth, transport)
+            self.assertEqual(metrics, [(name, value)])
+
+    def test_snmp_walk_no_metrics(self):
+        for retval in ([], (None, None, None, [])):
+            self.collector.cmdgen = Mock()
+            self.collector.cmdgen.nextCmd.return_value = retval
+
+            auth = Mock()
+            transport = Mock(transportAddr=('localhost', 161))
+            metrics = self.collector.snmp_walk('1.2', auth, transport)
+
+            self.assertEqual([], list(metrics))
+
+    def test_snmp_walk(self):
+        metrics = (None, None, None, [
+            (Mock(prettyPrint=lambda: '1.2.1'), Mock(prettyPrint=lambda: '41')),
+            (Mock(prettyPrint=lambda: '1.2.2'), Mock(prettyPrint=lambda: '42')),
+            (Mock(prettyPrint=lambda: '1.2.3'), Mock(prettyPrint=lambda: '43')),
+        ])
+
+        self.collector.cmdgen = Mock()
+        self.collector.cmdgen.nextCmd.return_value = metrics
+
+        auth = Mock()
+        transport = Mock(transportAddr=('localhost', 161))
+
+        with patch.object(self.collector, 'create_metric'):
+            self.collector.create_metric.side_effect = metrics[3]
+            ret_metrics = list(self.collector.snmp_walk('1.2', auth, transport))
+            self.assertEqual(metrics[3], ret_metrics)
+
+    @patch('snmp.IntegerType', Mock)
+    def test_collect_calls_snmp_get(self):
+        device = 'mydevice'
+        auth = Mock()
+        transport = Mock()
+        oids = {
+            '1.2.3': 'foo.bar',
+        }
+
+        metrics = [
+            (Mock(prettyPrint=lambda: '1.2.3'), Mock(prettyPrint=lambda: '42')),
+            (Mock(prettyPrint=lambda: '1.2.3.4'), Mock(prettyPrint=lambda: '43')),
+        ]
+
+        with patch.multiple(self.collector, snmp_get=Mock(), snmp_walk=Mock(), publish_metric=Mock()):
+            self.collector.snmp_get.return_value = metrics
+
+            self.collector.collect_snmp(device, auth, transport, oids)
+
+            # Are we calling the correct method
+            self.assertFalse(self.collector.snmp_walk.called)
+            self.assertTrue(self.collector.snmp_get.called)
+
+            calls = self.collector.publish_metric.call_args_list
+
+            # Do we publish metrics?
+            self.assertEqual(len(calls), 2)
+
+            # Were metrics properly namespaced
+            self.assertEqual(calls[0][0][0].path, 'devices.mydevice.foo.bar')
+            self.assertEqual(calls[1][0][0].path, 'devices.mydevice.foo.bar.4')
+
+    @patch('snmp.IntegerType', Mock)
+    def test_collect_calls_snmp_walk(self):
+        device = 'mydevice'
+        auth = Mock()
+        transport = Mock()
+        oids = {
+            '1.2.3.*': 'foo.bar',
+        }
+
+        metrics = [
+            (Mock(prettyPrint=lambda: '1.2.3'), Mock(prettyPrint=lambda: '42')),
+            (Mock(prettyPrint=lambda: '1.2.3.4'), Mock(prettyPrint=lambda: '43')),
+        ]
+
+        with patch.multiple(self.collector, snmp_get=Mock(), snmp_walk=Mock(), publish_metric=Mock()):
+            self.collector.snmp_walk.return_value = metrics
+
+            self.collector.collect_snmp(device, auth, transport, oids)
+
+            # Are we calling the correct method
+            self.assertTrue(self.collector.snmp_walk.called)
+            self.assertFalse(self.collector.snmp_get.called)
+
+            calls = self.collector.publish_metric.call_args_list
+
+            # Do we publish metrics?
+            self.assertEqual(len(calls), 2)
+
+            # Were metrics properly namespaced
+            self.assertEqual(calls[0][0][0].path, 'devices.mydevice.foo.bar')
+            self.assertEqual(calls[1][0][0].path, 'devices.mydevice.foo.bar.4')
+
+    @patch('snmp.cmdgen', None)
+    def test_collect_nothing_with_pysnmp_error(self):
+        with patch.object(self.collector, 'log'):
+            self.collector.collect()
+            self.collector.log.error.assert_called_with(
+                'pysnmp.entity.rfc3413.oneliner.cmdgen failed to load'
+            )
+
+    @patch('snmp.cmdgen')
+    def test_collect_no_devices(self, cmdgen):
+        with patch.multiple(self.collector, log=Mock(), config={}):
+            self.collector.collect()
+            self.collector.log.error.assert_called_with(
+                'No devices configured for this collector'
+            )
+
+    @patch('snmp.cmdgen')
+    def test_collect(self, cmdgen):
+        oids = {
+            '1.2.3': 'foo.bar',
+        }
+
+        config = {
+            'devices': {
+                'mydevice': {
+                    'host': 'localhost',
+                    'port': 161,
+                    'community': 'public',
+                    'oids': oids,
+                }
+            }
+        }
+
+        # Setup mocks
+        auth = Mock()
+        auth.return_value = auth
+
+        transport = Mock()
+        transport.return_value = transport
+
+        collect_snmp = Mock()
+
+        with patch.multiple(self.collector,
+                            config=config,
+                            create_auth=auth,
+                            create_transport=transport,
+                            collect_snmp=collect_snmp):
+            self.collector.collect()
+            auth.assert_called_with('public')
+            transport.assert_called_with('localhost', 161)
+            collect_snmp.assert_called_with('mydevice', auth, transport, oids)
+
+    @patch('snmp.cmdgen')
+    def test_collect_uses_defaults(self, cmdgen):
+        oids = {
+            '1.2.3': 'foo.bar',
+        }
+
+        # We should allow assuming default values for community and port
+        config = {
+            'devices': {
+                'mydevice': {
+                    'host': 'localhost',
+                    'oids': oids,
+                }
+            }
+        }
+
+        # Setup mocks
+        auth = Mock()
+        auth.return_value = auth
+
+        transport = Mock()
+        transport.return_value = transport
+
+        collect_snmp = Mock()
+
+        with patch.multiple(self.collector,
+                            config=config,
+                            create_auth=auth,
+                            create_transport=transport,
+                            collect_snmp=collect_snmp):
+            self.collector.collect()
+            auth.assert_called_with('public')
+            transport.assert_called_with('localhost', 161)
+            collect_snmp.assert_called_with('mydevice', auth, transport, oids)

From 306c9ef902db4dddf1136194b78d96bd8b3821ad Mon Sep 17 00:00:00 2001
From: Shaun Duncan <shaun.duncan@gmail.com>
Date: Wed, 14 Jan 2015 09:47:32 -0500
Subject: [PATCH 2/9] Mirror SNMPCollector as SNMPRawCollector for backwards
 compatibility

---
 src/collectors/snmpraw/snmpraw.py          | 178 +--------------------
 src/collectors/snmpraw/test/testsnmpraw.py |  50 ------
 2 files changed, 6 insertions(+), 222 deletions(-)
 delete mode 100644 src/collectors/snmpraw/test/testsnmpraw.py

diff --git a/src/collectors/snmpraw/snmpraw.py b/src/collectors/snmpraw/snmpraw.py
index 0b48bb810..af083f7e7 100644
--- a/src/collectors/snmpraw/snmpraw.py
+++ b/src/collectors/snmpraw/snmpraw.py
@@ -1,51 +1,8 @@
 # coding=utf-8
 
 """
-The SNMPRawCollector is designed for collecting data from SNMP-enables devices,
-using a set of specified OIDs
-
-#### Configuration
-
-Below is an example configuration for the SNMPRawCollector. The collector
-can collect data any number of devices by adding configuration sections
-under the *devices* header. By default the collector will collect every 60
-seconds. This might be a bit excessive and put unnecessary load on the
-devices being polled. You may wish to change this to every 300 seconds. However
-you need modify your graphite data retentions to handle this properly.
-
-```
-    # Options for SNMPRawCollector
-    enabled = True
-    interval = 60
-
-    [devices]
-
-    # Start the device configuration
-    # Note: this name will be used in the metric path.
-    [[my-identification-for-this-host]]
-    host = localhost
-    port = 161
-    community = public
-
-    # Start the OID list for this device
-    # Note: the value part will be used in the metric path.
-    [[[oids]]]
-    1.3.6.1.4.1.2021.10.1.3.1 = cpu.load.1min
-    1.3.6.1.4.1.2021.10.1.3.2 = cpu.load.5min
-    1.3.6.1.4.1.2021.10.1.3.3 = cpu.load.15min
-
-    # If you want another host, you can. But you probably won't need it.
-    [[another-identification]]
-    host = router1.example.com
-    port = 161
-    community = public
-    [[[oids]]]
-    oid = metric.path
-    oid = metric.path
-```
-
-Note: If you modify the SNMPRawCollector configuration, you will need to
-restart diamond.
+The SNMPRawCollector is a deprecated collector. It's functionality has been moved to the
+top level SNMPCollector
 
 #### Dependencies
 
@@ -55,133 +12,10 @@
 
 import os
 import sys
-import time
-
-sys.path.insert(0, os.path.join(os.path.dirname(os.path.dirname(__file__)),
-                                'snmp'))
-from snmp import SNMPCollector as parent_SNMPCollector
-from diamond.metric import Metric
-
-
-class SNMPRawCollector(parent_SNMPCollector):
-
-    def process_config(self):
-        super(SNMPRawCollector, self).process_config()
-        # list to save non-existing oid's per device, to avoid repetition of
-        # errors in logging. Signal HUP to diamond/collector to flush this
-        self.skip_list = []
-
-    def get_default_config(self):
-        """
-        Override SNMPCollector.get_default_config method to provide
-        default_config for the SNMPInterfaceCollector
-        """
-        default_config = super(SNMPRawCollector,
-                               self).get_default_config()
-        default_config.update({
-            'oids': {},
-            'path_prefix': 'servers',
-            'path_suffix': 'snmp',
-        })
-        return default_config
-
-    def _precision(self, value):
-        """
-        Return the precision of the number
-        """
-        value = str(value)
-        decimal = value.rfind('.')
-        if decimal == -1:
-            return 0
-        return len(value) - decimal - 1
-
-    def _skip(self, device, oid, reason=None):
-        self.skip_list.append((device, oid))
-        if reason is not None:
-            self.log.warn('Muted \'{0}\' on \'{1}\', because: {2}'.format(
-                oid, device, reason))
-
-    def _get_value_walk(self, device, oid, host, port, community):
-        data = self.walk(oid, host, port, community)
-
-        if data is None:
-            self._skip(device, oid, 'device down (#2)')
-            return
-
-        self.log.debug('Data received from WALK \'{0}\': [{1}]'.format(
-            device, data))
-
-        if len(data) != 1:
-            self._skip(
-                device,
-                oid,
-                'unexpected response, data has {0} entries'.format(
-                    len(data)))
-            return
-
-        # because we only allow 1-key dicts, we can pick with absolute index
-        value = data.items()[0][1]
-        return value
-
-    def _get_value(self, device, oid, host, port, community):
-        data = self.get(oid, host, port, community)
-
-        if data is None:
-            self._skip(device, oid, 'device down (#1)')
-            return
-
-        self.log.debug('Data received from GET \'{0}\': [{1}]'.format(
-            device, data))
-
-        if len(data) == 0:
-            self._skip(device, oid, 'empty response, device down?')
-            return
-
-        if oid not in data:
-            # oid is not even in hierarchy, happens when using 9.9.9.9
-            # but not when using 1.9.9.9
-            self._skip(device, oid, 'no object at OID (#1)')
-            return
-
-        value = data[oid]
-        if value == 'No Such Object currently exists at this OID':
-            self._skip(device, oid, 'no object at OID (#2)')
-            return
-
-        if value == 'No Such Instance currently exists at this OID':
-            return self._get_value_walk(device, oid, host, port, community)
-
-        return value
-
-    def collect_snmp(self, device, host, port, community):
-        """
-        Collect SNMP interface data from device
-        """
-        self.log.debug(
-            'Collecting raw SNMP statistics from device \'{0}\''.format(device))
-
-        dev_config = self.config['devices'][device]
-        if 'oids' in dev_config:
-            for oid, metricName in dev_config['oids'].items():
-
-                if (device, oid) in self.skip_list:
-                    self.log.debug(
-                        'Skipping OID \'{0}\' ({1}) on device \'{2}\''.format(
-                            oid, metricName, device))
-                    continue
+import warnings
 
-                timestamp = time.time()
-                value = self._get_value(device, oid, host, port, community)
-                if value is None:
-                    continue
+sys.path.insert(0, os.path.join(os.path.dirname(os.path.dirname(__file__)), 'snmp'))
 
-                self.log.debug(
-                    '\'{0}\' ({1}) on device \'{2}\' - value=[{3}]'.format(
-                        oid, metricName, device, value))
+from snmp import SNMPCollector as SNMPRawCollector  # NOQA
 
-                path = '.'.join([self.config['path_prefix'], device,
-                                 self.config['path_suffix'], metricName])
-                metric = Metric(path=path, value=value, timestamp=timestamp,
-                                precision=self._precision(value),
-                                metric_type='GAUGE')
-                self.publish_metric(metric)
+warnings.warn('The SNMPRawCollector is deprecated. Use SNMPCollector instead', DeprecationWarning)
diff --git a/src/collectors/snmpraw/test/testsnmpraw.py b/src/collectors/snmpraw/test/testsnmpraw.py
deleted file mode 100644
index 1231dce38..000000000
--- a/src/collectors/snmpraw/test/testsnmpraw.py
+++ /dev/null
@@ -1,50 +0,0 @@
-#!/usr/bin/python
-# coding=utf-8
-###############################################################################
-import time
-from test import CollectorTestCase
-from test import get_collector_config
-from test import unittest
-from mock import Mock
-from mock import patch
-
-from snmpraw import SNMPRawCollector
-from diamond.collector import Collector
-
-###############################################################################
-
-
-class TestSNMPRawCollector(CollectorTestCase):
-    def setUp(self):
-        config = get_collector_config('SNMPRawCollector', {
-        })
-        self.collector = SNMPRawCollector(config, None)
-
-    def test_import(self):
-        self.assertTrue(SNMPRawCollector)
-
-    @patch.object(Collector, 'publish_metric')
-    @patch.object(time, 'time', Mock(return_value=1000))
-    @patch.object(SNMPRawCollector, '_get_value', Mock(return_value=5))
-    def test_metric(self, collect_mock):
-        test_config = {'devices': {'test': {'oids': {'1.1.1.1': 'test'}}}}
-        self.collector.config.update(test_config)
-
-        path = '.'.join([self.collector.config['path_prefix'], 'test',
-                         self.collector.config['path_suffix'], 'test'])
-
-        self.collector.collect_snmp('test', None, None, None)
-        metric = collect_mock.call_args[0][0]
-
-        self.assertEqual(metric.metric_type, 'GAUGE')
-        self.assertEqual(metric.ttl, None)
-        self.assertEqual(metric.value, self.collector._get_value())
-        self.assertEqual(metric.precision, self.collector._precision(5))
-        self.assertEqual(metric.host, None)
-        self.assertEqual(metric.path, path)
-        self.assertEqual(metric.timestamp, 1000)
-
-
-###############################################################################
-if __name__ == "__main__":
-    unittest.main()

From f007bd370ca77969b483f4638c4b6dd37b5ad542 Mon Sep 17 00:00:00 2001
From: Shaun Duncan <shaun.duncan@gmail.com>
Date: Wed, 14 Jan 2015 12:36:09 -0500
Subject: [PATCH 3/9] Rework method signature for backwards compatibility

---
 src/collectors/snmp/snmp.py          | 32 ++++++++++++++++---
 src/collectors/snmp/test/testsnmp.py | 46 +++++++++++++++-------------
 2 files changed, 51 insertions(+), 27 deletions(-)

diff --git a/src/collectors/snmp/snmp.py b/src/collectors/snmp/snmp.py
index 98fad2607..aa5a5be66 100644
--- a/src/collectors/snmp/snmp.py
+++ b/src/collectors/snmp/snmp.py
@@ -170,6 +170,24 @@ def create_metric(self, oid, basename, name, value):
                       precision=self._precision(value),
                       metric_type='GAUGE')
 
+    def get(self, oid, host, port, community):
+        """
+        Backwards compatible snmp_get
+        """
+        auth = self.create_auth(community)
+        transport = self.create_transport(host, port)
+        rows = self.snmp_get(oid, auth, transport)
+        return dict((k.prettyPrint(), v.prettyPrint) for k, v in rows)
+
+    def walk(self, oid, host, port, community):
+        """
+        Backwards compatible snmp_walk
+        """
+        auth = self.create_auth(community)
+        transport = self.create_transport(host, port)
+        rows = self.snmp_walk(oid, auth, transport)
+        return dict((k.prettyPrint(), v.prettyPrint) for k, v in rows)
+
     def snmp_get(self, oid, auth, transport):
         """
         Perform SNMP get for a given OID
@@ -239,7 +257,7 @@ def create_auth(self, community):
         """
         return cmdgen.CommunityData('agent', community)
 
-    def collect_snmp(self, device, auth, transport, oids):
+    def collect_snmp(self, device, host, port, community):
         """
         Collect SNMP interface data from a device. Devices should be configured with
         an [oids] section that includes name-value pairs for data to gather. For example:
@@ -270,6 +288,10 @@ def collect_snmp(self, device, auth, transport, oids):
             self.config['path_suffix']
         ]).rstrip('.')
 
+        auth = self.create_auth(community)
+        transport = self.create_transport(host, port)
+        oids = self.config['devices'][device]['oids']
+
         for oid, basename in oids.items():
             if oid.endswith('.*'):  # Walk
                 oid = oid[:-2]
@@ -307,7 +329,7 @@ def collect(self):
 
         # Collect SNMP data from each device
         for device, config in self.config['devices'].items():
-            # Create auth and transport for this device
-            auth = self.create_auth(config.get('community', 'public'))
-            transport = self.create_transport(config['host'], int(config.get('port', 161)))
-            self.collect_snmp(device, auth, transport, config.get('oids', {}))
+            host = config['host']
+            port = int(config.get('port', 161))
+            community = config.get('community', 'public')
+            self.collect_snmp(device, host, port, community)
diff --git a/src/collectors/snmp/test/testsnmp.py b/src/collectors/snmp/test/testsnmp.py
index 270f85a00..560d2df32 100644
--- a/src/collectors/snmp/test/testsnmp.py
+++ b/src/collectors/snmp/test/testsnmp.py
@@ -138,14 +138,21 @@ def test_snmp_walk(self):
             self.assertEqual(metrics[3], ret_metrics)
 
     @patch('snmp.IntegerType', Mock)
+    @patch('snmp.cmdgen', Mock())
     def test_collect_calls_snmp_get(self):
         device = 'mydevice'
-        auth = Mock()
-        transport = Mock()
         oids = {
             '1.2.3': 'foo.bar',
         }
 
+        self.collector.config.update({
+            'devices': {
+                'mydevice': {
+                    'oids': oids,
+                },
+            },
+        })
+
         metrics = [
             (Mock(prettyPrint=lambda: '1.2.3'), Mock(prettyPrint=lambda: '42')),
             (Mock(prettyPrint=lambda: '1.2.3.4'), Mock(prettyPrint=lambda: '43')),
@@ -154,7 +161,7 @@ def test_collect_calls_snmp_get(self):
         with patch.multiple(self.collector, snmp_get=Mock(), snmp_walk=Mock(), publish_metric=Mock()):
             self.collector.snmp_get.return_value = metrics
 
-            self.collector.collect_snmp(device, auth, transport, oids)
+            self.collector.collect_snmp(device, 'localhost', 161, 'public')
 
             # Are we calling the correct method
             self.assertFalse(self.collector.snmp_walk.called)
@@ -170,14 +177,21 @@ def test_collect_calls_snmp_get(self):
             self.assertEqual(calls[1][0][0].path, 'devices.mydevice.foo.bar.4')
 
     @patch('snmp.IntegerType', Mock)
+    @patch('snmp.cmdgen', Mock())
     def test_collect_calls_snmp_walk(self):
         device = 'mydevice'
-        auth = Mock()
-        transport = Mock()
         oids = {
             '1.2.3.*': 'foo.bar',
         }
 
+        self.collector.config.update({
+            'devices': {
+                'mydevice': {
+                    'oids': oids,
+                },
+            },
+        })
+
         metrics = [
             (Mock(prettyPrint=lambda: '1.2.3'), Mock(prettyPrint=lambda: '42')),
             (Mock(prettyPrint=lambda: '1.2.3.4'), Mock(prettyPrint=lambda: '43')),
@@ -186,7 +200,7 @@ def test_collect_calls_snmp_walk(self):
         with patch.multiple(self.collector, snmp_get=Mock(), snmp_walk=Mock(), publish_metric=Mock()):
             self.collector.snmp_walk.return_value = metrics
 
-            self.collector.collect_snmp(device, auth, transport, oids)
+            self.collector.collect_snmp(device, 'localhost', 161, 'public')
 
             # Are we calling the correct method
             self.assertTrue(self.collector.snmp_walk.called)
@@ -243,15 +257,9 @@ def test_collect(self, cmdgen):
 
         collect_snmp = Mock()
 
-        with patch.multiple(self.collector,
-                            config=config,
-                            create_auth=auth,
-                            create_transport=transport,
-                            collect_snmp=collect_snmp):
+        with patch.multiple(self.collector, config=config, collect_snmp=collect_snmp):
             self.collector.collect()
-            auth.assert_called_with('public')
-            transport.assert_called_with('localhost', 161)
-            collect_snmp.assert_called_with('mydevice', auth, transport, oids)
+            collect_snmp.assert_called_with('mydevice', 'localhost', 161, 'public')
 
     @patch('snmp.cmdgen')
     def test_collect_uses_defaults(self, cmdgen):
@@ -278,12 +286,6 @@ def test_collect_uses_defaults(self, cmdgen):
 
         collect_snmp = Mock()
 
-        with patch.multiple(self.collector,
-                            config=config,
-                            create_auth=auth,
-                            create_transport=transport,
-                            collect_snmp=collect_snmp):
+        with patch.multiple(self.collector, config=config, collect_snmp=collect_snmp):
             self.collector.collect()
-            auth.assert_called_with('public')
-            transport.assert_called_with('localhost', 161)
-            collect_snmp.assert_called_with('mydevice', auth, transport, oids)
+            collect_snmp.assert_called_with('mydevice', 'localhost', 161, 'public')

From 7bd4ff3cd1d5e7c149c8bd45fbf2824e9ac19167 Mon Sep 17 00:00:00 2001
From: Shaun Duncan <shaun.duncan@gmail.com>
Date: Wed, 14 Jan 2015 16:04:06 -0500
Subject: [PATCH 4/9] Don't hard set metric path. Updates from testing. Attempt
 str -> float conversion for non-int types

---
 src/collectors/snmp/snmp.py          | 69 +++++++++++--------------
 src/collectors/snmp/test/testsnmp.py | 76 ++++++++++++++++------------
 2 files changed, 74 insertions(+), 71 deletions(-)

diff --git a/src/collectors/snmp/snmp.py b/src/collectors/snmp/snmp.py
index aa5a5be66..9a0f85b35 100644
--- a/src/collectors/snmp/snmp.py
+++ b/src/collectors/snmp/snmp.py
@@ -55,8 +55,6 @@
 
 import re
 import socket
-import time
-
 import warnings
 
 # pysnmp packages on debian 6.0 use sha and md5 which are deprecated
@@ -78,7 +76,6 @@
 warnings.showwarning = old_showwarning
 
 import diamond.collector
-from diamond.metric import Metric
 
 
 class SNMPCollector(diamond.collector.Collector):
@@ -95,8 +92,7 @@ def get_default_config(self):
         # Initialize default config
         default_config = super(SNMPCollector, self).get_default_config()
         default_config.update({
-            'path_suffix': '',
-            'path_prefix': 'devices',
+            'path': 'snmp',
             'timeout': 5,
             'retries': 3,
             'devices': {},
@@ -135,40 +131,47 @@ def _precision(self, value):
             return 0
         return len(value) - decimal - 1
 
-    def create_metric(self, oid, basename, name, value):
+    def _publish(self, device, oid, basename, name, value):
         """
-        Creates a :class:`Metric` from a name and value. Care is taken to ensure
-        that this data can be represented in graphite as a GAUGE type, so non-numeric datatypes
-        are excluded. This will also replace a root OID with a basename. For example,
-        if given a root oid '1.2.3.4.5', basename 'foo.bar', and name '1.2.3.4.5.6.7.8.9',
-        the result would be 'foo.bar.6.7.8.9'
+        Publishes a metric as a GAUGE with a given value. Non-integer datatyps are attempted
+        to be converted to a float value, and ignored if they cannot be. This will also replace
+        a root OID with a basename. For example, if given a root oid '1.2', basename 'foo.bar',
+        and name '1.2.3.4.5', the result would be 'foo.bar.3.4.5'.
 
+        :param device: the device name string
         :param oid: Root OID string
         :param basename: The replacement string of the OID root string
         :param name: An instance of pysnmp.proto.rfc1902.ObjectName
         :param value: Some form of subclass instance of pyasn1.type.base.AbstractSimpleAsn1Item
-        :returns: None or Metric
         """
         # If metric value is 'empty'
         if not value:
             self.log.debug("Metric '{0}' has no value".format(name))
             return None
 
-        # If metric value is a non-integer type
-        if not isinstance(value, IntegerType):
-            self.log.debug("Metric '{0}' is not an Integer type".format(name))
-            return None
+        # Simple integer types need no special work
+        if isinstance(value, IntegerType):
+            value = value.prettyPrint()
+        else:
+            # Otherwise attempt to convert to a float
+            try:
+                value = float(value.prettyPrint())
+            except ValueError:
+                self.log.debug("Metric '{0}' is not an Integer type".format(name))
+                return
 
         # Convert to a simple readable format
         name = name.prettyPrint()
         name = re.sub(r'^{0}'.format(oid), basename, name)
-        value = value.prettyPrint()
 
-        return Metric(path=name,
-                      value=value,
-                      timestamp=time.time(),
-                      precision=self._precision(value),
-                      metric_type='GAUGE')
+        self.log.debug("'{0}' on device '{1}' - value=[{2}]".format(name, device, value))
+        path = '.'.join([
+            'devices',
+            device,
+            name,
+        ])
+
+        self.publish_gauge(path, value)
 
     def get(self, oid, host, port, community):
         """
@@ -177,7 +180,7 @@ def get(self, oid, host, port, community):
         auth = self.create_auth(community)
         transport = self.create_transport(host, port)
         rows = self.snmp_get(oid, auth, transport)
-        return dict((k.prettyPrint(), v.prettyPrint) for k, v in rows)
+        return dict((k.prettyPrint(), v.prettyPrint()) for k, v in rows)
 
     def walk(self, oid, host, port, community):
         """
@@ -185,8 +188,8 @@ def walk(self, oid, host, port, community):
         """
         auth = self.create_auth(community)
         transport = self.create_transport(host, port)
-        rows = self.snmp_walk(oid, auth, transport)
-        return dict((k.prettyPrint(), v.prettyPrint) for k, v in rows)
+        table = self.snmp_walk(oid, auth, transport)
+        return dict((k.prettyPrint(), v.prettyPrint()) for row in table for k, v in row)
 
     def snmp_get(self, oid, auth, transport):
         """
@@ -282,11 +285,6 @@ def collect_snmp(self, device, host, port, community):
             my.metric.name.2.3
         """
         self.log.debug("Collecting SNMP data from device '{0}'".format(device))
-        path_prefix = '.'.join([
-            self.config['path_prefix'],
-            device,
-            self.config['path_suffix']
-        ]).rstrip('.')
 
         auth = self.create_auth(community)
         transport = self.create_transport(host, port)
@@ -300,16 +298,7 @@ def collect_snmp(self, device, host, port, community):
                 fn = self.snmp_get
 
             for metric_name, metric_value in fn(oid, auth, transport):
-                metric = self.create_metric(oid, basename, metric_name, metric_value)
-
-                self.log.debug(
-                    "'{0}' on device '{1}' - value=[{2}]".format(
-                        metric.path, device, metric.value
-                    )
-                )
-
-                metric.path = '.'.join([path_prefix, metric.path])
-                self.publish_metric(metric)
+                self._publish(device, oid, basename, metric_name, metric_value)
 
     def collect(self):
         """
diff --git a/src/collectors/snmp/test/testsnmp.py b/src/collectors/snmp/test/testsnmp.py
index 560d2df32..055132c52 100644
--- a/src/collectors/snmp/test/testsnmp.py
+++ b/src/collectors/snmp/test/testsnmp.py
@@ -1,6 +1,8 @@
 #!/usr/bin/python
 # coding=utf-8
 ################################################################################
+import socket
+
 from mock import Mock, patch
 
 from snmp import SNMPCollector
@@ -25,8 +27,7 @@ def test_import(self):
 
     def test_default_config(self):
         config = self.collector.get_default_config()
-        self.assertEqual(config['path_prefix'], 'devices')
-        self.assertEqual(config['path_suffix'], '')
+        self.assertEqual(config['path'], 'snmp')
         self.assertEqual(config['timeout'], 5)
         self.assertEqual(config['retries'], 3)
         self.assertEqual(config['devices'], {})
@@ -45,36 +46,47 @@ def test_from_oid_tuple_handles_string(self):
         string = '1.2.3'
         self.assertEqual(string, self.collector._from_oid_tuple(string))
 
-    def test_create_metric_empty_value(self):
-        self.assertIsNone(self.collector.create_metric('x', 'y', 'name', None))
+    def test_publish_empty_value(self):
+        with patch.object(self.collector, 'publish_gauge'):
+            self.collector._publish('device', 'x', 'y', 'name', None)
+            self.assertFalse(self.collector.publish_gauge.called)
 
     @patch('snmp.IntegerType', Mock)
-    def test_create_metric_path_replacement(self):
+    def test_publish_path_replacement(self):
         name = Mock(prettyPrint=lambda: '1.2.3.1.2.3')
         value = Mock(prettyPrint=lambda: '42')
 
-        metric = self.collector.create_metric('1.2', 'foo', name, value)
+        with patch.object(self.collector, 'publish_gauge'):
+            self.collector._publish('device', '1.2', 'foo', name, value)
+            self.collector.publish_gauge.assert_called_with(
+                'devices.device.foo.3.1.2.3', '42'
+            )
 
-        self.assertEqual(metric.path, 'foo.3.1.2.3')
+    @patch('snmp.IntegerType', int)
+    def test_publish_non_integer_type(self):
+        value = Mock(prettyPrint=lambda: 'value')
+        with patch.object(self.collector, 'publish_gauge'):
+            self.collector._publish('device', 'x', 'y', 'name', value)
+            self.assertFalse(self.collector.publish_gauge.called)
 
     @patch('snmp.IntegerType', int)
-    def test_create_metric_non_integer_type(self):
-        self.assertIsNone(self.collector.create_metric('x', 'y', 'name', 'value'))
+    def test_publish_non_integer_type_conversion(self):
+        name = Mock(prettyPrint=lambda: 'name')
+        value = Mock(prettyPrint=lambda: '42')
+        with patch.object(self.collector, 'publish_gauge'):
+            self.collector._publish('device', 'x', 'y', name, value)
+            self.collector.publish_gauge.assert_called_with(
+                'devices.device.name', 42.0
+            )
 
     @patch('snmp.IntegerType', Mock)
-    @patch('snmp.time')
-    def test_create_metric(self, time):
-        time.time.return_value = 100
+    def test_publish(self):
         name = Mock(prettyPrint=lambda: '1.2.3')
         value = Mock(prettyPrint=lambda: '42')
 
-        metric = self.collector.create_metric('1.2', 'foo', name, value)
-
-        self.assertEqual(metric.path, 'foo.3')
-        self.assertEqual(metric.value, 42.0)
-        self.assertEqual(metric.precision, 0)
-        self.assertEqual(metric.timestamp, 100)
-        self.assertEqual(metric.metric_type, 'GAUGE')
+        with patch.object(self.collector, 'publish_gauge'):
+            self.collector._publish('device', '1.2', 'foo', name, value)
+            self.collector.publish_gauge.assert_called_with('devices.device.foo.3', '42')
 
     def test_snmp_get_no_metrics(self):
         retvals = [
@@ -103,10 +115,8 @@ def test_snmp_get(self):
         auth = Mock()
         transport = Mock()
 
-        with patch.object(self.collector, 'create_metric'):
-            self.collector.create_metric.return_value = 'bar'
-            metrics = self.collector.snmp_get('1.2', auth, transport)
-            self.assertEqual(metrics, [(name, value)])
+        metrics = self.collector.snmp_get('1.2', auth, transport)
+        self.assertEqual(metrics, [(name, value)])
 
     def test_snmp_walk_no_metrics(self):
         for retval in ([], (None, None, None, [])):
@@ -132,10 +142,8 @@ def test_snmp_walk(self):
         auth = Mock()
         transport = Mock(transportAddr=('localhost', 161))
 
-        with patch.object(self.collector, 'create_metric'):
-            self.collector.create_metric.side_effect = metrics[3]
-            ret_metrics = list(self.collector.snmp_walk('1.2', auth, transport))
-            self.assertEqual(metrics[3], ret_metrics)
+        ret_metrics = list(self.collector.snmp_walk('1.2', auth, transport))
+        self.assertEqual(metrics[3], ret_metrics)
 
     @patch('snmp.IntegerType', Mock)
     @patch('snmp.cmdgen', Mock())
@@ -171,10 +179,13 @@ def test_collect_calls_snmp_get(self):
 
             # Do we publish metrics?
             self.assertEqual(len(calls), 2)
+            prefix = 'servers.{0}.snmp'.format(socket.gethostname())
 
             # Were metrics properly namespaced
-            self.assertEqual(calls[0][0][0].path, 'devices.mydevice.foo.bar')
-            self.assertEqual(calls[1][0][0].path, 'devices.mydevice.foo.bar.4')
+            self.assertEqual(calls[0][0][0].path,
+                             '{0}.devices.mydevice.foo.bar'.format(prefix))
+            self.assertEqual(calls[1][0][0].path,
+                             '{0}.devices.mydevice.foo.bar.4'.format(prefix))
 
     @patch('snmp.IntegerType', Mock)
     @patch('snmp.cmdgen', Mock())
@@ -210,10 +221,13 @@ def test_collect_calls_snmp_walk(self):
 
             # Do we publish metrics?
             self.assertEqual(len(calls), 2)
+            prefix = 'servers.{0}.snmp'.format(socket.gethostname())
 
             # Were metrics properly namespaced
-            self.assertEqual(calls[0][0][0].path, 'devices.mydevice.foo.bar')
-            self.assertEqual(calls[1][0][0].path, 'devices.mydevice.foo.bar.4')
+            self.assertEqual(calls[0][0][0].path,
+                             '{0}.devices.mydevice.foo.bar'.format(prefix))
+            self.assertEqual(calls[1][0][0].path,
+                             '{0}.devices.mydevice.foo.bar.4'.format(prefix))
 
     @patch('snmp.cmdgen', None)
     def test_collect_nothing_with_pysnmp_error(self):

From cdad995205aebb2669e18182ded77a905efd75d6 Mon Sep 17 00:00:00 2001
From: Shaun Duncan <shaun.duncan@gmail.com>
Date: Wed, 14 Jan 2015 16:31:08 -0500
Subject: [PATCH 5/9] Wrap lines >80 chars

---
 src/collectors/snmp/snmp.py          | 49 ++++++++++++++++---------
 src/collectors/snmp/test/testsnmp.py | 55 ++++++++++++++++++++--------
 2 files changed, 72 insertions(+), 32 deletions(-)

diff --git a/src/collectors/snmp/snmp.py b/src/collectors/snmp/snmp.py
index 9a0f85b35..2dea06e70 100644
--- a/src/collectors/snmp/snmp.py
+++ b/src/collectors/snmp/snmp.py
@@ -133,16 +133,19 @@ def _precision(self, value):
 
     def _publish(self, device, oid, basename, name, value):
         """
-        Publishes a metric as a GAUGE with a given value. Non-integer datatyps are attempted
-        to be converted to a float value, and ignored if they cannot be. This will also replace
-        a root OID with a basename. For example, if given a root oid '1.2', basename 'foo.bar',
-        and name '1.2.3.4.5', the result would be 'foo.bar.3.4.5'.
+        Publishes a metric as a GAUGE with a given value. Non-integer
+        datatyps are attempted to be converted to a float value, and
+        ignored if they cannot be. This will also replace a root OID
+        with a basename. For example, if given a root oid '1.2',
+        basename 'foo.bar', and name '1.2.3.4.5', the result would be
+        'foo.bar.3.4.5'.
 
         :param device: the device name string
         :param oid: Root OID string
         :param basename: The replacement string of the OID root string
         :param name: An instance of pysnmp.proto.rfc1902.ObjectName
-        :param value: Some form of subclass instance of pyasn1.type.base.AbstractSimpleAsn1Item
+        :param value: Some form of subclass instance of
+                      pyasn1.type.base.AbstractSimpleAsn1Item
         """
         # If metric value is 'empty'
         if not value:
@@ -157,14 +160,21 @@ def _publish(self, device, oid, basename, name, value):
             try:
                 value = float(value.prettyPrint())
             except ValueError:
-                self.log.debug("Metric '{0}' is not an Integer type".format(name))
+                self.log.debug(
+                    "Metric '{0}' is not an Integer type".format(name)
+                )
                 return
 
         # Convert to a simple readable format
         name = name.prettyPrint()
         name = re.sub(r'^{0}'.format(oid), basename, name)
 
-        self.log.debug("'{0}' on device '{1}' - value=[{2}]".format(name, device, value))
+        self.log.debug(
+            "'{0}' on device '{1}' - value=[{2}]".format(
+                name, device, value
+            )
+        )
+
         path = '.'.join([
             'devices',
             device,
@@ -189,7 +199,8 @@ def walk(self, oid, host, port, community):
         auth = self.create_auth(community)
         transport = self.create_transport(host, port)
         table = self.snmp_walk(oid, auth, transport)
-        return dict((k.prettyPrint(), v.prettyPrint()) for row in table for k, v in row)
+        return dict((k.prettyPrint(), v.prettyPrint())
+                    for row in table for k, v in row)
 
     def snmp_get(self, oid, auth, transport):
         """
@@ -262,22 +273,24 @@ def create_auth(self, community):
 
     def collect_snmp(self, device, host, port, community):
         """
-        Collect SNMP interface data from a device. Devices should be configured with
-        an [oids] section that includes name-value pairs for data to gather. For example:
+        Collect SNMP interface data from a device. Devices should
+        be configured with an [oids] section that includes name-value
+        pairs for data to gather. For example:
 
             [oids]
             1.3.6.1.4.1.1111 = my.metric.name
 
-        There are special circumstances where one could obtain large swaths of data using
-        an SNMP walk. In this situation, metrics aren't named, but namespaced by OID value
-        in graphite:
+        There are special circumstances where one could obtain large
+        swaths of data using an SNMP walk. In this situation, metrics
+        aren't named, but namespaced by OID value in graphite:
 
             [oids]
             1.2.6.1.4.1.1111.* = my.metric.name
 
-        Note, this will replace anything preceding .* with the name on the right hand side.
-        Anything obtained from this walk will be namespaced according to the remaining portion
-        of the OID. For example:
+        Note, this will replace anything preceding .* with the name on
+        the right hand side. Anything obtained from this walk will be
+        namespaced according to the remaining portion of the OID.
+        For example:
 
             my.metric.name.1
             my.metric.name.2.1
@@ -305,7 +318,9 @@ def collect(self):
         Collect stats via SNMP
         """
         if not cmdgen:
-            self.log.error('pysnmp.entity.rfc3413.oneliner.cmdgen failed to load')
+            self.log.error(
+                'pysnmp.entity.rfc3413.oneliner.cmdgen failed to load'
+            )
             return
 
         # If there are no devices, nothing can be collected
diff --git a/src/collectors/snmp/test/testsnmp.py b/src/collectors/snmp/test/testsnmp.py
index 055132c52..73f53f97d 100644
--- a/src/collectors/snmp/test/testsnmp.py
+++ b/src/collectors/snmp/test/testsnmp.py
@@ -86,7 +86,9 @@ def test_publish(self):
 
         with patch.object(self.collector, 'publish_gauge'):
             self.collector._publish('device', '1.2', 'foo', name, value)
-            self.collector.publish_gauge.assert_called_with('devices.device.foo.3', '42')
+            self.collector.publish_gauge.assert_called_with(
+                'devices.device.foo.3', '42'
+            )
 
     def test_snmp_get_no_metrics(self):
         retvals = [
@@ -110,7 +112,9 @@ def test_snmp_get(self):
         value = Mock()
 
         self.collector.cmdgen = Mock()
-        self.collector.cmdgen.getCmd.return_value = (None, None, None, [(name, value)])
+        self.collector.cmdgen.getCmd.return_value = (
+            None, None, None, [(name, value)]
+        )
 
         auth = Mock()
         transport = Mock()
@@ -131,9 +135,12 @@ def test_snmp_walk_no_metrics(self):
 
     def test_snmp_walk(self):
         metrics = (None, None, None, [
-            (Mock(prettyPrint=lambda: '1.2.1'), Mock(prettyPrint=lambda: '41')),
-            (Mock(prettyPrint=lambda: '1.2.2'), Mock(prettyPrint=lambda: '42')),
-            (Mock(prettyPrint=lambda: '1.2.3'), Mock(prettyPrint=lambda: '43')),
+            (Mock(prettyPrint=lambda: '1.2.1'),
+             Mock(prettyPrint=lambda: '41')),
+            (Mock(prettyPrint=lambda: '1.2.2'),
+             Mock(prettyPrint=lambda: '42')),
+            (Mock(prettyPrint=lambda: '1.2.3'),
+             Mock(prettyPrint=lambda: '43')),
         ])
 
         self.collector.cmdgen = Mock()
@@ -162,11 +169,16 @@ def test_collect_calls_snmp_get(self):
         })
 
         metrics = [
-            (Mock(prettyPrint=lambda: '1.2.3'), Mock(prettyPrint=lambda: '42')),
-            (Mock(prettyPrint=lambda: '1.2.3.4'), Mock(prettyPrint=lambda: '43')),
+            (Mock(prettyPrint=lambda: '1.2.3'),
+             Mock(prettyPrint=lambda: '42')),
+            (Mock(prettyPrint=lambda: '1.2.3.4'),
+             Mock(prettyPrint=lambda: '43')),
         ]
 
-        with patch.multiple(self.collector, snmp_get=Mock(), snmp_walk=Mock(), publish_metric=Mock()):
+        with patch.multiple(self.collector,
+                            snmp_get=Mock(),
+                            snmp_walk=Mock(),
+                            publish_metric=Mock()):
             self.collector.snmp_get.return_value = metrics
 
             self.collector.collect_snmp(device, 'localhost', 161, 'public')
@@ -204,11 +216,16 @@ def test_collect_calls_snmp_walk(self):
         })
 
         metrics = [
-            (Mock(prettyPrint=lambda: '1.2.3'), Mock(prettyPrint=lambda: '42')),
-            (Mock(prettyPrint=lambda: '1.2.3.4'), Mock(prettyPrint=lambda: '43')),
+            (Mock(prettyPrint=lambda: '1.2.3'),
+             Mock(prettyPrint=lambda: '42')),
+            (Mock(prettyPrint=lambda: '1.2.3.4'),
+             Mock(prettyPrint=lambda: '43')),
         ]
 
-        with patch.multiple(self.collector, snmp_get=Mock(), snmp_walk=Mock(), publish_metric=Mock()):
+        with patch.multiple(self.collector,
+                            snmp_get=Mock(),
+                            snmp_walk=Mock(),
+                            publish_metric=Mock()):
             self.collector.snmp_walk.return_value = metrics
 
             self.collector.collect_snmp(device, 'localhost', 161, 'public')
@@ -271,9 +288,13 @@ def test_collect(self, cmdgen):
 
         collect_snmp = Mock()
 
-        with patch.multiple(self.collector, config=config, collect_snmp=collect_snmp):
+        with patch.multiple(self.collector,
+                            config=config,
+                            collect_snmp=collect_snmp):
             self.collector.collect()
-            collect_snmp.assert_called_with('mydevice', 'localhost', 161, 'public')
+            collect_snmp.assert_called_with(
+                'mydevice', 'localhost', 161, 'public'
+            )
 
     @patch('snmp.cmdgen')
     def test_collect_uses_defaults(self, cmdgen):
@@ -300,6 +321,10 @@ def test_collect_uses_defaults(self, cmdgen):
 
         collect_snmp = Mock()
 
-        with patch.multiple(self.collector, config=config, collect_snmp=collect_snmp):
+        with patch.multiple(self.collector,
+                            config=config,
+                            collect_snmp=collect_snmp):
             self.collector.collect()
-            collect_snmp.assert_called_with('mydevice', 'localhost', 161, 'public')
+            collect_snmp.assert_called_with(
+                'mydevice', 'localhost', 161, 'public'
+            )

From 7c4cc99e79b9a253ea4abcc2efdb47ec7dfc6c9f Mon Sep 17 00:00:00 2001
From: Shaun Duncan <shaun.duncan@gmail.com>
Date: Wed, 14 Jan 2015 16:35:40 -0500
Subject: [PATCH 6/9] Fix line lengths in snmpraw.py

---
 src/collectors/snmpraw/snmpraw.py | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/src/collectors/snmpraw/snmpraw.py b/src/collectors/snmpraw/snmpraw.py
index af083f7e7..53ee5f3d7 100644
--- a/src/collectors/snmpraw/snmpraw.py
+++ b/src/collectors/snmpraw/snmpraw.py
@@ -1,8 +1,8 @@
 # coding=utf-8
 
 """
-The SNMPRawCollector is a deprecated collector. It's functionality has been moved to the
-top level SNMPCollector
+The SNMPRawCollector is a deprecated collector. It's functionality
+has been moved to the top level SNMPCollector
 
 #### Dependencies
 
@@ -14,8 +14,10 @@
 import sys
 import warnings
 
-sys.path.insert(0, os.path.join(os.path.dirname(os.path.dirname(__file__)), 'snmp'))
+file_path = os.path.dirname(__file__)
+sys.path.insert(0, os.path.join(file_path, 'snmp'))
 
 from snmp import SNMPCollector as SNMPRawCollector  # NOQA
 
-warnings.warn('The SNMPRawCollector is deprecated. Use SNMPCollector instead', DeprecationWarning)
+warnings.warn('SNMPRawCollector is deprecated. Use SNMPCollector',
+              DeprecationWarning)

From b1aac1be95fd567341fb309f946f1af74e1cd074 Mon Sep 17 00:00:00 2001
From: Shaun Duncan <shaun.duncan@gmail.com>
Date: Thu, 19 Mar 2015 10:09:28 -0400
Subject: [PATCH 7/9] Fixed a bug in snmp walk and normalize device name in
 output

---
 src/collectors/snmp/snmp.py          |  4 ++--
 src/collectors/snmp/test/testsnmp.py | 16 +++++++++-------
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/src/collectors/snmp/snmp.py b/src/collectors/snmp/snmp.py
index 2dea06e70..bb96af029 100644
--- a/src/collectors/snmp/snmp.py
+++ b/src/collectors/snmp/snmp.py
@@ -237,7 +237,7 @@ def snmp_walk(self, oid, auth, transport):
         result = self.cmdgen.nextCmd(auth, transport, self._to_oid_tuple(oid))
 
         try:
-            return result[3]
+            return [item[0] for item in result[3]]
         except IndexError:
             self.log.debug(
                 "SNMP WALK '{0}' on host '{1}' returned no data".format(
@@ -311,7 +311,7 @@ def collect_snmp(self, device, host, port, community):
                 fn = self.snmp_get
 
             for metric_name, metric_value in fn(oid, auth, transport):
-                self._publish(device, oid, basename, metric_name, metric_value)
+                self._publish(device.replace('.', '_'), oid, basename, metric_name, metric_value)
 
     def collect(self):
         """
diff --git a/src/collectors/snmp/test/testsnmp.py b/src/collectors/snmp/test/testsnmp.py
index 73f53f97d..ec7850c4a 100644
--- a/src/collectors/snmp/test/testsnmp.py
+++ b/src/collectors/snmp/test/testsnmp.py
@@ -135,14 +135,16 @@ def test_snmp_walk_no_metrics(self):
 
     def test_snmp_walk(self):
         metrics = (None, None, None, [
-            (Mock(prettyPrint=lambda: '1.2.1'),
-             Mock(prettyPrint=lambda: '41')),
-            (Mock(prettyPrint=lambda: '1.2.2'),
-             Mock(prettyPrint=lambda: '42')),
-            (Mock(prettyPrint=lambda: '1.2.3'),
-             Mock(prettyPrint=lambda: '43')),
+            [(Mock(prettyPrint=lambda: '1.2.1'),
+             Mock(prettyPrint=lambda: '41'))],
+            [(Mock(prettyPrint=lambda: '1.2.2'),
+             Mock(prettyPrint=lambda: '42'))],
+            [(Mock(prettyPrint=lambda: '1.2.3'),
+             Mock(prettyPrint=lambda: '43'))],
         ])
 
+        expected = [x[0] for x in metrics[3]]
+
         self.collector.cmdgen = Mock()
         self.collector.cmdgen.nextCmd.return_value = metrics
 
@@ -150,7 +152,7 @@ def test_snmp_walk(self):
         transport = Mock(transportAddr=('localhost', 161))
 
         ret_metrics = list(self.collector.snmp_walk('1.2', auth, transport))
-        self.assertEqual(metrics[3], ret_metrics)
+        self.assertEqual(expected, ret_metrics)
 
     @patch('snmp.IntegerType', Mock)
     @patch('snmp.cmdgen', Mock())

From 9610a66a6c529a3ab3c16a0dd1df168323e17c73 Mon Sep 17 00:00:00 2001
From: Shaun Duncan <shaun.duncan@gmail.com>
Date: Thu, 19 Mar 2015 10:25:38 -0400
Subject: [PATCH 8/9] Fix backwards compatible walk function

---
 src/collectors/snmp/snmp.py | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/collectors/snmp/snmp.py b/src/collectors/snmp/snmp.py
index bb96af029..a2189f350 100644
--- a/src/collectors/snmp/snmp.py
+++ b/src/collectors/snmp/snmp.py
@@ -198,9 +198,8 @@ def walk(self, oid, host, port, community):
         """
         auth = self.create_auth(community)
         transport = self.create_transport(host, port)
-        table = self.snmp_walk(oid, auth, transport)
-        return dict((k.prettyPrint(), v.prettyPrint())
-                    for row in table for k, v in row)
+        rows = self.snmp_walk(oid, auth, transport)
+        return dict((k.prettyPrint(), v.prettyPrint()) for k, v in rows)
 
     def snmp_get(self, oid, auth, transport):
         """

From f814c1b71a8bfd123f05398f3855eeea807bf7e5 Mon Sep 17 00:00:00 2001
From: Shaun Duncan <shaun.duncan@gmail.com>
Date: Sat, 30 May 2015 10:25:00 -0400
Subject: [PATCH 9/9] Fix PEP8 warning on line length

---
 src/collectors/snmp/snmp.py | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/collectors/snmp/snmp.py b/src/collectors/snmp/snmp.py
index a2189f350..923659546 100644
--- a/src/collectors/snmp/snmp.py
+++ b/src/collectors/snmp/snmp.py
@@ -310,7 +310,11 @@ def collect_snmp(self, device, host, port, community):
                 fn = self.snmp_get
 
             for metric_name, metric_value in fn(oid, auth, transport):
-                self._publish(device.replace('.', '_'), oid, basename, metric_name, metric_value)
+                self._publish(device.replace('.', '_'),
+                              oid,
+                              basename,
+                              metric_name,
+                              metric_value)
 
     def collect(self):
         """