sivy/node-statsd

increment(0) == increment(1)

chrisDeFouRire opened this issue · 4 comments

I know I shouldn't have called increment(0) but let me tell you it took a long time to find that bug !

Because increment (and decrement) compare delta to 0/null and not to undefined

StatsDClient.prototype.increment = function (name, delta) {
    this.counter(name, Math.abs(delta || 1));
};

could become :

StatsDClient.prototype.increment = function (name, delta) {
    this.counter(name, Math.abs(delta!=undefined?delta: 1));
};

or

StatsDClient.prototype.increment = function (name, delta) {
    if (delta==0) then return; // no op
    this.counter(name, Math.abs(delta || 1));
};

Why was it hard to figure it out:

  • my code could have been the problem (triple checked that)
  • custom statsd -> influxdb connector (which I thought was faulty)
  • influxdb could have been the problem too (checked that)
  • statsd could have been the problem (checked that too)

I think you could document that special case... or allow delta == 0

Many thanks for all your hard work anyway!

jishi commented

+1 on this one, we doubled and trippel-checked our implementation until we looked at the source and realized that it did value || 1and defaulted 0 into 1. It's kind of nice to not require you to write programming logic to avoid sending stats if you always have a counter that spans between 0 and infinity.

todd commented

I just opened #65, but I'm not certain on whether it'll get merged or not considering the staleness of this repo. You guys can feel free to point to my fork if you want.

@todd No updates have been made here for quite awhile, so I've been keeping hot-shots up to date as an alternative to this. It included a fix for this issue awhile ago: brightcove/hot-shots#13

todd commented

@bdeitte Thanks for the heads up. I'll give your package a whirl.