Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions disk/assets/configuration/spec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@ files:
start matching from the beginning and therefore to match anywhere you
must prepend `.*`. For exact matches append `$`.

Devices from pseudo or memory-based file systems can be excluded by disabling the
`include_all_devices` option.

When conflicts arise, this will override `file_system_whitelist`.
value:
example:
Expand Down Expand Up @@ -110,6 +113,16 @@ files:
type: array
items:
type: string
- name: include_all_devices
description: |
Instruct the check to collect from all devices, including non-physical devices.
Set this to false to exclude pseudo, memory, duplicate or inaccessible file systems.

For more fine-grained control, use the inclusion and exclusion options.
value:
example: false
type: boolean
default: true
- name: service_check_rw
description: |
Instruct the check to notify based on partition state.
Expand Down
11 changes: 11 additions & 0 deletions disk/datadog_checks/disk/data/conf.yaml.default
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ instances:
## start matching from the beginning and therefore to match anywhere you
## must prepend `.*`. For exact matches append `$`.
##
## Devices from pseudo or memory-based file systems can be excluded by disabling the
## `include_all_devices` option.
##
## When conflicts arise, this will override `file_system_whitelist`.
#
# file_system_blacklist:
Expand Down Expand Up @@ -98,6 +101,14 @@ instances:
# - /dev/sde
# - '[FJ]:'

## @param include_all_devices - boolean - optional - default: True
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is there an uppercase T in True here, whereas it's lowercase in the spec & in all other config options?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 I am not sure why, the conf.yaml.default file is generated automatically by ddev validate config -s using the spec.yaml file.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's how the config validator works since I added both a default and an example value:

if 'default' in value:
default = value['default']
if default is not None:
if type(default) is str:
writer.write(' - default: ', default)
else:
writer.write(' - default: ', repr(default))
else:
if example_type is bool:
writer.write(' - default: ', 'true' if example else 'false')

It prints the repr of the object (which is True since that's how it is written in Python) instead of true or false, which it does when there is no default config.

I am going to change it though, since it seems like elsewhere the example and default values always match and it can be confusing.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened #7405 to make the behavior consistent.

## Instruct the check to collect from all devices, including non-physical devices.
## Set this to false to exclude pseudo, memory, duplicate or inaccessible file systems.
##
## For more fine-grained control, use the inclusion and exclusion options.
#
# include_all_devices: false

## @param service_check_rw - boolean - optional - default: false
## Instruct the check to notify based on partition state.
##
Expand Down
19 changes: 15 additions & 4 deletions disk/datadog_checks/disk/disk.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ def __init__(self, name, init_config, instances):
self._all_partitions = is_affirmative(instance.get('all_partitions', False))
self._file_system_whitelist = instance.get('file_system_whitelist', [])
self._file_system_blacklist = instance.get('file_system_blacklist', [])
# FIXME (8.X): Exclude special file systems by default
self._include_all_devices = is_affirmative(instance.get('include_all_devices', True))
self._device_whitelist = instance.get('device_whitelist', [])
self._device_blacklist = instance.get('device_blacklist', [])
self._mount_point_whitelist = instance.get('mount_point_whitelist', [])
Expand All @@ -71,7 +73,7 @@ def check(self, instance):
self.devices_label = self._get_devices_label()

self._valid_disks = {}
for part in psutil.disk_partitions(all=True):
for part in psutil.disk_partitions(all=self._include_all_devices):
# we check all exclude conditions
if self.exclude_disk(part):
continue
Expand All @@ -88,7 +90,12 @@ def check(self, instance):
)
continue
except Exception as e:
self.log.warning('Unable to get disk metrics for %s: %s', part.mountpoint, e)
self.log.warning(
u'Unable to get disk metrics for %s: %s. '
u'You can exclude this mountpoint in the settings if it is invalid.',
part.mountpoint,
e,
)
continue

# Exclude disks with size less than min_disk_size
Expand Down Expand Up @@ -147,7 +154,6 @@ def _exclude_disk(self, device, file_system, mount_point):
"""
Return True for disks we don't want or that match regex in the config file
"""
self.log.debug('_exclude_disk: %s, %s, %s', device, file_system, mount_point)

if not device or device == 'none':
device = None
Expand Down Expand Up @@ -245,7 +251,12 @@ def _collect_inodes_metrics(self, mountpoint):
)
return metrics
except Exception as e:
self.log.warning('Unable to get disk metrics for %s: %s', mountpoint, e)
self.log.warning(
u'Unable to get disk metrics for %s: %s. '
u'You can exclude this mountpoint in the settings if it is invalid.',
mountpoint,
e,
)
return metrics

if inodes.f_files != 0:
Expand Down
17 changes: 17 additions & 0 deletions disk/tests/test_unit.py
Original file line number Diff line number Diff line change
Expand Up @@ -271,3 +271,20 @@ def f(mountpoint):
aggregator.assert_metric_has_tag(name, 'device_name:{}'.format(DEFAULT_DEVICE_BASE_NAME))

aggregator.assert_all_metrics_covered()


@pytest.mark.usefixtures('psutil_mocks')
def test_include_all_devices(aggregator, gauge_metrics, rate_metrics):
c = Disk('disk', {}, [{}])

with mock.patch('psutil.disk_partitions', return_value=[]) as m:
c.check({})
# By default, we include all devices
m.assert_called_with(all=True)

instance = {'include_all_devices': False}
c = Disk('disk', {}, [instance])

with mock.patch('psutil.disk_partitions', return_value=[]) as m:
c.check({})
m.assert_called_with(all=False)