feat(lambda): configurable retries for log retention custom resource #8258

jaapvanblaaderen · 2020-05-28T14:25:24Z

This prevents throttling issues on stacks with a lot of Lambdas.

Implemented configurable maxRetries and base properties as part of the LogRetentionRetryOptions. The AWS SDK also supports specifying a customBackoff function. I skipped that as it's hard to implement in the current setup (impossible to provide a callback function in the event JSON).

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

This prevents throttling issues on stacks with a lot of Lambdas. fixes aws#8257

aws-cdk-automation · 2020-05-28T14:43:57Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 53fcc4e
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

nija-at · 2020-06-02T10:10:28Z

packages/@aws-cdk/aws-lambda/lib/log-retention-provider/index.ts

+// Ensure we do not run into throttling issues when deploying stack(s) with a lot of Lambdas.
+const retryOptions = { maxRetries: 6, retryDelayOptions: { base: 300 }};


How about making this configurable here -

aws-cdk/packages/@aws-cdk/aws-lambda/lib/log-retention.ts

Line 12 in fd54a17

export interface LogRetentionProps {

- and leave the default as the SDK's default?

Ok, that makes sense. Do you mean something like this:

Add retry options to LogRetentionProps:

export interface LogRetentionProps { readonly logGroupName: string; readonly retention: logs.RetentionDays; readonly role?: iam.IRole; readonly logGroupRetryOptions?:LogGroupRetryOptions; } export interface LogGroupRetryOptions { readonly maxRetries?: number; readonly retryDelayOptions?: { readonly base?: number readonly customBackoff?: (retryCount: number, err?: Error) => number } }

Allow the retry options to be configured in the Lambda by adding the LogGroupRetryOptions property also in FunctionOptions.

Use the retry options in the log retention provider Lambda.

You don't need to do #2.

Once defined in LogRetentionProps, pass it into the properties key and it should be available in the provider lambda. Just like the other values under properties.

I understand that, but I guess the goal here is to also make the retry options configurable via the Lambda props? So when a CDK users creates a Lambda, they can also specify the retry options.

The logRetention and logRetentionRole props are also part of FunctionOptions to enable something similar.

@nija-at Just checking, does my previous comment makes sense? I'd like to continue and obviously create a wonderful PR to fix the issue 🙂

Yes, something like that makes sense. Thanks for explaining.

Made the changes, please check the PR description for additional details on implementation choices.

Pull request has been modified.

aws-cdk-automation · 2020-06-08T09:09:50Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 4856cb3
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

nija-at

Setting this back to request changes

… management fixes aws#8257

Pull request has been modified.

aws-cdk-automation · 2020-06-08T11:15:38Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 044a81c
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-06-08T14:07:12Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 4f3ef1d
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

fixes aws#8257

… into log-retention-rate-exceeded-error

aws-cdk-automation · 2020-06-08T22:37:59Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 5e9a4f0
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

nija-at

Thanks for making the changes. My initial comments below.

nija-at · 2020-06-09T14:36:32Z

packages/@aws-cdk/aws-lambda/lib/function.ts

+   * Retry options for creating CloudWatch log groups. Deploying many Lambdas
+   * with log retention resources may result in rate limit issues when creating
+   * CloudWatch Log groups. The retry options allow you to customize the retry
+   * options in order to successfully create these.


Sufficient to just document what these options are. There can be many reasons to use them.

Something like this -

Suggested change

* Retry options for creating CloudWatch log groups. Deploying many Lambdas

* with log retention resources may result in rate limit issues when creating

* CloudWatch Log groups. The retry options allow you to customize the retry

* options in order to successfully create these.

* When log retention is specified, a custom resource attempts to create the CloudWatch log group.

* These options control the retry policy when interacting with CloudWatch APIs.

Agree, will change that.

nija-at · 2020-06-09T14:39:08Z

packages/@aws-cdk/aws-lambda/lib/log-retention-provider/index.ts

@@ -2,15 +2,16 @@

