Skip to content

Add saturation metrics for network#10551

Merged
bkabrda merged 4 commits into
DataDog:masterfrom
luhenry:saturation-metrics
Jan 7, 2022
Merged

Add saturation metrics for network#10551
bkabrda merged 4 commits into
DataDog:masterfrom
luhenry:saturation-metrics

Conversation

@luhenry

@luhenry luhenry commented Nov 3, 2021

Copy link
Copy Markdown
Contributor

What does this PR do?

Add saturation metrics for network.

Motivation

We currently gather utilization and error metrics of network (bytes received and sent, number of packets) but not the saturation metrics (number of dropped packets).

Having this additional data is needed to apply the USE Method [1] for system performance analysis.

[1] https://www.brendangregg.com/usemethod.html

Additional Notes

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

codecov Bot commented Nov 3, 2021

Copy link
Copy Markdown

Codecov Report

Merging #10551 (7fd072c) into master (3b7604d) will increase coverage by 0.00%.
The diff coverage is 77.77%.

Flag Coverage Δ
network 77.73% <77.77%> (+0.97%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

We currently gather utilization and error metrics of network (bytes received and sent, number of packets) but not the saturation metrics (number of dropped packets).

Having this additional data is needed to apply the USE Method [1] for system performance analysis.

[1] https://www.brendangregg.com/usemethod.html
@luhenry
luhenry force-pushed the saturation-metrics branch from 0aa06fe to c349fcb Compare November 3, 2021 14:29
@bkabrda

bkabrda commented Nov 4, 2021

Copy link
Copy Markdown

Hey @luhenry, thanks for the PR! I think overall this looks ok, but I think it'd be best to only send the metric on platforms where we can actually collect them. I think sending 0 as value on platforms where we can't collect could be misleading for users. (Ideally, the metadata descriptions of the metrics should reflect this fact).

@luhenry

luhenry commented Nov 8, 2021

Copy link
Copy Markdown
Contributor Author

@bkabrda I've updated the PR to not send 0 when the metric isn't available.

@bkabrda bkabrda left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hi 👋 the code mostly looks good now, but there are some fixes that need to be made in order to ensure we don't break support for BSD/Solaris.

Comment thread network/datadog_checks/network/network.py Outdated
Comment thread network/tests/common.py
@luhenry
luhenry force-pushed the saturation-metrics branch from 3e139e1 to 55a985c Compare November 9, 2021 09:32
@luhenry
luhenry force-pushed the saturation-metrics branch from 55a985c to 8fc0988 Compare November 9, 2021 09:56
bkabrda
bkabrda previously approved these changes Nov 9, 2021
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.

3 participants