Add Tdigest Aggregation Plugin by PhoenixRion · Pull Request #6887 · influxdata/telegraf

PhoenixRion · 2020-01-09T22:23:35Z

Required for all PRs:

Signed CLA.
Associated README.md updated.
Has appropriate unit tests.

danielnelson

Here an initial review. Ultimately my goal is to shrink down the code as much as we can, as this goes a long way on understanding and maintaining the plugin.

danielnelson · 2020-01-22T19:11:28Z

.gitignore

 /telegraf.exe
 /telegraf.gz
 /vendor
+.idea/


Revert this file, place in your global core.excludesfile https://git-scm.com/docs/gitignore/2.22.1

danielnelson · 2020-01-22T19:12:34Z

Gopkg.toml

  name = "github.com/satori/go.uuid"
  revision = "b2ce2384e17bbe0c6d34077efa39dbab3e09123b"
+
+[[constraint]]


We updated master to use Go modules after this PR was opened, kind of surprised this didn't trigger a conflict.

I am not sure what I would need to change because of this

We will need to either merge or rebase against Telegraf master branch, then these files will be deleted. To add the dependency you just need to run:

go get github.com/influxdata/tdigest@v0.0.1

This should add it to the new go.mod/go.sum files.

danielnelson · 2020-01-22T19:14:00Z

plugins/aggregators/tdigestagg/README.md

+
+```
+# Keep the histogram of each metric passing through.
+[[aggregators.tdigestagg]]


Rename to tdigest, move files to directory with same name.

This what not initially possible because it conflicted with the tdigest library but that is not longer an issue

You can also adjust an imports name if needed:

import ( foo github.com/influxdata/tdigest ) var tdigest = foo.TDigest{}

danielnelson · 2020-01-22T19:15:54Z

plugins/aggregators/tdigestagg/aggregation.go

@@ -0,0 +1,172 @@
+//Copyright (c) 2019, Groupon, Inc.


Remove per file copyright notices

I am checking with Legal as the text per file is a current guideline.

danielnelson · 2020-01-22T19:39:59Z

plugins/aggregators/tdigestagg/bucketing/bucketConfig.go

+	ExcludeTags           []string `toml:"exclude_tags"`
+	SourceTagKey          string   `toml:"source_tag_key"`
+	AtomReplacementTagKey string   `toml:"atom_replacement_tag_key"`
+}


Move the contents of this file into tdigest.go

Why would I move this out of the bucketing package when it is only referenced within it?

Isn't this referenced from the main plugin struct?

danielnelson · 2020-01-22T21:55:02Z

plugins/aggregators/tdigestagg/README.md

+emitting the aggregations and the histogram every `period` seconds.
+
+### Aggregation Concepts
+* Tag key ```source``` has special meaning as the primary key of developers destination TSDB


The measurement aka metric name should be this.

Source is very different from measurement

It seems the some of the logic with source and atom is specific to your deployment. For inclusion into the main Telegraf tree I suggest we move these out of this plugin and into separate processor(s) than can check for tags and enforce rules as needed. If the current processors can't handle your case, then we can add new ones to complete the needed functionality.

I'm not exactly sure how you are using the source tag, but keep in mind that it is a somewhat special tag that Telegraf often uses to report the hostname of the system the metrics describe.

danielnelson · 2020-01-22T21:56:19Z

plugins/aggregators/tdigestagg/README.md

+
+### Aggregation Concepts
+* Tag key ```source``` has special meaning as the primary key of developers destination TSDB
+* Tag key ```atom``` has special meaning to denote the atomic dimension of an aggregation


What is this used for?

to denote the atomic dimension of an aggregation

Is this the units (meters, bytes, etc), the aggregation type (min, max, etc) or something entirely different? Can you add some examples?

danielnelson · 2020-01-22T22:00:01Z

plugins/aggregators/tdigestagg/bucketing/bucketConfig.go

@@ -0,0 +1,37 @@
+//Copyright (c) 2019, Groupon, Inc.


On all files, rename to use snake_case.go

danielnelson · 2020-01-22T22:07:25Z

plugins/aggregators/tdigestagg/aggregation.go

+	_time      time.Time
+}
+
+type ClamAggregation struct {


If we are going to keep this format in, I'd like to do two things:

Build everything as a single main type, and then do a conversion to you CLAM format at the last moment.

Remove documentation of this format.

If you don't think this will be possible, then we should remove the format. There isn't any way we will be able to maintain the format if it unless it is very simple and self contained.

danielnelson · 2020-01-22T22:11:16Z

plugins/aggregators/tdigestagg/aggregation.go

+	_histogram *tdigest.TDigest
+	_tags      map[string]string
+	_basicName string
+	_sum       float64
+	_time      time.Time


This isn't very idiomatic for Go, name these without the leading underscore and export the function names in the interface.

alexrocco · 2020-07-20T06:46:36Z

Any news about this PR? IMO this will be very useful for the project, especially for on-line percentiles.

sjwang90 · 2021-01-28T19:44:42Z

@PhoenixRion Thanks for the PR but closing this in preference for #8594 with less code. If there's anything that this PR contains that is missing from #8594 feel free to comment.

Ryan Ascheman and others added 18 commits September 23, 2019 16:44

Initial public change

6dacad1

running gofmt to pass make check

abb2c9d

add plugin to all.go

ff44680

use influxdb/tdigest, update output format, grpn features added

4a52ccb

Merge remote-tracking branch 'upstream/master'

c797fbe

gofmt

632818b

Update machine.go

80f6c90

probably ready for pr

906f9eb

Merge branch 'master' of github.com:PhoenixRion/telegraf

78f7af8

add tdigest to gopkg

d5edcc7

Merge branch 'master' into master

1145a82

fixing typo

74f1e72

exclude intellij folder from git

b62604e

Merge branch 'master' of github.com:PhoenixRion/telegraf

c0d4b76

run dep ensure

3fd71a7

Merge remote-tracking branch 'upstream/master'

8059114

run dep ensure

0a17044

run gofmt -w plugins/aggregators/tdigestagg

8340647

danielnelson suggested changes Jan 22, 2020

View reviewed changes

danielnelson mentioned this pull request Feb 11, 2020

Aggregator for Percentiles #6999

Closed

danielnelson added the new plugin label Feb 28, 2020

reimda mentioned this pull request Dec 8, 2020

Fix #7344: Add percentiles to the ping plugin #7345

Merged

3 tasks

srebhan mentioned this pull request Dec 18, 2020

Add quantile aggregator plugin #8594

Merged

3 tasks

sjwang90 added the plugin/processor label Jan 22, 2021

sjwang90 closed this Jan 28, 2021

Conversation

PhoenixRion commented Jan 9, 2020 • edited by danielnelson Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Required for all PRs:

Uh oh!

danielnelson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexrocco commented Jul 20, 2020

Uh oh!

sjwang90 commented Jan 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

PhoenixRion commented Jan 9, 2020 •

edited by danielnelson

Loading