-
Notifications
You must be signed in to change notification settings - Fork 5.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
StatsD plugin: Support multiple fields for timers #737
Conversation
Thanks @mattheath for the detailed comment and for writing unit tests 👍 I only have one possible change to make to this: maybe we don't need to provide the ie, stats, ok := metric.fields[defaultFieldName]
if !ok {
// add field name prefix to stats
} else {
// no field name prefix to stats
} that way if users are specifying fields in their template, then the statsd input will automatically just use them. |
Yep, that approach works perfectly for us, my only worry would be it breaking backwards compatibility for current users? |
I think in this case it's OK...seems odd that someone would specify a |
Agreed, I think it would be better behaviour. I am still a little worried that this will completely change the data model for some existing users though? If we were using this set up already we'd have to change all of our grafana dashboards when the new version was deployed, and would have to either have both sets of series drawn on the same graph for the switchover, or switch between two sets of graphs to look at historical data? Happy to change this though if that's ok 😄 |
But there's a simple workaround, isn't there? Just remove Since we are not at a 1.0 release version yet, it's generally been treated as OK to break backwards compatability (as long as we call it out in the changelog) |
Great, I'll get that changed and update the tests 😄 |
4a63c37
to
7d4abd8
Compare
7d4abd8
to
0e39265
Compare
Simplified this a bit, and rebased to squash the commits on top of master. |
Currently the statsd plugin supports multiple fields per measurement for counters, but not timers as the statsd plugin uses fields in InfluxDB to record the various stats (upper, mean, percentiles etc). The current behaviour is to append the field name (if parsed via a template) to the measurement name, which in our use case clutters the measurement namespace.
This PR changes the behaviour with an optional config setting of
multiple_timer_fields
(disabled by default) to allow multiple fields per measurement to be recorded on timers by prepending the stat name with the field name.As an example, in the case of counters we can record success and error counters as fields within the same measurement. So a measurement of
api_request
and a template such asmeasurement.field
allows us to increment counters by sending statsd values such asapi_request.success:1|c
andapi_request.error:1|c
which results in a measurement in influxDB of:In contrast, timings currently create two measurements, with the various calculated stats:
In this case the number of fields is comparatively small, and we'd like to combine both of these into the same measurement, both tidying up our measurement namespace, and reducing the series cardinality in InfluxDB. This results in a single measurement as below:
Would really love feedback and suggestions for improvements!