[disk] backward compatibility with old Disk check

checks.d/disk.py

@@ -19,7 +19,6 @@ class Disk(AgentCheck):
     """ Collects metrics about the machine's disks. """
     # -T for filesystem info
     DF_COMMAND = ['df', '-T']
-    FAKE_DEVICES = ['udev', 'sysfs', 'rpc_pipefs', 'proc', 'devpts']
     METRIC_DISK = 'system.disk.{0}'
     METRIC_INODE = 'system.fs.inodes.{0}'
 
@@ -38,6 +37,7 @@ def check(self, instance):
         if self._psutil():
             self.collect_metrics_psutil()
         else:
+            # FIXME: implement all_partitions (df -a)
             self.collect_metrics_manually()
 
     @classmethod
@@ -49,12 +49,8 @@ def _load_conf(self, instance):
         self._excluded_disks = instance.get('excluded_disks', [])
         self._tag_by_filesystem = _is_affirmative(
             instance.get('tag_by_filesystem', False))
-        # On Windows, we need all_partitions to True by default to collect
-        # metrics about remote disks
-        # On Linux, we need all_partitions to False to avoid collecting metrics
-        # about nodev filesystems
         self._all_partitions = _is_affirmative(
-            instance.get('all_partitions', Platform.is_win32()))
+            instance.get('all_partitions', False))
 
         # FIXME: 6.x, drop use_mount option in datadog.conf
         self._load_legacy_option(instance, 'use_mount', False,
@@ -79,20 +75,31 @@ def _load_legacy_option(self, instance, option, default,
 
     def collect_metrics_psutil(self):
         self._valid_disks = {}
-        for part in psutil.disk_partitions(all=self._all_partitions):
+        for part in psutil.disk_partitions(all=True):
             # we check all exclude conditions
             if self._exclude_disk_psutil(part):
                 continue
+            # Get disk metrics here to be able to exclude on total usage
+            try:
+                disk_usage = psutil.disk_usage(part.mountpoint)
+            except Exception, e:
+                self.log.debug("Unable to get disk metrics for %s: %s",
+                               part.mountpoint, e)
+                continue
+            # Exclude disks with total disk size 0
+            if disk_usage.total == 0:
+                continue
             # For later, latency metrics
             self._valid_disks[part.device] = (part.fstype, part.mountpoint)
             self.log.debug('Passed: {0}'.format(part.device))
 
             tags = [part.fstype] if self._tag_by_filesystem else []
             device_name = part.mountpoint if self._use_mount else part.device
+
             # legacy check names c: vs psutil name C:\\
             if Platform.is_win32():
                 device_name = device_name.strip('\\').lower()
-            for metric_name, metric_value in self._collect_part_metrics(part).iteritems():
+            for metric_name, metric_value in self._collect_part_metrics(part, disk_usage).iteritems():
                 self.gauge(metric_name, metric_value,
                            tags=tags, device_name=device_name)
         # And finally, latency metrics, a legacy gift from the old Windows Check
@@ -110,13 +117,12 @@ def _exclude_disk_psutil(self, part):
 
     # We don't want all those incorrect devices
     def _exclude_disk(self, name, filesystem):
-        return (name in self.FAKE_DEVICES or
+        return (((not name or name == 'none') and not self._all_partitions) or
                 name in self._excluded_disks or
                 self._excluded_disk_re.match(name) or
                 filesystem in self._excluded_filesystems)
 
-    def _collect_part_metrics(self, part):
-        usage = psutil.disk_usage(part.mountpoint)
+    def _collect_part_metrics(self, part, usage):
         metrics = {}
         for name in ['total', 'used', 'free']:
             # For legacy reasons,  the standard unit it kB

conf.d/disk.yaml.default

@@ -6,8 +6,8 @@ instances:
   - use_mount: no
     # The (optional) excluded_filesystems parameter will instruct the check to
     # ignore disks using these filesystems
-    excluded_filesystems:
-      - tmpfs
+    # excluded_filesystems:
+    #   - tmpfs
 
     # The (optional) excluded_disks parameter will instruct the check to
     # ignore this list of disks
@@ -24,7 +24,6 @@ instances:
     # tag_by_filesystem: no
     #
     # The (optional) all_partitions parameter will instruct the check to
-    # get metrics for all partitions (and not only physical disks)
-    # On Windows, this parameter default is 'yes' in order to get remote disks
-    # metrics; if they are not needed, set it to 'no'
+    # get metrics for all partitions. use_mount should be set to yes (to avoid
+    # collecting empty device names) when using this option.
     # all_partitions: no

tests/checks/mock/test_disk.py

@@ -67,7 +67,9 @@ def test_device_exclusion_logic(self):
         self.assertFalse(exclude_disk(MockPart()))
 
         # standard fake devices
-        self.assertTrue(exclude_disk(MockPart(device='udev')))
+        self.assertTrue(exclude_disk(MockPart(device='')))
+        self.assertTrue(exclude_disk(MockPart(device='none')))
+        self.assertFalse(exclude_disk(MockPart(device='udev')))
 
         # excluded filesystems list
         self.assertTrue(exclude_disk(MockPart(fstype='aaaaaa')))
@@ -81,6 +83,12 @@ def test_device_exclusion_logic(self):
         self.assertTrue(exclude_disk(MockPart(device='tevvv')))
         self.assertFalse(exclude_disk(MockPart(device='tevvs')))
 
+        # and now with all_partitions
+        self.check._all_partitions = True
+        self.assertFalse(exclude_disk(MockPart(device='')))
+        self.assertFalse(exclude_disk(MockPart(device='none')))
+        self.assertFalse(exclude_disk(MockPart(device='udev')))
+
     @mock.patch('psutil.disk_partitions', return_value=[MockPart()])
     @mock.patch('psutil.disk_usage', return_value=MockDiskMetrics())
     @mock.patch('os.statvfs', return_value=MockInodesMetrics())
@@ -97,29 +105,6 @@ def test_psutil(self, mock_partitions, mock_usage, mock_inodes):
 
             self.coverage_report()
 
-    # FIXME: patch the import of Platform to be able to test mocked Windows
-    # @mock.patch('psutil.disk_partitions',
-    #             return_value=[MockPart(device='C:\\', fstype='ntfs',
-    #                                    mountpoint='C:\\')])
-    # @mock.patch('psutil.disk_usage', return_value=MockDiskMetrics())
-    # @mock.patch('psutil.disk_io_counters',
-    #             return_value={'PhysicalDisk0': MockIoCountersMetrics()})
-    # @mock.patch('util.Platform', return_value=WindowsPlatform)
-    # def test_psutil_windows(self, mock_partitions, mock_usage,
-    #                         mock_io_counters, mock_platform):
-    #     self.run_check({'instances': [{'use_mount': 'no'}]},
-    #                    mocks={'_psutil': lambda: True})
-    #
-    #     # Assert metrics
-    #     for metric, value in self.GAUGES_VALUES.iteritems():
-    #         self.assertMetric(metric, value=value, tags=[],
-    #                           device_name='c:')
-    #     for metric, value in self.GAUGES_VALUES_PSUTIL.iteritems():
-    #         self.assertMetric(metric, value=value, tags=[],
-    #                           device_name='PhysicalDisk0')
-    #
-    #     self.coverage_report()
-
     @mock.patch('psutil.disk_partitions', return_value=[MockPart()])
     @mock.patch('psutil.disk_usage', return_value=MockDiskMetrics())
     @mock.patch('os.statvfs', return_value=MockInodesMetrics())