Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add UDP metrics. #1974

Merged
merged 1 commit into from
Oct 14, 2015
Merged

Add UDP metrics. #1974

merged 1 commit into from
Oct 14, 2015

Conversation

gphat
Copy link
Contributor

@gphat gphat commented Oct 12, 2015

What's this PR do?

Adds UDP metrics from /proc/net/snmp in addition to the existing TCP metrics.

Motivation

We have some things using UDP and they are having trouble keeping up. Our most reliable indicator of this problem are dropped UDP due to full buffers. This adds that functionality to the agent so that we can see it in charts!

Notes

I basically lifted the existing TCP code and made few modifications to have different variables for TCP versus UDP. If you'd prefer this refactored some for lessening code duplication, I'm open to refactoring with guidance.

@remh
Copy link

remh commented Oct 13, 2015

Thanks @gphat we'll review this soon

@remh remh added this to the 5.6.0 milestone Oct 13, 2015
hkaj
hkaj reviewed Oct 14, 2015
View reviewed changes
checks.d/network.py
@@ -239,6 +244,20 @@ def _check_linux(self, instance):

for key, metric in tcp_metrics_name.iteritems():
self.rate(metric, self._parse_value(tcp_metrics[key]))

asert(udp_metrics['Udp:'] == 'Udp:')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small typo, asert --> assert

@hkaj
Copy link
Member

hkaj commented Oct 14, 2015

Hi @gphat, thanks for the contribution! A few typos to fix, but apart from that it looks good to me.
Once you will have fixed them we'll merge, and it will go out with 5.6 🎉

@hkaj hkaj self-assigned this Oct 14, 2015
@hkaj hkaj added the checks label Oct 14, 2015
@gphat
Copy link
Contributor Author

gphat commented Oct 14, 2015

Thanks @hkaj! Gonna do some testing here to verify things work before I say this is ready again. Sorry for not testing this.

@gphat
Copy link
Contributor Author

gphat commented Oct 14, 2015

Ok, I verified this works as expected locally. Sorry again, @hkaj!

@hkaj
Copy link
Member

hkaj commented Oct 14, 2015

No worries. Thanks for the contribution, UDP metrics will join their TCP counterpart in 5.6.0! 🎊

hkaj added a commit that referenced this pull request Oct 14, 2015
@hkaj
Merge pull request #1974 from gphat/add-udp-metrics
5290f99 
Add UDP metrics.
@hkaj hkaj merged commit 5290f99 into DataDog:master Oct 14, 2015
@gphat
Copy link
Contributor Author

gphat commented Oct 14, 2015

🎉 Yeah @alq666 I guess I type "stripe" a lot. 😅

@olivielpeau
Copy link
Member

FYI, the RcvbufErrors and SndbufErrors metrics do not exist on CentOS 5, which makes the network check fail on that OS (for reference see https://www.mail-archive.com/[email protected]/msg03802.html).

A simple test on the existence of the metrics here could be enough to fix this.

I won't be super available for the next couple of days, could you have a further look at this @hkaj? :)

@gphat
Copy link
Contributor Author

gphat commented Oct 19, 2015

Added a test, good catch!

@hkaj hkaj mentioned this pull request Oct 20, 2015
Merged
@hkaj
Copy link
Member

hkaj commented Oct 20, 2015

Good catch indeed! I checked on Ubuntu and CentOS 7 but didn't test on them oldies. #1986 should fix that.

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

@hkaj hkaj

Labels
checks
Projects
None yet
Milestone
5.6.0
Development

Successfully merging this pull request may close these issues.
6 participants
@gphat @remh @hkaj @olivielpeau @alq666 @cory-stripe