Advanced networking metrics #2720
Conversation
|
Thanks a lot for this effort @joewilliams, looks very promising. We may have some similar PR contribution in the backlog so we might try to consolidate the two. I'll keep you posted. Thank you so much for your contribution! |
|
I merged master and it looks like there were some tests added, I'll fix those up soon. |
This reverts commit 488ec7d.
|
|
||
| for key, metric in tcpext_metrics_name.iteritems(): | ||
| if key in tcpext_metrics: | ||
| self.rate(metric, self._parse_value(tcpext_metrics[key])) |
There was a problem hiding this comment.
Wouldn't a monotonic_count be a more accurate representation of these values? IMO, they should show up as counters within DataDog.
See #2630
|
👍 ✨ we're doing something similar at Remind (except, using monotonic_counters instead of rates). It'd be great to get this (or something along the lines of this) in master. |
There was a problem hiding this comment.
Hey @joewilliams, I've looked through this. This is a huge overhaul of the network metrics. I have a few requests for you.
First, while I think you're right about the TCP states changes, I would request that you put them behind a flag in the config file. (This is the same request I made in my review of #2856.)
I also would ask that you fix the network tests that this PR affects.
Also, as I asked in #2856, do you think that this PR would be superseded by the other PR?
Other than that I think this is probably good to go. Let me know when you've fixed the tests and I think it can probably be merged pretty easily.
Note: Please remember to review the Datadog Contribution Guidelines
if you have not yet done so.
What does this PR do?
This changes and extends the network metrics that the dd-agent collects.
Motivation
At GitHub we need more specific and low level network metrics collected.
Testing Guidelines
An overview on testing
is available in our contribution guidelines.
Additional Notes
It's worth noting that this changes behavior of how the dd-agent counts TCP connection states. It splits them up into their original form rather than opening, closing, established. I believe the rest of this PR simply adds metrics without modifying behavior.