Skip to content

Add Ip and Tcp metric collection#11170

Merged
steveny91 merged 4 commits into
masterfrom
bonnefoa/ip-frag
Apr 4, 2022
Merged

Add Ip and Tcp metric collection#11170
steveny91 merged 4 commits into
masterfrom
bonnefoa/ip-frag

Conversation

@bonnefoa

@bonnefoa bonnefoa commented Jan 19, 2022

Copy link
Copy Markdown
Collaborator

What does this PR do?

Collect additional IP and TCP metrics from /proc/net/{snmp,netstat}.

Motivation

For IP, we are interested in errors (checksum issues, invalid headers) and detailed informations about fragmentation. Knowing when an instance is fragmenting sent packets or has to reassemble fragmented packet can be a good indicator of a MTU issue somewhere in the network path.
For TCP, the connection metrics (ActiveOpens, PassiveOpens, AttemptFails...) will provide better visibility to detect connection churn. system.net.tcp4.opening only gives a snapshot of connections in an open state and is expensive to compute on instance with a lot of socket to parse.

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

@bonnefoa
bonnefoa requested review from a team as code owners January 19, 2022 08:45
@ghost ghost added the integration/network label Jan 19, 2022
@github-actions

github-actions Bot commented Jan 19, 2022

Copy link
Copy Markdown
Contributor

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

coignetp
coignetp requested changes Jan 20, 2022
View reviewed changes
Comment thread network/tests/test_network.py
coignetp
coignetp previously approved these changes Jan 20, 2022
View reviewed changes
albertvaka
albertvaka reviewed Jan 26, 2022
View reviewed changes

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

Other Linux-only metrics explicitly say "Linux-only" in the metadata. Should we do the same here? Having so many Linux-specific metrics we should probably start looking for a way to mention that that scales better, but 🤷

Also, are these available on all Linux systems (even on older distros)? If they are not, what does the check do in that case? At the very least it shouldn't blow up but ideally it should not report them at all.

@bonnefoa

bonnefoa commented Jan 26, 2022

Copy link
Copy Markdown
Collaborator Author

I've added the (Linux Only) to the new metric metadata.
The extended metrics will differ with the kernel version but they are only pushed when they are in the /proc/net/netstat output. Otherwise, they are not reported (see https://github.com/DataDog/integrations-core/blob/master/network/datadog_checks/network/network.py#L538-L543)

@bonnefoa
bonnefoa requested a review from albertvaka January 26, 2022 14:55
albertvaka
albertvaka previously approved these changes Jan 26, 2022
View reviewed changes

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

LGTM

sarah-witt
sarah-witt reviewed Jan 31, 2022
View reviewed changes

@sarah-witt sarah-witt 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, looks good! I just had two nits about metrics metadata

Comment thread network/metadata.csv Outdated
Comment thread network/metadata.csv Outdated
sarah-witt
sarah-witt previously approved these changes Jan 31, 2022
View reviewed changes
@codecov

codecov Bot commented Feb 15, 2022 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #11170 (d555324) into master (9fd40de) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Flag Coverage Δ
network 79.92% <100.00%> (+2.72%) ⬆️

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

sarah-witt
sarah-witt previously approved these changes Feb 17, 2022
View reviewed changes
albertvaka
albertvaka previously approved these changes Feb 17, 2022
View reviewed changes
@albertvaka

albertvaka commented Feb 17, 2022

Copy link
Copy Markdown
Contributor

@bonnefoa @DrynnBavis I think we need the metadata update to also include this PR in addition to #11156

@bonnefoa
bonnefoa dismissed stale reviews from albertvaka and sarah-witt via 7d95878 March 16, 2022 07:56
roxelo
roxelo previously approved these changes Mar 29, 2022
View reviewed changes
@steveny91
steveny91 merged commit e57a23e into master Apr 4, 2022
@steveny91
steveny91 deleted the bonnefoa/ip-frag branch April 4, 2022 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coignetp coignetp coignetp left review comments
@sarah-witt sarah-witt sarah-witt left review comments
+3 more reviewers
@albertvaka albertvaka albertvaka approved these changes
@yzhan289 yzhan289 yzhan289 approved these changes
@roxelo roxelo roxelo left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

integration/network

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@bonnefoa @albertvaka @roxelo @coignetp @yzhan289 @sarah-witt @steveny91