Skip to content
This repository has been archived by the owner on Sep 19, 2024. It is now read-only.

increment(0) == increment(1) #41

Open
chrisDeFouRire opened this issue Jun 21, 2014 · 4 comments
Open

increment(0) == increment(1) #41

chrisDeFouRire opened this issue Jun 21, 2014 · 4 comments

Comments

@chrisDeFouRire
Copy link

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!

@Dieterbe Dieterbe removed their assignment Jul 16, 2015
@jishi
Copy link

jishi commented Jan 13, 2016

+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
Copy link

todd commented Dec 13, 2016

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.

@bdeitte
Copy link

bdeitte commented Dec 13, 2016

@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
Copy link

todd commented Dec 13, 2016

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants