Skip to content

Add device_name tag#6332

Merged
ofek merged 4 commits intomasterfrom
therve/disk-device
Apr 16, 2020
Merged

Add device_name tag#6332
ofek merged 4 commits intomasterfrom
therve/disk-device

Conversation

@therve
Copy link
Copy Markdown
Contributor

@therve therve commented Apr 14, 2020

This adds a new tag which only contains the base part of the device, for
compatibility with iostats check.

@therve therve requested review from a team as code owners April 14, 2020 13:52
albertvaka
albertvaka previously approved these changes Apr 14, 2020
Copy link
Copy Markdown
Contributor

@albertvaka albertvaka left a comment

Choose a reason for hiding this comment

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

LGTM for 7.20

@albertvaka
Copy link
Copy Markdown
Contributor

Related to: DataDog/datadog-agent#5311

Comment thread disk/datadog_checks/disk/disk.py Outdated
device_name = device_name.strip('\\').lower()

tags.append('device:{}'.format(device_name))
tags.append('device_name:{}'.format(os.path.basename(device_name)))
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.

I think the device_name tag should be consistent and always be the part.device L90. But the tag device should be retrocompatible and follow the use_mount option.

This adds a new tag which only contains the base part of the device, for
compatibility with iostats check.
@therve therve force-pushed the therve/disk-device branch from 10498c4 to 764edad Compare April 15, 2020 07:31
kbogtob
kbogtob previously approved these changes Apr 15, 2020
Copy link
Copy Markdown
Contributor

@kbogtob kbogtob left a comment

Choose a reason for hiding this comment

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

LGTM for 7.20! Thanks a lot.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 15, 2020

Comment thread disk/datadog_checks/disk/disk.py Outdated
IGNORE_CASE = re.I if platform.system() == 'Windows' else 0


def _base_device_name(device):
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.

Let's invert the conditional to choose a function def

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry I don't understand what you mean.

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.

The OS will never change, so choose logic once at load time

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.

In fact, re-use the above if platform.system() == 'Windows'

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh I see, thanks.

Copy link
Copy Markdown
Contributor

@ofek ofek left a comment

Choose a reason for hiding this comment

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

🔥

Copy link
Copy Markdown
Contributor

@albertvaka albertvaka left a comment

Choose a reason for hiding this comment

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

🚀

@ofek ofek merged commit 8fda5f7 into master Apr 16, 2020
@ofek ofek deleted the therve/disk-device branch April 16, 2020 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants