Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

mattheath
Copy link
Contributor

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 as measurement.field allows us to increment counters by sending statsd values such as api_request.success:1|c and api_request.error:1|c which results in a measurement in influxDB of:

select * from api_request;

|         time         | success | error |    host     | metric_type | other_fields...
|----------------------|---------|-------|-------------|-------------|-----------------
| 2016-02-22T15:31:20Z |      10 |     1 | example.com | counter     |

In contrast, timings currently create two measurements, with the various calculated stats:

select * from api_request_success;

|         time         | count | lower | upper | mean | 90_percentile | stddev |
|----------------------|-------|-------|-------|------|---------------|--------|
| 2016-02-22T15:31:20Z |     5 |     1 |    11 |    3 |            11 |      4 |

select * from api_request_error;

|         time         | count | lower | upper | mean | 90_percentile | stddev |
|----------------------|-------|-------|-------|------|---------------|--------|
| 2016-02-22T15:31:20Z |     5 |     2 |    22 |    6 |            22 |      8 |

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:

select * from api_request;

|         time         | error_count | error_lower | error_upper | error_mean | error_90_percentile | error_stddev | success_count | success_lower | success_upper | success_mean | success_90_percentile | success_stddev |
|----------------------|-------------|-------------|-------------|------------|---------------------|--------------|---------------|---------------|---------------|--------------|-----------------------|----------------|
| 2016-02-22T15:31:20Z |           5 |           2 |          22 |          6 |                  22 |            8 |             5 |             1 |            11 |            3 |                    11 |              4 |

Would really love feedback and suggestions for improvements!

@sparrc
Copy link
Contributor

sparrc commented Feb 22, 2016

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 multiple_timer_fields = false option at all. Instead, could you add the prefix to the field names iff the field name is not value,

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.

@mattheath
Copy link
Contributor Author

Yep, that approach works perfectly for us, my only worry would be it breaking backwards compatibility for current users?

@sparrc
Copy link
Contributor

sparrc commented Feb 22, 2016

I think in this case it's OK...seems odd that someone would specify a field template on a timing measurement and then expect it not to be applied.

@mattheath
Copy link
Contributor Author

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 😄

@sparrc
Copy link
Contributor

sparrc commented Feb 22, 2016

But there's a simple workaround, isn't there? Just remove field from your timing template (since you weren't using it anyways)

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)

@mattheath
Copy link
Contributor Author

Great, I'll get that changed and update the tests 😄

@mattheath mattheath force-pushed the timerfields branch 2 times, most recently from 4a63c37 to 7d4abd8 Compare February 22, 2016 23:31
@mattheath
Copy link
Contributor Author

Simplified this a bit, and rebased to squash the commits on top of master.

@sparrc sparrc closed this in e983d35 Feb 23, 2016
geodimm pushed a commit to miketonks/telegraf that referenced this pull request Mar 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants