Skip to content

Add conntrack metrics#3624

Merged
coignetp merged 4 commits into
masterfrom
paul/network-enh
Apr 23, 2019
Merged

Add conntrack metrics#3624
coignetp merged 4 commits into
masterfrom
paul/network-enh

Conversation

@coignetp

@coignetp coignetp commented Apr 15, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

What does this PR do?

Add conntrack metrics to the network check.

Some metrics are only available with sudo conntrack -S, so the check is calling the subprocess to have:

  • system.net.conntrack.drop
  • system.net.conntrack.early_drop
  • system.net.conntrack.error
  • system.net.conntrack.found
  • system.net.conntrack.ignore
  • system.net.conntrack.insert
  • system.net.conntrack.insert_failed
  • system.net.conntrack.invalid
  • system.net.conntrack.search_restart
    They are tagged by cpu #.

The rest is available in {proc_location}/sys/net/netfilter/. The most interesting ones are system.net.conntrack.count and system.net.conntrack.max enabled by default.

Motivation

Customer and internal request.

Additional Notes

Information available since kernel 3.6

Some documentation still need to be done for the different metrics available.
The documentation in the metadata.csv mainly comes from here

cc @lbernail

Review checklist (to be filled by reviewers)

  • 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
  • Feature or bugfix must have tests
  • Git history must be clean
  • If PR adds a configuration option, it must be added to the configuration file.

@coignetp
coignetp requested review from a team as code owners April 15, 2019 16:28
@codecov

codecov Bot commented Apr 15, 2019 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #3624 into master will decrease coverage by 13.38%.
The diff coverage is 84.37%.

@@            Coverage Diff             @@
##           master   #3624       +/-   ##
==========================================
- Coverage   85.98%   72.6%   -13.39%     
==========================================
  Files         741       7      -734     
  Lines       39177     449    -38728     
  Branches     4712      81     -4631     
==========================================
- Hits        33688     326    -33362     
+ Misses       4196     100     -4096     
+ Partials     1293      23     -1270

@dabcoder dabcoder mentioned this pull request Apr 16, 2019
Closed
6 tasks
lbernail
lbernail reviewed Apr 16, 2019
View reviewed changes
Comment thread network/datadog_checks/network/network.py Outdated
lbernail
lbernail reviewed Apr 16, 2019
View reviewed changes
Comment thread network/datadog_checks/network/network.py Outdated
lbernail
lbernail reviewed Apr 16, 2019
View reviewed changes
Comment thread network/datadog_checks/network/network.py Outdated
lbernail
lbernail reviewed Apr 16, 2019
View reviewed changes
Comment thread network/datadog_checks/network/network.py
@lbernail

lbernail commented Apr 16, 2019

Copy link
Copy Markdown

Went through the PR quickly, looks good. Looking forward to testing it on our cluster
I'm skeptical about the parameters we retrieve as metrics:

  • booleans
  • timeouts

Maybe we can reach out to the customer and get more info on their use cases? I'm pretty sure they are mostly interested in stats and just ls in the proc conntrack directory without checking

@coignetp

coignetp commented Apr 16, 2019

Copy link
Copy Markdown
Contributor Author

Thanks @lbernail !

Since pyroute2 needs sudo to get the conntrack information, we get these information with calling the subprocess. This subprocess is not available by default everywhere, so there is an option to enable it.

For the list of metrics in /${proc}/sys/net/netfilter/, there is now a whitelist/blacklist configuration.

zippolyte
zippolyte reviewed Apr 17, 2019
View reviewed changes

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

A few comments, looking good otherwise

Comment thread network/datadog_checks/network/data/conf.yaml.default Outdated
Comment thread network/datadog_checks/network/network.py Outdated
Comment thread network/datadog_checks/network/network.py Outdated
Comment thread network/datadog_checks/network/network.py Outdated
Comment thread network/datadog_checks/network/network.py Outdated
Comment thread network/datadog_checks/network/data/conf.yaml.default Outdated
Comment thread network/datadog_checks/network/data/conf.yaml.default Outdated
Comment thread network/datadog_checks/network/data/conf.yaml.default Outdated
Comment thread network/datadog_checks/network/data/conf.yaml.default Outdated
Comment thread network/datadog_checks/network/network.py Outdated
@coignetp
coignetp requested a review from a team as a code owner April 17, 2019 12:14
@dabcoder dabcoder mentioned this pull request Apr 19, 2019
Closed
apigirl
apigirl approved these changes Apr 22, 2019
View reviewed changes

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

approved grammar/style for docs

@coignetp
coignetp merged commit 0497a14 into master Apr 23, 2019
@coignetp
coignetp deleted the paul/network-enh branch April 23, 2019 08:57
@yzhan289 yzhan289 mentioned this pull request Sep 14, 2022
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zippolyte zippolyte zippolyte approved these changes
+2 more reviewers
@lbernail lbernail lbernail left review comments
@apigirl apigirl apigirl approved these changes
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.

4 participants

@coignetp @lbernail @zippolyte @apigirl