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

Support Processor & Aggregator Plugins #1777

Merged
merged 13 commits into from
Oct 12, 2016
Merged

Support Processor & Aggregator Plugins #1777

merged 13 commits into from
Oct 12, 2016

Conversation

sparrc
Copy link
Contributor

@sparrc sparrc commented Sep 16, 2016

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)

TODO:

  • Cleanup TODOs
  • Handle name_override, name_suffix, name_prefix in running aggregator.
  • Handle adding adhoc tags in running aggregator.
  • drop_original arg: Provide a way of dropping the original metric after it gets sent to the aggregator(s)
  • Determine how to handle adding aggregate metrics that are getting gathered at the same time as the aggregator flushing. Maybe introduce a configurable delay before each ticker fires.
  • Print processors and aggregators in the telegraf auto-generated config file
  • support for --processor-filter and --aggregator-filter command-line args.
  • Document contributing guidelines for processor and aggregator plugins.
  • Document configuration of processor and aggregator plugins.
  • Support aggregator plugin period argument. Flush the aggregator period outside of the plugin itself.

closes #1726

@sparrc sparrc force-pushed the cs1726 branch 13 times, most recently from 788db33 to eb04366 Compare September 20, 2016 15:14
@sparrc sparrc mentioned this pull request Sep 21, 2016
3 tasks
@sparrc sparrc force-pushed the cs1726 branch 6 times, most recently from f843520 to 2b4cac5 Compare September 28, 2016 10:39
@sparrc sparrc force-pushed the cs1726 branch 9 times, most recently from 42a5ea5 to a809288 Compare October 5, 2016 13:21
}
return
case m := <-r.metrics:
if m.Time().Before(r.periodStart) || m.Time().After(r.periodEnd) {
Copy link
Contributor

@dgnorton dgnorton Oct 12, 2016

Choose a reason for hiding this comment

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

This drops metrics at the beginning of each period.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you look at the <-periodT.C case I tried to make it so that each interval is overlapping by the "delay" amount. So for example:

:00 <- 1st period starts
:10 <- 2nd period starts
:10 + delay <- 1st period ends
:20 <- 3rd period starts
:20 + delay <- 2nd period ends

Copy link
Contributor

Choose a reason for hiding this comment

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

But once a metric is read from the channel and dropped, it won't be available on the channel during the next period. Seems like points where end < t < end+delay would have to either go into a buffer or back into the metric chan.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

end is defined as: start + period + delay (see 735a564#diff-65518ad6bfcfeef60e9881df019b20a8R116)

so a metric where end < t < end+delay would always be accepted.

Maybe it's not very clear the way I've done it. Instead of defining end to include the delay, I could make this if-statement include the delay.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see. I missed something in the code earlier. I thought the ticker was firing on period + delay intervals.

@dgnorton
Copy link
Contributor

👍 with changelog update

@sparrc sparrc merged commit b00ad65 into master Oct 12, 2016
@sparrc sparrc deleted the cs1726 branch October 12, 2016 16:19
@johnrengelman
Copy link
Contributor

@sparrc I didn't want to open a new issue for this cause it's just a simple question. If a processor wants to add a Tag to a metric, does it need to copy the whole metric? It seems like Metric.Tags() returns a copy of the map, so modifying it has no affect.

@sparrc
Copy link
Contributor Author

sparrc commented Nov 14, 2016

I think you'd need to, although it would probably be more efficient to add a passthru function to the underlying influxdb points object, which I believe has a some sort of AddTags method: https://github.com/influxdata/influxdb/blob/master/models/points.go

@johnrengelman
Copy link
Contributor

Yup. That's what I ended up doing in my PR. Thanks.
On Mon, Nov 14, 2016 at 5:45 AM Cameron Sparr notifications@github.com
wrote:

I think you'd need to, although it would probably be more efficient to add
a passthru function to the underlying influxdb points object, which I
believe has a some sort of AddTags method:
https://github.com/influxdata/influxdb/blob/master/models/points.go


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#1777 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA90JDZHq2dsU74l4gYw-d0gWpEY_B7iks5q-EnOgaJpZM4J-3k0
.

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.

Processor & Aggregator Plugin Support
3 participants