Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 15 commits into from
Jun 12, 2020
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
53fcc4e
fix(lambda): increase retry count for creating cloudwatch log groups
jaapvanblaaderen May 28, 2020
4856cb3
Merge branch 'master' into log-retention-rate-exceeded-error
jaapvanblaaderen Jun 8, 2020
044a81c
fix(lambda): Make retry options configurable for CloudWatchLogs group…
jaapvanblaaderen Jun 8, 2020
4f3ef1d
Merge branch 'master' into log-retention-rate-exceeded-error
jaapvanblaaderen Jun 8, 2020
1f86f2f
fix(lambda): Fix integration test
jaapvanblaaderen Jun 8, 2020
5e9a4f0
Merge remote-tracking branch 'jaap/log-retention-rate-exceeded-error'…
jaapvanblaaderen Jun 8, 2020
7ca18c7
fix(lambda): Code cleanup - fix review comments
jaapvanblaaderen Jun 11, 2020
f33ae20
Merge branch 'master' into log-retention-rate-exceeded-error
jaapvanblaaderen Jun 11, 2020
c4823d5
fix(lambda):Update readme
jaapvanblaaderen Jun 11, 2020
1c6f5d7
Update packages/@aws-cdk/aws-lambda/lib/log-retention.ts
jaapvanblaaderen Jun 12, 2020
6708723
Tweak comment
jaapvanblaaderen Jun 12, 2020
6f4057c
fix(lambda): Fix review comments
jaapvanblaaderen Jun 12, 2020
458b074
Merge remote-tracking branch 'jaap/log-retention-rate-exceeded-error'…
jaapvanblaaderen Jun 12, 2020
1c15e29
Merge branch 'master' into log-retention-rate-exceeded-error
mergify[bot] Jun 12, 2020
7de918b
Merge branch 'master' into log-retention-rate-exceeded-error
mergify[bot] Jun 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions packages/@aws-cdk/aws-lambda/lib/log-retention-provider/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,17 @@
// eslint-disable-next-line import/no-extraneous-dependencies
import * as AWS from 'aws-sdk';

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

@nija-at nija-at Jun 2, 2020

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@jaapvanblaaderen jaapvanblaaderen Jun 2, 2020

Choose a reason for hiding this comment

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

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

  1. 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
  }
}

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

  2. Use the retry options in the log retention provider Lambda.

Copy link
Contributor

@nija-at nija-at Jun 2, 2020

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 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.

Copy link
Contributor Author

@jaapvanblaaderen jaapvanblaaderen Jun 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@jaapvanblaaderen jaapvanblaaderen Jun 3, 2020

Choose a reason for hiding this comment

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

@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 🙂

Copy link
Contributor

@nija-at nija-at Jun 8, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@jaapvanblaaderen jaapvanblaaderen Jun 8, 2020

Choose a reason for hiding this comment

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

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


/**
* Creates a log group and doesn't throw if it exists.
*
* @param logGroupName the name of the log group to create
*/
async function createLogGroupSafe(logGroupName: string) {
try { // Try to create the log group
const cloudwatchlogs = new AWS.CloudWatchLogs({ apiVersion: '2014-03-28' });
const cloudwatchlogs = new AWS.CloudWatchLogs({ apiVersion: '2014-03-28', ...retryOptions});
await cloudwatchlogs.createLogGroup({ logGroupName }).promise();
} catch (e) {
if (e.code !== 'ResourceAlreadyExistsException') {
Expand All @@ -26,7 +29,7 @@ async function createLogGroupSafe(logGroupName: string) {
* @param retentionInDays the number of days to retain the log events in the specified log group.
*/
async function setRetentionPolicy(logGroupName: string, retentionInDays?: number) {
const cloudwatchlogs = new AWS.CloudWatchLogs({ apiVersion: '2014-03-28' });
const cloudwatchlogs = new AWS.CloudWatchLogs({ apiVersion: '2014-03-28', ...retryOptions});
if (!retentionInDays) {
await cloudwatchlogs.deleteRetentionPolicy({ logGroupName }).promise();
} else {
Expand Down
18 changes: 9 additions & 9 deletions packages/@aws-cdk/aws-lambda/test/integ.log-retention.expected.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@
"Properties": {
"Code": {
"S3Bucket": {
"Ref": "AssetParameters82c54bfa7c42ba410d6d18dad983ba51c93a5ea940818c5c20230f8b59c19d4eS3Bucket7046E6CE"
"Ref": "AssetParametersc11219c29950dc50a505625251bd4e6b553c3d85f04bcba46572f2e25e8fe6beS3BucketD782C750"
},
"S3Key": {
"Fn::Join": [
Expand All @@ -146,7 +146,7 @@
"Fn::Split": [
"||",
{
"Ref": "AssetParameters82c54bfa7c42ba410d6d18dad983ba51c93a5ea940818c5c20230f8b59c19d4eS3VersionKey3194A583"
"Ref": "AssetParametersc11219c29950dc50a505625251bd4e6b553c3d85f04bcba46572f2e25e8fe6beS3VersionKeyB87E9196"
}
]
}
Expand All @@ -159,7 +159,7 @@
"Fn::Split": [
"||",
{
"Ref": "AssetParameters82c54bfa7c42ba410d6d18dad983ba51c93a5ea940818c5c20230f8b59c19d4eS3VersionKey3194A583"
"Ref": "AssetParametersc11219c29950dc50a505625251bd4e6b553c3d85f04bcba46572f2e25e8fe6beS3VersionKeyB87E9196"
}
]
}
Expand Down Expand Up @@ -331,17 +331,17 @@
}
},
"Parameters": {
"AssetParameters82c54bfa7c42ba410d6d18dad983ba51c93a5ea940818c5c20230f8b59c19d4eS3Bucket7046E6CE": {
"AssetParametersc11219c29950dc50a505625251bd4e6b553c3d85f04bcba46572f2e25e8fe6beS3BucketD782C750": {
"Type": "String",
"Description": "S3 bucket for asset \"82c54bfa7c42ba410d6d18dad983ba51c93a5ea940818c5c20230f8b59c19d4e\""
"Description": "S3 bucket for asset \"c11219c29950dc50a505625251bd4e6b553c3d85f04bcba46572f2e25e8fe6be\""
},
"AssetParameters82c54bfa7c42ba410d6d18dad983ba51c93a5ea940818c5c20230f8b59c19d4eS3VersionKey3194A583": {
"AssetParametersc11219c29950dc50a505625251bd4e6b553c3d85f04bcba46572f2e25e8fe6beS3VersionKeyB87E9196": {
"Type": "String",
"Description": "S3 key for asset version \"82c54bfa7c42ba410d6d18dad983ba51c93a5ea940818c5c20230f8b59c19d4e\""
"Description": "S3 key for asset version \"c11219c29950dc50a505625251bd4e6b553c3d85f04bcba46572f2e25e8fe6be\""
},
"AssetParameters82c54bfa7c42ba410d6d18dad983ba51c93a5ea940818c5c20230f8b59c19d4eArtifactHashB967D42A": {
"AssetParametersc11219c29950dc50a505625251bd4e6b553c3d85f04bcba46572f2e25e8fe6beArtifactHashBA1C5764": {
"Type": "String",
"Description": "Artifact hash for asset \"82c54bfa7c42ba410d6d18dad983ba51c93a5ea940818c5c20230f8b59c19d4e\""
"Description": "Artifact hash for asset \"c11219c29950dc50a505625251bd4e6b553c3d85f04bcba46572f2e25e8fe6be\""
}
}
}