[disk] Add `include_all_devices` option and improve error logs #7378

mx-psi · 2020-08-17T10:58:01Z

What does this PR do?

Add option to exclude non-physical devices.
Improve logging about failing devices and nudge users towards using the settings to disable them if needed.
Remove debug log which was consistently confusing to support engineers.

Motivation

Address common pain points of users of the check and support engineers.

Additional Notes

See all argument for disk_partitions.

Review checklist (to be filled by reviewers)

Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
PR title must be written as a CHANGELOG entry (see why)
Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
PR must have changelog/ and integration/ labels attached

codecov · 2020-08-17T11:05:26Z

Codecov Report

Merging #7378 into master will increase coverage by 6.17%.
The diff coverage is 86.66%.

Impacted Files	Coverage Δ
disk/datadog_checks/disk/disk.py	`87.25% <50.00%> (+13.54%)`	⬆️
disk/tests/test_unit.py	`100.00% <100.00%> (+5.97%)`	⬆️
ibm_mq/datadog_checks/ibm_mq/stats/base_stats.py
istio/datadog_checks/istio/metrics.py
istio/datadog_checks/istio/istio.py
...heduler/datadog_checks/kube_scheduler/__about__.py
...og_checks_dev/tests/tooling/configuration/utils.py
gitlab/datadog_checks/gitlab/__init__.py
airflow/tests/test_check_metrics_up_to_date.py
go_expvar/datadog_checks/go_expvar/__about__.py
... and 804 more

Improve log messages

disk/datadog_checks/disk/disk.py

It makes no sense to use `file_system_whitelist` in the way previously mentioned, if users want to have more fine grained control they can just specify the lists themselves

KSerrania

LGTM, I see that an empty disk/log/.lock file got added, which can probably be removed.

disk/assets/configuration/spec.yaml

This does the same that this PR intended but supporting all operating systems and by checking (on Linux) if the file system is backed by a block device.

KSerrania

LGTM, left a comment about the check documentation.

KSerrania · 2020-08-19T15:05:08Z

disk/datadog_checks/disk/data/conf.yaml.default

@@ -98,6 +101,14 @@ instances:
    #   - /dev/sde
    #   - '[FJ]:'

+    ## @param include_all_devices - boolean - optional - default: True


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

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

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

integrations-core/datadog_checks_dev/datadog_checks/dev/tooling/configuration/consumers/example.py

Lines 115 to 124 in fda6a83

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.

I opened #7405 to make the behavior consistent.

This way it is more consistent with the rest of options

mx-psi added the changelog/Fixed label Aug 17, 2020

github-actions bot added the integration/disk label Aug 17, 2020

mx-psi force-pushed the mx-psi/disk-warning branch from dc85436 to 5c679fe Compare August 17, 2020 11:06

github-actions bot added the documentation label Aug 17, 2020

mx-psi force-pushed the mx-psi/disk-warning branch 2 times, most recently from 79b1843 to 53a6257 Compare August 17, 2020 14:46

mx-psi added changelog/Added and removed changelog/Fixed labels Aug 17, 2020

[disk] Add option to exclude special file systems on Linux

ef53636

Improve log messages

mx-psi force-pushed the mx-psi/disk-warning branch from 53a6257 to ef53636 Compare August 17, 2020 15:13

mx-psi marked this pull request as ready for review August 17, 2020 15:27

mx-psi requested review from a team as code owners August 17, 2020 15:27

KSerrania reviewed Aug 17, 2020

View reviewed changes

disk/datadog_checks/disk/disk.py Outdated Show resolved Hide resolved

mx-psi added 3 commits August 18, 2020 12:09

Rewrite test and spec

e09657e

It makes no sense to use `file_system_whitelist` in the way previously mentioned, if users want to have more fine grained control they can just specify the lists themselves

Address review comment

0b351bb

Fix test

ce5c4cc

mx-psi requested a review from KSerrania August 18, 2020 10:50

KSerrania previously approved these changes Aug 18, 2020

View reviewed changes

Delete .lock

09e8990

mx-psi dismissed KSerrania’s stale review via 09e8990 August 18, 2020 12:16

KSerrania previously approved these changes Aug 18, 2020

View reviewed changes

hithwen reviewed Aug 18, 2020

View reviewed changes

disk/assets/configuration/spec.yaml Outdated Show resolved Hide resolved

Use all option from psutil.disk_partitions

20e5d20

This does the same that this PR intended but supporting all operating systems and by checking (on Linux) if the file system is backed by a block device.

mx-psi dismissed KSerrania’s stale review via 20e5d20 August 19, 2020 14:18

Sync config

fda6a83

mx-psi requested a review from KSerrania August 19, 2020 14:27

KSerrania previously approved these changes Aug 19, 2020

View reviewed changes

mx-psi changed the title ~~[disk] Improve excluded devices messages and defaults~~ [disk] Add include_all_devices option and improve error logs Aug 20, 2020

Use default instead of example

2271969

This way it is more consistent with the rest of options

mx-psi dismissed KSerrania’s stale review via 2271969 August 20, 2020 08:50

KSerrania approved these changes Aug 21, 2020

View reviewed changes

mx-psi merged commit 8793c78 into master Aug 21, 2020

mx-psi deleted the mx-psi/disk-warning branch August 21, 2020 13:24

mx-psi mentioned this pull request Aug 21, 2020

dd.collector[1724]: WARNING (disk.py:109): Unable to get disk metrics for ... DataDog/dd-agent#2932

Closed

mx-psi mentioned this pull request Sep 24, 2020

[disk] Blacklist certain partitions by default #2492

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[disk] Add `include_all_devices` option and improve error logs #7378

[disk] Add `include_all_devices` option and improve error logs #7378

mx-psi commented Aug 17, 2020 •

edited

Loading

codecov bot commented Aug 17, 2020 •

edited

Loading

KSerrania left a comment

KSerrania left a comment

KSerrania Aug 19, 2020

mx-psi Aug 19, 2020

mx-psi Aug 20, 2020

mx-psi Aug 20, 2020

	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')

[disk] Add include_all_devices option and improve error logs #7378

[disk] Add include_all_devices option and improve error logs #7378

Conversation

mx-psi commented Aug 17, 2020 • edited Loading

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

codecov bot commented Aug 17, 2020 • edited Loading

Codecov Report

KSerrania left a comment

Choose a reason for hiding this comment

KSerrania left a comment

Choose a reason for hiding this comment

KSerrania Aug 19, 2020

Choose a reason for hiding this comment

mx-psi Aug 19, 2020

Choose a reason for hiding this comment

mx-psi Aug 20, 2020

Choose a reason for hiding this comment

mx-psi Aug 20, 2020

Choose a reason for hiding this comment

[disk] Add `include_all_devices` option and improve error logs #7378

[disk] Add `include_all_devices` option and improve error logs #7378

mx-psi commented Aug 17, 2020 •

edited

Loading

codecov bot commented Aug 17, 2020 •

edited

Loading