Skip to content

adding mtu and speed tags to interfaces that support it#18819

Merged
iliakur merged 2 commits into
DataDog:masterfrom
ocient-cliimatta:update-network-integration
Oct 17, 2024
Merged

adding mtu and speed tags to interfaces that support it#18819
iliakur merged 2 commits into
DataDog:masterfrom
ocient-cliimatta:update-network-integration

Conversation

@ocient-cliimatta

@ocient-cliimatta ocient-cliimatta commented Oct 11, 2024

Copy link
Copy Markdown
Contributor

What does this PR do?

This PR will add two new tags (mtu and speed) to any network device that supports that information out of /sys/class/net/
If the data cannot be determined it is skipped (for example, the lo interface has no speed).

The following system.net metrics will receive the new tags:

bytes_rcvd
bytes_sent
packets_in.count
packets_in.drop
packets_in.error
packets_out.count
packets_out.drop
packets_out.error

Motivation

It's currently not possible to build host network device dashboards grouped by speed, unless you manually define which interfaces are which speed (this can be different between OS flavors, or just different server configurations).

There is an existing metric named system.net.iface.mtu which is tagged by iface, but in my opinion mtu makes more sense to be in a tag, MTU does not change often (or ever) in a server's lifetime. Graphing MTU in a timeseries is not very useful.

With this change, you could build dashboards where you can differentiate between your 1Gb interfaces vs. your 10Gb interfaces like this:
avg:system.net.bytes_rcvd{host:$host, speed:10000} by {device}
or
avg:system.net.bytes_rcvd{host:$host} by {device,speed}

Additional Notes

This change is only tested on linux, but could be extended to the other OS specific network check files

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • Changelog entries must be created for modifications to shipped code
  • Add the qa/skip-qa label if the PR doesn't need to be tested during QA.
  • If you need to backport this PR to another branch, you can add the backport/<branch-name> label to the PR and it will automatically open a backport PR once this one is merged

@ocient-cliimatta
ocient-cliimatta requested review from a team as code owners October 11, 2024 14:39
ameech
ameech approved these changes Oct 11, 2024
View reviewed changes

@ameech ameech 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.

lgtm

@codecov

codecov Bot commented Oct 11, 2024 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.44%. Comparing base (9d54b18) to head (86fecf0).
Report is 25 commits behind head on master.

Additional details and impacted files
Flag Coverage Δ
activemq ?
cassandra ?
hive ?
hivemq ?
hudi ?
ignite ?
jboss_wildfly ?
kafka ?
network 93.77% <100.00%> (+0.18%) ⬆️
presto ?
solr ?

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

iliakur
iliakur requested changes Oct 14, 2024
View reviewed changes

@iliakur iliakur left a comment

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.

Thanks for the contribution @ocient-cliimatta!

Do I understand correctly that this will only work on linux?

Could you add a changelog entry and fix the formatting?

Here are some links to get you started:

@ocient-cliimatta

ocient-cliimatta commented Oct 14, 2024

Copy link
Copy Markdown
Contributor Author

Thanks for the contribution @ocient-cliimatta!

Do I understand correctly that this will only work on linux?

To clarify, the changes proposed here will only affect Linux systems, since the network check files are split out by major OS distribution name. My change only modifies check_linux.py, so it won't even run on any other OS types

@iliakur

iliakur commented Oct 17, 2024

Copy link
Copy Markdown
Contributor

@ocient-cliimatta any tips for how to add unit tests for this? I'll do that as a follow-up, but would appreciate pointers when it comes to the data that's necessary.

@iliakur
iliakur added this pull request to the merge queue Oct 17, 2024
Merged via the queue into DataDog:master with commit 725e35e Oct 17, 2024
@ocient-cliimatta

ocient-cliimatta commented Oct 17, 2024

Copy link
Copy Markdown
Contributor Author

@iliakur This integration is a bit strange, it seems that some of the metrics are emitted when running datadog-agent check network, but not all. Some of them are emitted by the system integration.

My change uses the _read_int_file() method to return the contents of mtu and speed under /sys/class/net/<adapter>, which is mocked up here

That test already covers opening the mtu file since it's used as the value of system.net.iface.mtu - but there's no test that returns the speed.

I would suggest adding a test for that path, usually interface speeds would be one of 10/100/1000/10000/100000
this would be a 10Gb interface:

elif location == '/sys/class/net/ens5/speed':
    return 10000

And test for an "invalid" adapter name

elif location == '/sys/class/net/invalid/speed':
        return None

I can't tell if any of the tests assert that tag values are set, but if such a thing exists, the metrics listed here should all include something like tags: ['speed:10000', 'mtu:1500']

Looks like the metrics themselves are checked here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iliakur iliakur iliakur approved these changes
+1 more reviewer
@ameech ameech ameech approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

agent/approved ecosystems/review-requested integration/network product/review-requested team/agent-integrations team/windows-agent

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ocient-cliimatta @iliakur @ameech