-
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): deprecate logRetention
properties in favor of logGroup
#28737
Conversation
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.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request
. Additionally, if clarification is needed add Clarification Request
to a comment.
Exempting tests, as this is a deprecation without changing functionality. Feat in name only to show up in change log. |
logRetention
properties in favor of logGroup
logRetention
properties in favor of logGroup
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Question from the peanut gallery: I pulled this change in from today's release, and there are warnings coming from code that is seemingly native to the CDK, in EKS. Should that be happening? I can't do anything about it, as far as I can tell, so this is a pretty tiresome warning. Here's the stack logged from my tests:
I can't find where this is happening in the source of this repo. There is some magic going on here that I do not understand:
|
Seeing similar warnings when using CustomResourceProviders with no way to resolve |
Noisy for CDK project(s) with |
Thanks for calling this out folks! The internal usage should have been flagged by our tools, but wasn't. Looking into options right now. |
…avor of `logGroup` See #28737 for full details. Some custom resources have made the `logRetention` property part of their own API. In these cases, we are now also deprecating `logRetention`. Migrating log groups for custom resource would follow the same steps as outline in #28737. Given that custom resource logging is for debugging purposes and there are no guarantees about the output format, it should be possible to simply replace `logRetention` with a simple `logGroup` in most cases: ```ts const awsCustom1 = new cr.AwsCustomResource(this, 'API1', { // Replace this logRetention: logs.RetentionDays.ONE_WEEK, // with logGroup: new logs.LogGroup(this, 'AwsCustomResourceLogs', { retention: logs.RetentionDays.ONE_WEEK, }), }); ```
Fix is coming via #28783. Cluster is now perfectly quiet again. 🤫 |
Thanks! |
…gGroup` (#28783) In #28737 we have deprecated `logRetention` in favor of `logGroup`. Some custom resources have made the `logRetention` property part of their own API and are now emitting deprecation warnings with no way forward to resolve them. So we are now also deprecating `logRetention` for any custom resources. Migrating log groups for custom resource would follow the same steps as outline in #28737. Given that custom resource logging is for debugging purposes and there are no guarantees about the output format, it should be possible to simply replace `logRetention` with a simple `logGroup` in most cases: ```ts const awsCustom1 = new cr.AwsCustomResource(this, 'API1', { // Replace this logRetention: logs.RetentionDays.ONE_WEEK, // with logGroup: new logs.LogGroup(this, 'AwsCustomResourceLogs', { retention: logs.RetentionDays.ONE_WEEK, }), }); ``` Fixes #28806 Fixes #28809 Related to #28737 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
What happens to orphaned log groups? What is the cost implications of having both log groups from older stacks. |
You can find pricing information here: Logs are charged by data, not log groups. |
### Issue Closes #28919 ### Reason for this change In #28737 and #28783 we have deprecated various `logRetention` properties in favor of the new Logging Configuration feature for Lambda Functions. This new feature provides full control over the Lambda Function log group via the `logGroup` property. However Logging Configuration is not available yet for all regions. Particularly GovCloud and CN regions (at the time this issue was created). For customers in of these regions there is currently no clear migration path. We therefore revert the deprecation of previously deprecated properties. ### Description of changes Revert the deprecation of previously deprecated `logRetention` properties. Update documentation to be more explicit about the various options customers have. ### Description of how you validated changes Documentation & annotation change only. This is a partial revert of the PRs linked about, with minor documentation updates. ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
### Issue Closes #28919 ### Reason for this change In #28737 and #28783 we have deprecated various `logRetention` properties in favor of the new Logging Configuration feature for Lambda Functions. This new feature provides full control over the Lambda Function log group via the `logGroup` property. However Logging Configuration is not available yet for all regions. Particularly GovCloud and CN regions (at the time this issue was created). For customers in of these regions there is currently no clear migration path. We therefore revert the deprecation of previously deprecated properties. ### Description of changes Revert the deprecation of previously deprecated `logRetention` properties. Update documentation to be more explicit about the various options customers have. ### Description of how you validated changes Documentation & annotation change only. This is a partial revert of the PRs linked about, with minor documentation updates. ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@mrgrain I converted all Lambdas in a Stack to use Expanding on the first question "What happens to orphaned log groups?" and disregarding the cost implications question:
|
[surprised] Brewer, Michael (US - California) reacted to your message:
…________________________________
From: Mark Lambert ***@***.***>
Sent: Friday, February 23, 2024 4:24:31 PM
To: aws/aws-cdk ***@***.***>
Cc: Fiserv Plat Eng ***@***.***>; Comment ***@***.***>
Subject: Re: [aws/aws-cdk] feat(lambda): deprecate `logRetention` properties in favor of `logGroup` (PR #28737)
⚠ EXTERNAL MESSAGE – Think Before You Click
What happens to orphaned log groups? What is the cost implications of having both log groups from older stacks.
You can find pricing information here: https://aws.amazon.com/cloudwatch/pricing/<https://urldefense.com/v3/__https://aws.amazon.com/cloudwatch/pricing/__;!!P9vvK-4S!gHAZrwPiBLcgkEmJyT149WOIREZER4VeZCAH_Jj4O4RPKVVqAB3-8OKK-7-P9a3R2nYtkbozO_kxmorvy_lgu_GczK3x8NyoFUgbwg$>
Logs are charged by data, not log groups. Hope that helps.
@mrgrain<https://urldefense.com/v3/__https://github.com/mrgrain__;!!P9vvK-4S!gHAZrwPiBLcgkEmJyT149WOIREZER4VeZCAH_Jj4O4RPKVVqAB3-8OKK-7-P9a3R2nYtkbozO_kxmorvy_lgu_GczK3x8NzSH776HA$> I converted all Lambdas in a Stack to use logGroup and redeployed. The Custom::LogRetention custom resource that was created when using logRetention is still showing in the Stack Template and is assumed to be orphaned.
Expanding on the first question "What happens to orphaned log groups?" and disregarding the cost implications question:
* Are there manual or other recommended steps to clean up CloudFormation Stacks and AWS environments after refactoring your CDK to use logGroup instead of logRetention on Lambdas?
* What is the recommended way to remove the Cutom::LogRetention Resource from your Stack and clean up its resources? Should this be automated in CDK if this is truly orphaned?
* En lieu of an automated/native solution in CDK, is it okay to manually delete the resources Custom::LogRetention creates (LogRetention Lambda, IAM Roles and the CloudWatch Log Groups the LogRetention Lambda creates) and manually delete Custom::LogRetention from the CloudFormation Stack Template? Or is this used in some other way by CDK?
—
Reply to this email directly, view it on GitHub<https://urldefense.com/v3/__https://github.com/aws/aws-cdk/pull/28737*issuecomment-1961625684__;Iw!!P9vvK-4S!gHAZrwPiBLcgkEmJyT149WOIREZER4VeZCAH_Jj4O4RPKVVqAB3-8OKK-7-P9a3R2nYtkbozO_kxmorvy_lgu_GczK3x8NyetK0LYg$>, or unsubscribe<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/A7EKCDH7NLSN3E2WC2FU3ODYVC7D7AVCNFSM6AAAAABB6KGLLKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRRGYZDKNRYGQ__;!!P9vvK-4S!gHAZrwPiBLcgkEmJyT149WOIREZER4VeZCAH_Jj4O4RPKVVqAB3-8OKK-7-P9a3R2nYtkbozO_kxmorvy_lgu_GczK3x8Nx12KPb2Q$>.
You are receiving this because you commented.Message ID: ***@***.***>
|
If you still have a
No. With a default configuration, only the log group created for the execution logs of the custom resource lambda will remain in your account. However this LogGroup has a retention policy of 1 day. If you want to you can delete this log group manually. There is no easy automated way to detect these. However you can limit the search by filtering by name prefix |
#28039 introduced support for custom logging configurations for AWS Lambda Functions.
This change deprecates the
logRetention
,logRetentionRole
andlogRetentionRetryOptions
properties in favor of using a custom logging configuration.By default, Lambda functions send logs to an automatically created default log group named
/aws/lambda/<function name>
. However you cannot change the properties of this auto-created log group using the AWS CDK, e.g. you cannot set a different log retention. To overcome the limitation, a custom resource was introduced and configuration exposed via thelogRetention
properties. This is what we are deprecating in this change.With the introduction of custom logging configuration and the new
logGroup
property, users can now create a fully customizableLogGroup
ahead of time, and instruct the Lambda function to send logs to it.Migrating from
logRetention
tologGroup
will cause the name of the log group to change. Don't attempt to use the name of the auto-created log group, this will cause subtle issue. We recommend using auto-naming for lambda log groups, they can easily be accessed via the Lambda Console. If you want use a well-known name, we recommend using a pattern like/<your service>/lambda/<function name>
. Be aware that a names log group can prevent a stack from being recreated without manual intervention after it has been deployed (errorResource already exists
). This is becauseLogGroups
are retained by default.Either way, users will have to adjust and documentation will need to be updated. Any code referencing the old log group name verbatim will have to be changed as well. Keep in mind that in AWS CDK code, you can access the log group name directly from the
LogGroup
construct:By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license