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

fix(core): fix rate limit errors when deploying cloudwatch log groups #822

Merged
merged 4 commits into from
Sep 21, 2022

Conversation

xuetinp
Copy link
Contributor

@xuetinp xuetinp commented Sep 20, 2022

Problem #807

When the RFDK creates a CloudWatch Log group it does it via the CDK's LogRetention construct. The CloudWatch Logs APIs that the LogRetention construct calls have a very low TPS limit (5 TPS). The result is that when deploying multiple RFDK constructs one can encounter rate limit errors in the deployment -- this is particularly seen, randomly, with the RFDK's integration tests.

Solution

CDK's LogRetention construct provides a way - logRetentionRetryOptions to configure the Javascript SDK that it's using to make the CloudWatch API calls. Using this, the RFDK should set higher max retries and retry delay. Currently default backoff retry base is 100ms and max retry count is 3, proposed value is 200ms and 7. This is a good combination as it achieves a lot of retries while the total function runtime is not too long.

Testing

  • Created a CloudFormation app that iteratively initialize LogRetention constructs. Wrote a script to deploy constructs in parallel to reproduce the rate limit error. Verified proposed values for max retry count and retry delay prevent the error.
  • Added unit test to verify template is modified as expected.
  • Build success.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@xuetinp xuetinp changed the title fix(core): fix rate limit errors when deploying cloudwatch log groups #807 fix(core): fix rate limit errors when deploying cloudwatch log groups Sep 20, 2022
@ddneilson ddneilson self-requested a review September 20, 2022 16:04
ddneilson
ddneilson previously approved these changes Sep 20, 2022
Copy link
Contributor

@ddneilson ddneilson left a comment

Choose a reason for hiding this comment

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

This is perfect. Thank you!

Copy link
Contributor

@jusiskin jusiskin left a comment

Choose a reason for hiding this comment

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

Great work. I have two minor suggestions to improve this.

packages/aws-rfdk/lib/core/lib/exporting-log-group.ts Outdated Show resolved Hide resolved
packages/aws-rfdk/lib/core/lib/exporting-log-group.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@jericht jericht left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

4 participants