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

Update default value for Cloudwatch rate limit #2520

Merged
merged 4 commits into from
Mar 15, 2017

Conversation

AntoineAugusti
Copy link
Contributor

@AntoineAugusti AntoineAugusti commented Mar 9, 2017

Follow up of: #2298

New documentation: http://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/cloudwatch_limits.html

Rate limit is used for the GetMetricStatistics endpoint.

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)

Copy link
Contributor

@sparrc sparrc left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

please update the changelog

@sparrc
Copy link
Contributor

sparrc commented Mar 9, 2017

PS: also please put that documentation link in the SampleConfig

@sparrc
Copy link
Contributor

sparrc commented Mar 9, 2017

cc @johnrengelman

@AntoineAugusti
Copy link
Contributor Author

@sparrc Done that and squashed. thanks!

Copy link
Contributor

@johnrengelman johnrengelman left a comment

Choose a reason for hiding this comment

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

Lgtm

@sparrc sparrc added this to the 1.3.0 milestone Mar 9, 2017
@phemmer
Copy link
Contributor

phemmer commented Mar 9, 2017

If we're saying cloudwatch has a limit of 400 requests per second, do we really want to default to telegraf using all of it? This means that if telegraf has it maxed out, and someone simply goes to the AWS dashboard and tries to pull up cloudwatch metrics, their browser is going to push it over the limit. Ditto if they have any other system which also uses cloudwatch metrics. Should we not choose a safer default, and let people increase it to the max if the default isn't enough?

@johnrengelman
Copy link
Contributor

it's probably time revive this PR #1795 so that telegraf can pull multiple periods of data in 1 request thus saving the API limit.

@sparrc
Copy link
Contributor

sparrc commented Mar 9, 2017

@phemmer makes a good point....@AntoineAugusti can you lower the default to ~200?

@AntoineAugusti
Copy link
Contributor Author

@sparrc Sure, done

@danielnelson danielnelson merged commit 426182b into influxdata:master Mar 15, 2017
ssorathia pushed a commit to ssorathia/telegraf that referenced this pull request Mar 25, 2017
calerogers pushed a commit to calerogers/telegraf that referenced this pull request Apr 5, 2017
vlamug pushed a commit to vlamug/telegraf that referenced this pull request May 30, 2017
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.

5 participants