Add conntrack basic metrics to the network integration.#2981
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2981 +/- ##
===========================================
- Coverage 84.9% 68.39% -16.52%
===========================================
Files 665 4 -661
Lines 36455 386 -36069
Branches 4337 71 -4266
===========================================
- Hits 30953 264 -30689
+ Misses 4224 100 -4124
+ Partials 1278 22 -1256 |
|
Sorry I just noticed the job was failing. |
gzussa
left a comment
gzussa
left a comment
There was a problem hiding this comment.
Thanks @aerostitch This looks really good. I just have one question below out of curiosity and minor comments.
| conntrack_count += conntrack_buffer.count('\n') | ||
| self.gauge('system.net.conntrack.count', conntrack_count, tags=custom_tags) | ||
| except IOError: | ||
| self.log.debug("Unable to read %s. Skipping conntrack metrics pull.", proc_conntrack_path) |
There was a problem hiding this comment.
Hi, should we change conntrack to nf_conntrack like formatted below?
There was a problem hiding this comment.
The reason I didn't put nf_conntrack is that one of the enhancements could be to also have separate count for ipv4 and ipv6 which would result in metrics looking like:
system.net.conntrack.count=> totall conntrack countsystem.net.conntrack.ipv4.count=> total conntrack count for ipv4 connections onlysystem.net.conntrack.ipv6.count=> total conntrack count for ipv6 connections only
But if you prefer I can use nf_conntrack, just let me know if an enhancement like that would make sense or if we should rather put the ipv4 conntrack in system.net.ipv4.conntrack.count in which case system.net.nf_conntrack.count would probably make more sense. I have no preference at this point.
There was a problem hiding this comment.
Thanks for the details explanation 😄. I like your reasoning. Is adding the ipv4/ipv6 logic something you want to do with this PR? I love the idea to be honest
There was a problem hiding this comment.
No I'd rather keep the 1st one as basic as possible for now. The split ipv4/ipv6 might be a bit trickier to do because depending on the systems they might not be in the same files.
So I'd rather do a separate PR for that.
| # Starting at 0 as the last line has a line return | ||
| conntrack_count = 0 | ||
| while 1: | ||
| conntrack_buffer = conntrack_file.read(65536) |
There was a problem hiding this comment.
It just reads the file by chunks of 64k to avoid loading too much data into memory at once. Random choice. Do you have a preference in terms of buffer size?
There was a problem hiding this comment.
Ok. let's add one comment line so we know why long term :)
|
Hi @gzussa, |
3 participants
What does this PR do?
Adds the conntrack count and the conntrack table max capacity metrics in the standard network integration.
Motivation
This is something that we have been doing in custom and that was started a while back in DataDog/dd-agent#1904 by someone else but never finished.
We need those metrics in general to monitor our conntrack tables that sometimes get full and start generating issues.
Review checklist
no-changeloglabel attachedThanks for your time,
Joseph