Update Lambda logging configuration to use non-deprecated AWS CDK properties #2968
Conversation
🦋 Changeset detectedLatest commit: 259e15b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| // We can't easily check the CDK properties from the instance | ||
| // So we verify the synthesized CloudFormation template | ||
| const template = JSON.parse( | ||
| JSON.stringify(app.synth().getStackArtifact('TestStack').template), | ||
| ); | ||
|
|
||
| // Find the LogGroup resource | ||
| const logGroupResources = Object.values(template.Resources).filter( | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| (resource: any) => resource.Type === 'AWS::Logs::LogGroup', | ||
| ); |
There was a problem hiding this comment.
I'm not too proud of this, however, I can see that template is also defined ultimate as any. If someone has a better idea how to implement this, feel free to let me know.
There was a problem hiding this comment.
It looks like you might be able to get the same functionality out of template.resourceCountIs() (which is a part of the aws-cdk-lib/assertions library that is already used in this file). See: https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.assertions-readme.html#counting-resources -- you were doing this in the factory test file as well
| const logGroupResource = logGroupResources[0] as { | ||
| // eslint-disable-next-line @typescript-eslint/naming-convention | ||
| Properties: { RetentionInDays: number }; | ||
| }; |
There was a problem hiding this comment.
Also, I hate using eslint-disable commands. However, the resource names are not following the same naming convention as the amplify-backend repo. Open for other recommendations.
There was a problem hiding this comment.
You should also be able to use aws-cdk-lib/assertions for this with template.hasResourceProperties -- https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.assertions-readme.html#resource-matching--retrieval
|
@pahud Could you maybe assist in testing this against a real AWS environment? I have no account at hand that I can run the test suite against without having the risk of possibly causing some costs on my personal credit card. |
|
Overall this PR looks pretty good, I am going to approve PR workflows to run on it and we'll see how it does. I'll get back to you about testing in a real AWS environment once we see how your PR did with the workflows. |
|
Based on the PR checks run, it confirms that your won't be introducing any API changes to |
| // We can't easily check the CDK properties from the instance | ||
| // So we verify the synthesized CloudFormation template | ||
| const template = JSON.parse( | ||
| JSON.stringify(app.synth().getStackArtifact('TestStack').template), | ||
| ); | ||
|
|
||
| // Find the LogGroup resource | ||
| const logGroupResources = Object.values(template.Resources).filter( | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| (resource: any) => resource.Type === 'AWS::Logs::LogGroup', | ||
| ); |
There was a problem hiding this comment.
It looks like you might be able to get the same functionality out of template.resourceCountIs() (which is a part of the aws-cdk-lib/assertions library that is already used in this file). See: https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.assertions-readme.html#counting-resources -- you were doing this in the factory test file as well
| const logGroupResource = logGroupResources[0] as { | ||
| // eslint-disable-next-line @typescript-eslint/naming-convention | ||
| Properties: { RetentionInDays: number }; | ||
| }; |
There was a problem hiding this comment.
You should also be able to use aws-cdk-lib/assertions for this with template.hasResourceProperties -- https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.assertions-readme.html#resource-matching--retrieval
|
|
||
| /** | ||
| * Converts logging options to CDK format using non-deprecated properties | ||
| * Note: This function no longer includes 'retention' in the return object |
There was a problem hiding this comment.
Can you remove the note about retention being removed from here and references to the deprecated logRetention property in the other comments in this file, this will not be useful for people in the future who don't need to know this was previously implemented with logRetention.
Problem
Issue number, if available: #2965
Changes
Refactored
logging_options_parser.tsto use modern, non-deprecated AWS CDK properties for Lambda loggingReplaced the deprecated
logRetentionproperty with a dedicatedLogGroupresourceUpdated
applicationLogLevelV2andloggingFormatusage to follow AWS CDK best practicesUpdated tests to verify the new implementation works correctly
Maintained backward compatibility for end users (no API changes)
Updated documentation to have logging field documented in general. See PR [Amplify JS] add logging configuration options for functions docs#8428
Why
AWS CDK marked several properties in
NodejsFunctionas deprecated:logRetention- Recommended to use a dedicatedLogGroupresourceLogGroupautomatically to be used if logRetention is set. Will fallback to defaultNodeJsFunctionwhenundefined.This change ensures that Lambda functions continue to work properly with future AWS CDK versions.
Testing
Corresponding docs PR, if applicable: TBD
Validation
Have ran tests locally. Should be tested against an AWS account before going live.
Checklist
run-e2elabel set.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.