Skip to content
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

Maintenance: remove logRetention prop from CDK usage #1980

Closed
1 of 2 tasks
dreamorosi opened this issue Jan 26, 2024 · 5 comments
Closed
1 of 2 tasks

Maintenance: remove logRetention prop from CDK usage #1980

dreamorosi opened this issue Jan 26, 2024 · 5 comments
Assignees
Labels
automation This item relates to automation internal PRs that introduce changes in governance, tech debt and chores (linting setup, baseline, etc.) rejected This is something we will not be working on. At least, not in the measurable future

Comments

@dreamorosi
Copy link
Contributor

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:

  • examples
  • layers canary stack
  • integration tests for all utilities

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.

@dreamorosi dreamorosi added triage This item has not been triaged by a maintainer, please wait internal PRs that introduce changes in governance, tech debt and chores (linting setup, baseline, etc.) labels Jan 26, 2024
@dreamorosi dreamorosi added automation This item relates to automation discussing The issue needs to be discussed, elaborated, or refined and removed triage This item has not been triaged by a maintainer, please wait labels Jan 26, 2024
@polothy
Copy link

polothy commented Jan 29, 2024

FYI, LoggingConfig for Lambda is not in GovCloud yet.

@dreamorosi
Copy link
Contributor Author

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
Copy link

perpil commented Feb 1, 2024

FYI they reverted this change: aws/aws-cdk#28919

@dreamorosi
Copy link
Contributor Author

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.

@dreamorosi dreamorosi closed this as not planned Won't fix, can't repro, duplicate, stale Feb 2, 2024
Copy link
Contributor

github-actions bot commented Feb 2, 2024

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

@dreamorosi dreamorosi added rejected This is something we will not be working on. At least, not in the measurable future and removed discussing The issue needs to be discussed, elaborated, or refined labels Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automation This item relates to automation internal PRs that introduce changes in governance, tech debt and chores (linting setup, baseline, etc.) rejected This is something we will not be working on. At least, not in the measurable future
Projects
Development

No branches or pull requests

4 participants