raintank / statsdaemon

Metrics aggregation daemon like statsd, in Go and with a bunch of extra features.
The Unlicense
327 stars 30 forks source link

feat(counters): Adds `.count` metric for counters #18

Closed torkelo closed 8 years ago

torkelo commented 8 years ago

This PR adds support for regular statsd counters that are aggregated and sent to graphite using a .count suffix .

Why? Currently vimeo/statsdaemon only sends rates for counters to a rates prefix. Missing the standard .count (sum for flush interval) metric. The count metric is very important for many types of metrics where accurate aggregation across rollup ranges is important. For example using summarize to group in to 1 hour buckets or 1 day buckets, using rates this is not possible (unless you apply a scale factor, but that will only work for one rollup region). Also rates will be aggregated in graphite using averages which is not very good if you care about accurate totals.

In many ways the .count metric is more useful as the rate can always reliably be calculated from it using graphite's scaleToSeconds function.

Implementation: In order not to change things for current users of vimeo/statsdaemon new options have been added.

Example: So if send_counters set to true and prefix_counters set to "stats.counters."

StatD Input logins:1|c\nlogins:2|c\nlogins:3|c

Will send to graphite stats.counters.logins.count 6.000000 <epoch>

If rate is also enabled (if flush interval is 10s): stats.rates.logins 0.600000 <epoch>

Open to any suggested changes to this PR.

Dieterbe commented 8 years ago

looks good. my main issue is that this brings the default configuration somewhere half between legacyNameSpace and half into modernized namespace. (see https://github.com/etsy/statsd/blob/master/docs/namespacing.md) and the readme still claims to honor legacynamespace

with this PR:

so here's what i suggest:

we can then in readme clarify that we ship with default settings drop-in compatible with etsy's statsd with traditional/ default settings (legacyNameSpace true), but also provide the recommended configuration, and that is:

legacyNameSpace = false
prefix_counters = stats.counters. # will get .count added automatically
prefix_rates = stats.counters. # will get .rate added automatically

and at raintank we can then use that config as well.

this way we can make it work for everyone without breaking compatibility.

Dieterbe commented 8 years ago

hmm https://github.com/metrics20/go-metrics20/blob/master/metrics2.go has hardcoded the behavior where counts get .count but rates get no suffix. we could add a legacy conditional there to accomodate both legacy on non-legacy statsd.

However since the metrics2.0 library aims to be modern, it could also just assume modern statsd behavior for non metrics20 metrics and always add those suffixes. if we go this route, it gives us an excuse to not support legacyNameSpace in statsdaemon (in which case we should set default prefix_rates to stats.counters.)

this will break people's setups when they upgrade, but maybe it's worth it. a lot of people are still stuck with the old namespace, but maybe it's time to break free. especially if we make it clear in the readme.

torkelo commented 8 years ago

@Dieterbe good feedback, removed the send_rates and send_counters option and replaced it with a legacyNamespace option. Now everything is very much aligned with etsy statsd.

Added unit test so both legacy and non legacy options are tested. Also did manual tests for both options. Added commented out "recommended" config to sample ini file and readme.