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 StatisticValues in cloudwatch output plugin (#4318) #4364

Merged
merged 5 commits into from
Aug 1, 2018

Conversation

david7482
Copy link
Contributor

Fix #4318

Required for all PRs:

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

Add enable_statistic_values flag. If true, this plugin would try to parse required statistic fields and send StatisticSet to Cloudwatch.

@danielnelson danielnelson added feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin area/aws AWS plugins including cloudwatch, ecs, kinesis labels Jul 7, 2018
@danielnelson danielnelson added this to the 1.8.0 milestone Jul 7, 2018
}

// Try to parse statisticSet first, then build statistic/value datum accordingly.
set, ok := getStatisticSet(point)
Copy link
Contributor

Choose a reason for hiding this comment

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

What about if the telegraf.Metric contains multiple aggregated field sets like this (leaving out some required aggregations):

cpu foo_min=1,foo_max=2,bar_min=1,bar_max=2

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 getStatisticSet() could not get all required fields, it just keeps the original behavior and sends them as independent metrics. It would generate 4 metrics to cloudwatch: cpu_foo_min, cpu_foo_max, cpu_bar_min and cpu_bar_max.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, my example was somewhat lazy. What if there are two full sets of statistics in a single telegraf.Metric, does it handle this case?

cpu foo_min=1,foo_max=2,foo_sum=4,foo_count=2,bar_min=1,bar_max=2,bar_sum=4,bar_count=2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Will support this multiple sets case.

## calculate required statistic fields (count, min, max, and sum) and
## enable this flag. This plugin would try to parse those fields and
## send statistic values to Cloudwatch.
# enable_statistic_values = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's call this option write_statistics, this is little shorter, but also communicates that it is something that affects the output data. So long as you are in here, can you mention in the comment that if a field does not have the required statistic fields then it will still be sent as a custom/raw metric.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, will update the field name and comments

@david7482
Copy link
Contributor Author

@danielnelson any update for this PR?
We are looking forward to using this on the nightly build.

@david7482
Copy link
Contributor Author

Already supported multiple statistic sets.

switch {
case strings.HasSuffix(name, "_max"):
sType = statisticTypeMax
fieldName = name[:len(name)-len("_max")]
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use string.TrimSuffix(name, "_max") here


func (f *valueField) buildDatum() []*cloudwatch.MetricDatum {

// Do CloudWatch boundary checking
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we still need to do the boundary checking on the statistic datums? Maybe we can do this where we call convert?

@david7482
Copy link
Contributor Author

Requested changes are fixed.


### write_statistics

If you have a large amount of metrics, you should consider to send statistic
Copy link
Contributor

Choose a reason for hiding this comment

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

Some minor grammar:

  • s/consider to send/consider sending/
  • s/ which could/. This should/
  • s/If enable this flag/If this flag is enabled/


if f.hasAllFields() {
// If we have all required fields, we build datum with StatisticValues
min, _ := f.values[statisticTypeMin]
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to change this, but just using min := f.values[statisticTypeMin] is sufficient, since you are ignoring the bool value anyways.

@@ -50,6 +172,14 @@ var sampleConfig = `

## Namespace for the CloudWatch MetricDatums
namespace = "InfluxData/Telegraf"

## If you have a large amount of metrics, you should consider to send statistic
Copy link
Contributor

Choose a reason for hiding this comment

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

Same grammar suggestions as above.

// Make a MetricDatum from telegraf.Metric. It would check if all required fields of
// cloudwatch.StatisticSet are available. If so, it would build MetricDatum from statistic values.
// Otherwise, fields would still been built independently.
func BuildMetricDatum(buildStatistic bool, point telegraf.Metric) []*cloudwatch.MetricDatum {
Copy link
Contributor

Choose a reason for hiding this comment

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

In general, changing an exported function's signature is advised against. Since nothing outside of this package uses it, maybe add a note that BuildMetricDatum(telegraf.Metric) is deprecated and create a new (unexported) buildMetricDatum(bool, Metric) function.

Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't something we are concerned about right now, maybe someday but not now. The only API we provide stability for is the TOML file. I keep meaning to document this, but haven't gotten to it.

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 this is not an issue, I could help to unexport all exported functions to avoid confusion.
How do you think?

value = t
case bool:
if t {
value = 1
Copy link
Contributor

Choose a reason for hiding this comment

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

I see this is how it was before, but should this be float64(1) and float64(0) in order to preserve the type when sending to an output? cc @danielnelson

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks okay to me, value is a float64 and we are assigning a const to it.

@glinton glinton merged commit 199841a into influxdata:master Aug 1, 2018
@david7482 david7482 deleted the fix-GH4318 branch August 2, 2018 03:28
rgitzel pushed a commit to rgitzel/telegraf that referenced this pull request Oct 17, 2018
otherpirate pushed a commit to otherpirate/telegraf that referenced this pull request Mar 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/aws AWS plugins including cloudwatch, ecs, kinesis feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants