search

segmentio / stats

Go package for abstracting stats collection
https://godoc.org/github.com/segmentio/stats
MIT License
208 stars 32 forks source link

engine: add .IncrBy #26

Closed yields closed 7 years ago

achille-roussel commented 7 years ago

Is it really a pain to do eng.Add("name", float64(value)) ?

yields commented 7 years ago

Is it really a pain to do eng.Add("name", float64(value)) ?

yes

achille-roussel commented 7 years ago

Add + float64 + ( + ) = 12 characters IncrBy = 6 characters

Typing 6 characters is too many :P

What if your variable is of type int64, should we all have something called IncrByInt64? (and follow up questions with all other numeric types). I also feel like for consistency purposes we should have similar methods on Gauge metrics, add top-level helpers, and have a DecrBy version as well.

yields commented 7 years ago

What if your variable is of type int64, should we all have something called IncrByInt64? (and follow up questions with all other numeric types). I also feel like for consistency purposes we should have similar methods on Gauge metrics, add top-level helpers, and have a DecrBy version as well.

You have a good point, we've seen this in statsd/datadog package, i think that 99% of the time you'll just need an int right?

I think int64 is an edge case, and for that users can just use .Add(), i think having float64(n) all around is a bit ugly, or even worse, having to write a wrapper with IncrBy :(

For me it's not about the N of characters, its just about how the code looks, i think since 99% of the time you have an int, it makes sense to add IncrBy(name, int).

achille-roussel commented 7 years ago

From my experience the 99% cases are:

I've ran into a couple of situations where I needed to cast but I've never found this to be impacting readability enough that it justified the extra API, but that's just my opinion.

One of the reasons I didn't want to add those is because most of the times when I had to add a cast was on an Observe call (measuring the sizes of request payloads for example), and it wasn't clear to me what the method should be named.

yields commented 7 years ago

Might just be me, but Incr() without IncrBy() feels a bit odd, when i batch things it is useful to use IncrBy(), it is also useful for async error counts and more, i personally use it heavily.

I agree though that since we have .Add(name, float64), adding IncrBy() seems confusing -- so going to close this ;)