Fix for wrapper function returning 0ms timings

[anthroprose]

DEBUG: Bad line: 0,ms in msg "production.timer:0|ms"
https://github.com/etsy/statsd/blob/5b900711191a9c5cfd536c877a3c119d1df5679a/lib/helpers.js#L35
return isNumber(fields[0]) && Number(fields[0]) > 0;

statsD expects a > 0 number for ms counters.

[jsocol]

That's interesting. I'd actually argue that's an issue with StatsD. Dropping a sub millisecond time that the application submitted changes the measured value. Unless they've specifically rejected changing that it seems like the fix belongs there.

[anthroprose]

I know this could spiral down code philosophical hell, but the official statsD implementation does in fact reject 0ms times, as well, its not theoretically possible to run something in less than the unit of time that it measures (which is actually 10ms).

I would change it in my code, but I am using the wrapper function, not the .timer() so I cannot apply logic, and is also why I did not change the actual call, only the function.

I will fix the tests and rebase.

[jsocol]

 its not theoretically possible to run something in less than the unit of time that it measures (which is actually 10ms).

I don't understand that, obviously it's possible to run something in less than a millisecond or else you never would have seen this. 

I'm saying the "official" implementation—which is only a de facto standard—is wrong. If you look at the commit that added the >0 check, it wasn't specifically supposed to do that, just to more liberally validate that the value was actually a number. 

Since the timers are averaged, dropping 0 values in a client (or the server, really) systematically skews the data. 

Sent from my phone, please excuse the occasional typo.

On Sat, Mar 8, 2014 at 7:16 PM, Alex Corley notifications@github.com
wrote:

I know this could spiral down code philosophical hell, but the official statsD implementation does in fact reject 0ms times, as well, its not theoretically possible to run something in less than the unit of time that it measures (which is actually 10ms).
I would change it in my code, but I am using the wrapper function, not the .timer() so I cannot apply logic, and is also why I did not change the actual call, only the function.

I will fix the tests and rebase.

Reply to this email directly or view it on GitHub:
#41 (comment)

[anthroprose]

I meant the original simply drops them, so this was an attempt to save network i/o, and statsd error handling, but honestly after reading through all the tests, I am tending to lean towards moving this up to statsd with a 'allowZeroTiming' option for those of us who rely on the aggregate timing functions of counting.

[jsocol]

I'm happy to open a pull req upstream as well. I'm sold that they shouldn't drop 0 times--

Sent from my phone, please excuse the occasional typo.

On Sat, Mar 8, 2014 at 7:44 PM, Alex Corley notifications@github.com
wrote:

Closed #41.

Reply to this email directly or view it on GitHub:
#41

[anthroprose]

statsd/statsd#397

Seems there were a few of us on it, I just started a bit downstream instead of from the source ;)

[jsocol]

Ah, looked at the python-statsd pull req, and the timing floor WoLpH mentioned is actually 10µs, not 10ms (just for myself/posterity).

[anthroprose]

One of the more thorough people I've encountered... many thanks for your info as well as guidance... honestly learned more digging through code and this issue than blinding running services for a while.