node-statsd icon indicating copy to clipboard operation
node-statsd copied to clipboard

increment(0) == increment(1)

Open chrisDeFouRire opened this issue 10 years ago • 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!

chrisDeFouRire avatar Jun 21 '14 13:06 chrisDeFouRire

+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.

jishi avatar Jan 13 '16 12:01 jishi

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 avatar Dec 13 '16 22:12 todd

@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: https://github.com/brightcove/hot-shots/pull/13

bdeitte avatar Dec 13 '16 23:12 bdeitte

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

todd avatar Dec 13 '16 23:12 todd