Allow multiple metrics per packet #93

Merged
merged 2 commits into from Jun 25, 2012

Projects

None yet

5 participants

@PSUdaemon

This just adds a new loop that splits first on "\n". The patch looks bigger than it is because of white space chagnes.

@travisbot

This pull request passes (merged 04e397f5 into 087fde9).

@mheffner
mheffner commented Jun 4, 2012

The whitespace changes should really be done separately, it's hard to tell what the functional changes are.

@mheffner
mheffner commented Jun 4, 2012

Forgot to add: I do like the idea of supporting multiple metrics per payload and have considered submitting a similar change myself in the past.

@PSUdaemon

I've reverted then separated the patch into a functional piece and a white space piece.

@travisbot

This pull request passes (merged 01bfd03 into 087fde9).

@mrtazz
Member
mrtazz commented Jun 7, 2012

This is really great! I will test it some more and merge it then. It would be awesome if you could also take a stab at writing some unit tests for this.

@Dieterbe

people that want to implement this in clients need to be careful to never construct udp packets larger than 512B. see http://stackoverflow.com/questions/1098897/what-is-the-largest-safe-udp-packet-size-on-the-internet

seems like a good idea to add a README change too..

@PSUdaemon

I looked at writing some tests for this, but the only existing test script that I could find was for graphite. I didn't feel that I could adequately write a completely new test script for stats.js proper. If I am misunderstanding what you are requesting let me know. I am happy to do whatever you need to get this accepted.

Also, RE the README change, is that a general comment or a request for me to supply a patch for that as well?

@mrtazz
Member
mrtazz commented Jun 12, 2012

The tests are pretty graphite centric, which we should improve at some point. Thanks for looking into it, it's not a blocker for getting this merged. If you can update the README, that would be great. If not, I'll add the notice into the README when merging the PR.

@mrtazz mrtazz merged commit 01bfd03 into etsy:master Jun 25, 2012
@mrtazz
Member
mrtazz commented Jun 25, 2012

Finally found time to test it, sorry for the long wait.

@Dieterbe
Dieterbe commented Aug 7, 2012

seems like there's still nothing in the readme about the udp packet descriptions, which affects this feature.

@PSUdaemon

Dieterbe,

I was reluctant to add something because I didn't feel knowledgeable enough on the subject. Since then I have researched this some more and it's a rather complicated topic. It seems to me that what you are concerned about is the MTU. Even the article you linked goes into how clouded this issue is. It might be nice to mention that there are limits to the size, but what they are can vary greatly. Most internal networks where I assume statsd would be implemented are going to have an MTU of 1500 which gives you a minimum of 1432 bytes of payload (68 byte max ip + udp headers). If you are on a gige network with jumbo frame support (9000 MTU), you can do 8932 bytes. Going out over the routed internet adds tighter limits, and I think there something in the 512 byte area might be more prudent.

Given all that, I think a warning might be a good idea, maybe with suggestions for the three scenario's I mentioned. But I don't think claiming that they should never be over 512 bytes is a good idea. That would limit the benefit of this feature.

Thoughts?

@Dieterbe

that makes perfect sense indeed.

@PSUdaemon

Sent a new pull request with my proposed change #225.

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