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

Add ability to limit stats in the basicstats aggregator #3413

Closed
danielnelson opened this issue Oct 31, 2017 · 11 comments
Closed

Add ability to limit stats in the basicstats aggregator #3413

danielnelson opened this issue Oct 31, 2017 · 11 comments
Labels
feature request Requests for new plugin and for new features to existing plugins

Comments

@danielnelson
Copy link
Contributor

Feature Request

Option to limit the new values to a single calculation (stdv or median or all)

Requested in #3402 by @xkilian

Proposal:

Add method to select which statistics are collected, perhaps:

[[aggregators.basicstats]]
  stats = ["stdev", "s2"]

Current behavior:

Must generate all stats.

Due to the way measurement filtering works with processors/aggregators, you cannot use the filtering option to limit the output. This is because filtering applies to the metrics that enter the plugin, but not to the output. Unlike inputs/outputs, these plugins really need two sets of filters.

Desired behavior:

A method for limiting the output.

Use case: [Why is this important (helps with prioritizing requests)]

This will make the new aggregator much more flexible for hosts that generate lots of metrics.

@JeffAshton
Copy link
Contributor

I'd be interested in implementing this functionality. I'd like to amend the proposal to allow configuration per field. For example, one field I might want Min/Max computed, and another field I might just want the mean computed.

A default configuration for non matched fields makes sense to me.

@danielnelson
Copy link
Contributor Author

Thanks for the offer @JeffAshton, the first step I would take is to design the configuration.

I'll also mention a workaround for this issue, you can use the standard measurement filtering on the output plugins to remove stats you don't want.

@JeffAshton
Copy link
Contributor

JeffAshton commented Dec 13, 2017

@danielnelson , I've sent you a PR for the first part.

I'll look at adding the field specific stuff as a separate PR. The configuration will roughly look like:

[[aggregators.basicstats]]
  stats = ["stdev", "s2"]

  [[aggregators.basicstats.field]]
    name = "cpu"
    stats = [ "min", "max" ]

  [[aggregators.basicstats.field]]
    name = "mem"
    stats = [ "mean" ]

Not a great example, since this data wouldn't be in the same measurement, but you get the idea.

@danielnelson
Copy link
Contributor Author

Maybe we can do something similar to how the histogram aggregator is configured, here is an example of how it looks:

[[aggregators.histogram]]
  [[aggregators.histogram.config]]
  #   ## The set of buckets.
  buckets = [0.0, 10.0, 20.0, 30.0, 40.0, 50.0, 60.0, 70.0, 80.0, 90.0, 100.0]
  #   ## The name of metric.
  measurement_name = "diskio"
  #   ## The concrete fields of metric
  fields = ["io_time", "read_time", "write_time"]

So fields is a list, and you can specify the measurement name (actually you must set this in the histogram aggregator) which might be useful for conflicts.

@JeffAshton
Copy link
Contributor

JeffAshton commented Dec 15, 2017

Yeah, I can get behind that, so it would look like:

[[aggregators.basicstats]]
  stats = ["stdev", "s2"]

  [[aggregators.basicstats.config]]
    fields = [ "cpu" ]
    stats = [ "min", "max" ]

  [[aggregators.basicstats.config]]
    fields = [ "reads", "writes" ]
    stats = [ "mean" ]

I feel like I want the name to represent that it's an override. Brainstorming:

  • [[aggregators.basicstats.override]]
  • [[aggregators.basicstats.field]]
  • [[aggregators.basicstats.fieldoverride]]
  • [[aggregators.basicstats.field_override]]

Config just seems too vague. It's config within config. :)

@JeffAshton
Copy link
Contributor

JeffAshton commented Dec 15, 2017

The other interesting consideration is whether it's an aggregate, or first match. For example, if I do

[[aggregators.basicstats]]
  stats = ["stdev"]

  [[aggregators.basicstats.config]]
    fields = [ "cpu" ]
    stats = [ "min", "max" ]

  [[aggregators.basicstats.config]]
    fields = [ "cpu", "other" ]
    stats = [ "mean" ]

Does cpu => [ "stdev", "min", "max", "mean" ]

  • stdev from the root
  • and the remaining from the configs, obviously not overrides anymore

@danielnelson
Copy link
Contributor Author

I guess it should probably be the union of all configs.

@JeffAshton
Copy link
Contributor

JeffAshton commented Dec 15, 2017

Yeah, I think I like that best too. It's the most expressive.

I guess the only case it doesn't handle is the wildcard case. If not one of the specified fields.

Hmmm, let me think about it some more.

@JeffAshton
Copy link
Contributor

Maybe we just solve that case with something like


  [[aggregators.basicstats.config]]
    fields = [ "???" ]
    stats = [ "mean" ]

Seems a little hacky.

@danielnelson danielnelson added the feature request Requests for new plugin and for new features to existing plugins label Mar 21, 2019
@sanchitsharma
Copy link

from documentation here it seems done [github readme of basicstats]. (https://github.com/influxdata/telegraf/tree/master/plugins/aggregators/basicstats) @danielnelson shouldn't this be closed now or I am interpreting the doc wrong.

@danielnelson
Copy link
Contributor Author

The feature described in the initial comment is done, but not the proposed per field options, though it is possible to do the same computations using multiple aggregator instances. The #5616 pr is open that provides per field controls but hasn't been reviewed yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Requests for new plugin and for new features to existing plugins
Projects
None yet
Development

No branches or pull requests

4 participants