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

Added new basicstats aggregator #2167

Merged

Conversation

toni-moreno
Copy link
Contributor

Required for all PRs:

  • CHANGELOG.md updated (we recommend not updating this until the PR has been approved by a maintainer)
  • Sign CLA (if not already signed)
  • README.md updated (if adding a new plugin)

Description

BasicStats Aggregator Plugin

The BasicStats aggregator plugin give us count,max,min,mean,s2(variance), stdev for a set of values,
emitting the aggregate every period seconds.

Configuration:

# Keep the aggregate basicstats of each metric passing through.
[[aggregators.basicstats]]
  ## General Aggregator Arguments:
  ## The period on which to flush & clear the aggregator.
  period = "30s"
  ## If true, the original metric will be dropped by the
  ## aggregator and will not get sent to the output plugins.
  drop_original = false

Measurements & Fields:

  • measurement1
    • field1_count
    • field1_max
    • field1_min
    • field1_mean
    • field1_s2 (variance)
    • field1_stdev (standar deviation)

Tags:

No tags are applied by this aggregator.

Example Output:

$ telegraf --config telegraf.conf --quiet
system,host=tars load1=1 1475583980000000000
system,host=tars load1=1 1475583990000000000
system,host=tars load1_count=2,load1_max=1,load1_min=1,load1_mean=1,load1_s2=0,load1_stdev=0 1475584010000000000
system,host=tars load1=1 1475584020000000000
system,host=tars load1=3 1475584030000000000
system,host=tars load1_count=2,load1_max=3,load1_min=1,load1_mean=2,load1_s2=2,load1_stdev=1.414162 1475584010000000000

@sparrc
Copy link
Contributor

sparrc commented Dec 15, 2016

Looks good @toni-moreno, but for the stats I'd prefer if you used https://github.com/aaw/histosketch, which can also make approximate quantiles (and thus an approximate median). The algorithm used in that library is more or less the standard for creating these stats.

@sparrc
Copy link
Contributor

sparrc commented Dec 15, 2016

there's also this: https://github.com/VividCortex/gohistogram

@sparrc sparrc added this to the 1.3.0 milestone Dec 15, 2016
@toni-moreno
Copy link
Contributor Author

Hi @sparrc I've written this plugin as a way to get "basic" stats ( indeed I only need count max , min and averages) I suggest to add this plugin as a way to get count/mean/stdev values as fast as possible.

Is a good idea get more detailed statistic info but perhaps we could create a new plugin "fullstats" based in https://github.com/aaw/histosketch to get also quantiles and histograms.

Can I maintain the basicstats plugin as it is now? Do you need any change?

@sparrc
Copy link
Contributor

sparrc commented Dec 16, 2016

@toni-moreno I need to think about that one.

My main concern is that there is a significant performance overhead of each aggregator plugin which needs to individually filter and inspect each field of each metric that passes through it.

for that reason, it would be much more efficient to have a single plugin which had the capability to aggregate all of these statistics (albeit possibly with a small memory hit).

@toni-moreno
Copy link
Contributor Author

Hi @sparrc.

About performance , I agree on that point.

But IMHO if you need only basic stats , count and mean by example ( for each field) , the "fullstats" plugin with quantiles could have a lot of overhead. User perhaps would prefer switch from a fullstats to a basicstats plugin with less cpu overhead.

About the plugin behaviour itself .

I'm working on add a flag "allow_send_zero_counters" , to send on each push the XXX_count=0 even if not added metrics for each measurement/tags values.

Sometimes we need counter metrics even when the value is 0 . (for graphite output by example)

If allow_send_zero_counters = true , the basicstats plugin will send zeroes only for registered measurement/tags pairs (and only for XXXX_count field)

# Keep the aggregate basicstats of each metric passing through.
[[aggregators.basicstats]]
  ## General Aggregator Arguments:
  ## The period on which to flush & clear the aggregator.
  period = "30s"
  ## If true, the original metric will be dropped by the
  ## aggregator and will not get sent to the output plugins.
  drop_original = false
  ## if true  zeroes will be sent for all registered measurement/tags pairs (and only for XXXX_count field) 
  allow_send_zero_counters = true

Are you agree to set default value as true for this new flag?

@sparrc
Copy link
Contributor

sparrc commented Dec 16, 2016

@toni-moreno please open a separate issue about that where we can discuss further

### Configuration:

```toml
# Keep the aggregate min/max of each metric passing through.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doc line needs to be updated

}

func (m *BasicStats) Description() string {
return "Keep the aggregate min/max of each metric passing through."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs to be updated

@toni-moreno
Copy link
Contributor Author

Hi @sparrc sorry I've seen now your request.
I'll do ASAP.

@danielnelson
Copy link
Contributor

@toni-moreno I'd love to get this into version 1.4, is there any more work that needs done before the next review?

@toni-moreno
Copy link
Contributor Author

Hi @danielnelson 2 minor fixes where requested by @sparrc. But you can fix yourself if needed, I don't know when I could spend some time to do it. When are you planning to release 1.4 version?

@danielnelson
Copy link
Contributor

I am aiming to do the release towards the end of August or early September. (Initially the plan was to do it this month but there is not enough work completed and I think it will give us release fatigue to pace this quickly)

If I can find the time I'll try to finish it up, but I'll post here again before starting any work.

@danielnelson danielnelson modified the milestones: 1.4.0, 1.5.0 Aug 14, 2017
@danielnelson danielnelson added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Aug 24, 2017
@lexmag
Copy link
Contributor

lexmag commented Oct 6, 2017

I'm wondering about the name of this aggregator. Should not it be named basic_stats? Typically it is the way how plugins are named, right?

@toni-moreno toni-moreno force-pushed the added_new_basicstats_aggregator branch from 0f96138 to b7e38b4 Compare October 7, 2017 06:40
@toni-moreno toni-moreno force-pushed the added_new_basicstats_aggregator branch from 5e1c6a3 to 0ce8426 Compare October 7, 2017 07:01
@toni-moreno
Copy link
Contributor Author

Hi @danielnelson I've finally updated the plugin and also rebased !! I hope you can merge ASAP to enjoy it on next releases!. Thank you very much

Copy link
Contributor

@danielnelson danielnelson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, all I could see were a couple typos.

- field1_min
- field1_mean
- field1_s2 (variance)
- field1_stdev (standar deviation)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo in standard

min float64
max float64
mean float64
M2 float64 //intermemedia value for variance/stdev
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

max float64
mean float64
M2 float64 //intermemedia value for variance/stdev
//stdev float64
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove

@toni-moreno
Copy link
Contributor Author

@danielnelson fixed!

@danielnelson
Copy link
Contributor

Thanks @toni-moreno, this will be in 1.5!

@lexmag
Copy link
Contributor

lexmag commented Oct 10, 2017

Hi @danielnelson, any thoughts on my question regarding naming? :bowtie:

@danielnelson
Copy link
Contributor

I think it's okay, we have other plugins like procstat, filestat, sqlserver, and hddtemp.

maxunt pushed a commit that referenced this pull request Jun 26, 2018
@ushuz
Copy link
Contributor

ushuz commented Mar 19, 2019

RunningStats model from statsd input could be reused for this aggregator, which provides percentiles.
https://github.com/influxdata/telegraf/blob/master/plugins/inputs/statsd/running_stats.go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin new plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants