aws-powertools / powertools-lambda-typescript

Powertools is a developer toolkit to implement Serverless best practices and increase developer velocity.
https://docs.powertools.aws.dev/lambda/typescript/latest/
MIT No Attribution
1.57k stars 138 forks source link

Maintenance: remove `logRetention` prop from CDK usage #1980

Closed dreamorosi closed 9 months ago

dreamorosi commented 9 months ago

Summary

In a recent release AWS CDK deprecated the logRetention property in Lambda resources in favor of logGroup. The project uses this prop in a few places and this issue serves to track the efforts to replace it with alternatives.

Currently we use logRetention in three places:

Why is this needed?

The property has been marked as deprecated and will be removed in the next major CDK version. While its eventual removal is likely far off, the CDK team has opted to emit fairly noisy warning logs for each usage of the property (example).

These warnings cause our logs to become quite messy and hard to actually use, so we should get this done sooner rather than later.

Which area does this relate to?

Automation

Solution

In the case of the example the most appropriate action is likely to just remove the logRetention property and use the default value. Adding other methods would needlessly complicate the sample since the point of the example is to show how to use Powertools rather than how to configure the construct.

For the other two cases we need to find a suitable migration path which might or might not be the same for both.

At a high level, the recommended migration path would be to create a log group resource and then pass it to the function resource. This allows us to set the retention period on the log group and also have control over its name.

The main challenge we'll face is naming and resource conflict due to Log Groups being retained by default upon stack deletion. Since the name of the Lambda functions we create is tied to the test being run but unique we might be able to use a well-known pattern like /aws/lambda/<function name> for the name of the log group.

This seems to be in line with the recommendation from the CDK team however we need to run a few integration tests to confirm this. If this is the case, we might also have an opportunity to simplify the test setup and logic slightly by removing the need to obtain the log group name from the CloudFormation stack outputs.

For the Layer stack instead I am less clear on what's the impact since the canary stack seems to stay deployed after releases (which is weird on its own right).

Acknowledgment

Future readers

Please react with 👍 and your use case to help us understand customer demand.

polothy commented 9 months ago

FYI, LoggingConfig for Lambda is not in GovCloud yet.

dreamorosi commented 9 months ago

Hi @polothy, thank you for letting us know!

At the moment we are not running integration tests nor publishing Lambda Layers to non commercial regions.

If/when we'll start we will introduce conditional logic in our CDK app to handle these regions if needed.

perpil commented 9 months ago

FYI they reverted this change: https://github.com/aws/aws-cdk/issues/28919

dreamorosi commented 9 months ago

Great to hear, thank you @perpil and, in hindsight to @polothy for escalating this.

We'll close the issue for the time being and focus on more productive areas.

github-actions[bot] commented 9 months ago

⚠️ COMMENT VISIBILITY WARNING ⚠️

This issue is now closed. Please be mindful that future comments are hard for our team to see.

If you need more assistance, please either tag a team member or open a new issue that references this one.

If you wish to keep having a conversation with other community members under this issue feel free to do so.