 // eslint-disable-next-line import/no-extraneous-dependencies
 import * as AWS from 'aws-sdk';
+import { LogRetentionRetryOptions } from '../log-retention';


I don't think this will work, the lambda function execution should fail. Did you try deploying a stack that used this?

This file gets deployed as a lambda function during stack deployment. The CDK code is not available there and it should not be.

I linked it using a postinstall hook in my local test project. Then it works (I guess since it's just an interface that doesn't add any logic to the deployed lambda function). But this is indeed incorrect, I'll fix it.

nija-at · 2020-06-09T14:44:48Z

packages/@aws-cdk/aws-lambda/lib/log-retention.ts

@@ -69,6 +94,7 @@ export class LogRetention extends cdk.Construct {
      properties: {
        ServiceToken: provider.functionArn,
        LogGroupName: props.logGroupName,
+        LogRetentionRetryOptions: props.logRetentionRetryOptions,


Just Retry or SdkRetry. We're already in the context of LogRetention.

Ok, will change

nija-at · 2020-06-09T14:47:59Z

packages/@aws-cdk/aws-lambda/lib/log-retention.ts

+  /**
+   * The maximum amount of retries.
+   *
+   * @default - AWS SDK default


We usually document the default, even if it comes from a dependency

Hmmm, yeah I can change that, but left it like this on purpose. How will you make sure it stays in sync?

It's unlikely the SDK's defaults change. Changing so may break existing customers using the CLI whose applications work because of a given set of defaults, so this is likely to be considered a breaking change.

It may change on the next major version but we have better control over that from the import statements.

nija-at · 2020-06-09T14:48:40Z

packages/@aws-cdk/aws-lambda/lib/log-retention.ts

+   *
+   * @default - AWS SDK default
+   */
+  readonly base?: number;


Use Duration type from the @aws-cdk/core module.

nija-at · 2020-06-09T14:49:52Z

packages/@aws-cdk/aws-lambda/lib/log-retention.ts

@@ -26,6 +26,31 @@ export interface LogRetentionProps {
   * @default - A new role is created
   */
  readonly role?: iam.IRole;
+
+  /**
+   * Retry options for managing CloudWatch log groups.


maybe this?

Suggested change

* Retry options for managing CloudWatch log groups.

* Retry options for all AWS API calls.

packages/@aws-cdk/aws-lambda/lib/log-retention.ts

fixes aws#8257

Pull request has been modified.

aws-cdk-automation · 2020-06-11T08:01:26Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 7ca18c7
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-06-11T08:29:20Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: f33ae20
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

fixes aws#8257

aws-cdk-automation · 2020-06-11T09:35:56Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: c4823d5
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

jaapvanblaaderen · 2020-06-11T09:38:27Z

All review comments addressed, please review again.

nija-at

Looks great! Good stuff with this PR.

Couple of small suggestions to polish up.

nija-at · 2020-06-12T08:36:41Z

packages/@aws-cdk/aws-lambda/lib/log-retention-provider/index.ts

+  function parseRetryOptions(rawOptions: any) {
+    const retryOptions: { maxRetries?: number, retryOptions?: { base?: number } } = {};


Might be nice to take advantage of type checking (instead of using any) and define a type like

interface SdkRetryOptions { readonly maxRetries?: number; retryOptions?: AWS.RetryDelayOptions; }

parseRetryOptions will return this type and the options parameter in APIs like createLogGroupSafe and setRetentionPolicy() can be set to this type.

packages/@aws-cdk/aws-lambda/lib/log-retention.ts

Tweak comment aws#8258 Co-authored-by: Niranjan Jayakar <[email protected]>

aws#8258 Co-authored-by: Niranjan Jayakar <[email protected]>

Pull request has been modified.

aws-cdk-automation · 2020-06-12T10:41:18Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 1c6f5d7
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-06-12T10:42:21Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 6708723
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

fixes aws#8257

… into log-retention-rate-exceeded-error

jaapvanblaaderen · 2020-06-12T15:10:43Z

Added your suggestions. Hopefully it's merge worthy now 🙂

aws-cdk-automation · 2020-06-12T15:54:15Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 458b074
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2020-06-12T16:08:44Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2020-06-12T16:56:50Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 1c15e29
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-06-12T17:40:19Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 7de918b
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2020-06-12T17:40:44Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

fix(lambda): increase retry count for creating cloudwatch log groups

53fcc4e

This prevents throttling issues on stacks with a lot of Lambdas. fixes aws#8257

SomayaB assigned nija-at Jun 1, 2020

nija-at previously requested changes Jun 2, 2020

View reviewed changes

Merge branch 'master' into log-retention-rate-exceeded-error

4856cb3

nija-at previously requested changes Jun 8, 2020

View reviewed changes

fix(lambda): Make retry options configurable for CloudWatchLogs group…

044a81c

… management fixes aws#8257

Merge branch 'master' into log-retention-rate-exceeded-error

4f3ef1d

jaapvanblaaderen added 2 commits June 8, 2020 23:48

fix(lambda): Fix integration test

1f86f2f

fixes aws#8257

Merge remote-tracking branch 'jaap/log-retention-rate-exceeded-error'…

5e9a4f0

… into log-retention-rate-exceeded-error

jaapvanblaaderen requested a review from nija-at June 9, 2020 13:36

nija-at changed the title ~~fix(lambda): increase retry count for creating cloudwatch log groups~~ feat(lambda): configurable retries for log retention custom resource Jun 9, 2020

nija-at previously requested changes Jun 9, 2020

View reviewed changes

fix(lambda): Code cleanup - fix review comments

7ca18c7

fixes aws#8257

Merge branch 'master' into log-retention-rate-exceeded-error

f33ae20

fix(lambda):Update readme

c4823d5

fixes aws#8257

jaapvanblaaderen requested a review from nija-at June 11, 2020 09:04

nija-at previously requested changes Jun 12, 2020

View reviewed changes

Update packages/@aws-cdk/aws-lambda/lib/log-retention.ts

1c6f5d7

Tweak comment aws#8258 Co-authored-by: Niranjan Jayakar <[email protected]>

Tweak comment

6708723

aws#8258 Co-authored-by: Niranjan Jayakar <[email protected]>

jaapvanblaaderen added 2 commits June 12, 2020 17:06

fix(lambda): Fix review comments

6f4057c

fixes aws#8257

Merge remote-tracking branch 'jaap/log-retention-rate-exceeded-error'…

458b074

… into log-retention-rate-exceeded-error

jaapvanblaaderen requested a review from nija-at June 12, 2020 15:09

nija-at approved these changes Jun 12, 2020

View reviewed changes

mergify bot added 2 commits June 12, 2020 16:09

Merge branch 'master' into log-retention-rate-exceeded-error

1c15e29

Merge branch 'master' into log-retention-rate-exceeded-error

7de918b

mergify bot merged commit e17a49a into aws:master Jun 12, 2020

clifflaschet mentioned this pull request Aug 9, 2023

Throttling: Rate exceeded #5637

Closed

		// Ensure we do not run into throttling issues when deploying stack(s) with a lot of Lambdas.
		const retryOptions = { maxRetries: 6, retryDelayOptions: { base: 300 }};

	* Retry options for managing CloudWatch log groups.
	* Retry options for all AWS API calls.

		function parseRetryOptions(rawOptions: any) {
		const retryOptions: { maxRetries?: number, retryOptions?: { base?: number } } = {};

feat(lambda): configurable retries for log retention custom resource #8258

feat(lambda): configurable retries for log retention custom resource #8258

Conversation

jaapvanblaaderen commented May 28, 2020 • edited Loading

aws-cdk-automation commented May 28, 2020

AWS CodeBuild CI Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaapvanblaaderen Jun 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Jun 8, 2020

AWS CodeBuild CI Report

nija-at left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Jun 8, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jun 8, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jun 8, 2020

AWS CodeBuild CI Report

nija-at left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaapvanblaaderen Jun 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaapvanblaaderen Jun 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Jun 11, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jun 11, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jun 11, 2020

AWS CodeBuild CI Report

jaapvanblaaderen commented Jun 11, 2020

nija-at left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Jun 12, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jun 12, 2020

AWS CodeBuild CI Report

jaapvanblaaderen commented Jun 12, 2020

aws-cdk-automation commented Jun 12, 2020

AWS CodeBuild CI Report

mergify bot commented Jun 12, 2020

aws-cdk-automation commented Jun 12, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jun 12, 2020

AWS CodeBuild CI Report

mergify bot commented Jun 12, 2020

jaapvanblaaderen commented May 28, 2020 •

edited

Loading

jaapvanblaaderen Jun 2, 2020 •

edited

Loading

jaapvanblaaderen Jun 9, 2020 •

edited

Loading

jaapvanblaaderen Jun 9, 2020 •

edited

Loading