-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat(lambda): add support for log retention #2067
Conversation
Adds a new property `logRetentionDays` on `Function` to control the log retention policy of the function logs in CloudWatch Logs. The implementation uses a Custom Resource to create the log group if it doesn't exist yet and to set the retention policy as discussed in aws#667. A retention policy of 1 day is set on the logs of the Lambda provider. The different retention days supported by CloudWatch Logs have been centralized in `@aws-cdk/aws-logs`. Some have been renamed to better match the console experience. Closes aws#667 BREAKING CHANGE: `cloudWatchLogsRetentionTimeDays` in `@aws-cdk/aws-cloudtrail` now uses a `logs.RetentionDays` instead of a `LogRetention`.
// Need to use a CfnResource here to prevent lerna dependency cycles | ||
// @aws-cdk/aws-cloudformation -> @aws-cdk/aws-lambda -> @aws-cdk/aws-cloudformation | ||
new cdk.CfnResource(this, 'LogRetentionCustomResource', { | ||
type: 'AWS::CloudFormation::CustomResource', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use a custom type name that indicates what this type is doing (I believe "AWS::Custom::XXX" or something along those lines)
// Log retention | ||
if (props.logRetentionDays) { | ||
// Custom resource provider | ||
const provider = new SingletonFunction(this, 'LogRetentionProvider', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrap this whole thing up into a construct that encapsulates all the custom resource details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, what about moving this construct to aws-cloudformation
or aws-logs
(would cause a dep cycle) then? It could be used for CodeBuild and other services. For RDS, we have a similar situation when CloudWatch Logs are enabled (the log group is created during instance creation)
new LogRetention(this, 'LogRetention', {
logGroupName: ...,
retentionInDays: ...
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or to a new package (@aws-cdk/custom-resource
) as discussed in #1850.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this will create dependency cycles (SingletonFunction
)...
} catch (e) { | ||
console.log(e); | ||
|
||
await respond('FAILED', e.message, context.logStreamName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use logGroupName
as the physical resource name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, should be logGroupName
here also.
@@ -0,0 +1,30 @@ | |||
import logs = require('@aws-cdk/aws-logs'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a unit test for the handler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great.
Hello, when using this property on my existing lambda resource, new lambda resource attempts to be created, is this intentional behavior? Why this new lambda function is attempted to be created? |
Adds a new property
logRetentionDays
onFunction
to control the logretention policy of the function logs in CloudWatch Logs.
The implementation uses a Custom Resource to create the log group if it doesn't
exist yet and to set the retention policy as discussed in #667.
A retention policy of 1 day is set on the logs of the Lambda provider.
The different retention days supported by CloudWatch Logs have been centralized
in
@aws-cdk/aws-logs
. Some have been renamed to better match the consoleexperience.
Closes #667
BREAKING CHANGE:
cloudWatchLogsRetentionTimeDays
in@aws-cdk/aws-cloudtrail
now uses a
logs.RetentionDays
instead of aLogRetention
.Pull Request Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.