[lambda][logs] Make LogRetention construct reusable for any log group not just Lambda's

[humanzz]

Various AWS resources autocreate log groups if they don't exist e.g. Lambda functions, SageMaker Endpoints.

For a while, my team have managed to create log groups explicitly for lambda functions to customize the log retention policies. This worked well because Lambda didn't attempt to create the log groups until the first request. However, I believe that with recent changes over the past year in Lambda (VPC improvements, Provisioned Concurrency), log groups can be created before the first request which makes our approach slightly problematic.

We noticed the introduction of LogRetention construct and log retention props for Lambda functions that utilize it to let the function create the log group and have a custom resource that manages setting the retention policy.

We think that the LogRetention construct can be reused outside of just Lambda functions e.g. SageMaker Endpoints.

This is a feature request to update LogsRetention construct and the provider it uses to make them reusable for non-Lambda function log groups.

Use Case

Enable using the LogRetention construct for any AWS resource that auocreates logs to manage those logs retention.
Out of scope is introducing different props to different resource constructs to add that support. That can come in later changes.

• 👋 I may be able to implement this feature request
• ⚠️ This feature might incur a breaking change

As my team might be able to contribute to this work, we're asking for feedback on this approach. If you think it's sensible, then one of the questions we have is about the location of this construct (it's currently part of the lambda module) but if it's to become general purpose, it probably should go somewhere else e.g. the logs module.

---

This is a 🚀 Feature Request

[nija-at]

We currently don't have higher level construct support for sagemaker. This makes sense to implement something like this when we build that there.

@humanzz - do you have a wishlist (besides sagemaker) where this would be useful?

[humanzz]

The point is, I'm trying to separate the 2 things

1. Make LogRetention construct reusable across multiple services. We're willing to contribute here
2. Other services to implement the same approach. That's the part we want to leave to you guys

The usage of 1 need not be coupled with other higher level constructs. For instance, we use Glue... and Glue creates a central non Glue job specific glue that we'd also like to set retention on.

In terms of the immediate services we have in mind; Lambda ✅ , SageMaker, AWS Chatbot where we're also considering contributing a higher level construct for

AWS Chatbot has an interesting aspect to it though because it's a global service that puts its logs in us-east-1 regardless of the region you create the chatbot resource in. My understanding is that this is also a common pattern for global services.
In our own infrastructure, we're doing a quick PoC that shows that the constructs in 1 above can also handle that.

So in our mind we had a plan a long the lines of

1. Do the contribution in 1 above
2. Do the AWS Chatbot contribution utilizing it

[jogold]

@humanzz the LogRetention construct is already re-usable outside Lambda. It acts on a log group not a Lambda function. It has been initially located in the aws-lambda module instead of the aws-logs module to avoid a circular dependency between those two modules. Not ideal in terms of discoverability, I agree.

It's currently used in aws-rds for example:

aws-cdk/packages/@aws-cdk/aws-rds/lib/instance.ts

Lines 573 to 578 in 4bc8188

	for (const log of this.cloudwatchLogsExports) {
	new lambda.LogRetention(this, `LogRetention${log}`, {
	logGroupName: `/aws/rds/instance/${this.instanceIdentifier}/${log}`,
	retention: this.cloudwatchLogsRetention,
	role: this.cloudwatchLogsRetentionRole,
	});

[humanzz]

@jogold Thanks for pointing that out. My initial scan of the provider code drove me to the wrong conclusion as I thought the implementation is Lambda-specific

aws-cdk/packages/@aws-cdk/aws-lambda/lib/log-retention-provider/index.ts

Lines 71 to 72 in 014c13a

	await createLogGroupSafe(`/aws/lambda/${context.functionName}`, retryOptions);
	await setRetentionPolicy(`/aws/lambda/${context.functionName}`, retryOptions, 1);

as I thought that's the Lambda function whose logs we were setting the retention for. Turns out these lines are just for the custom resource Lambda function.

The other thing we might try to contribute to (if we figure this out properly) is updating the construct to allow for cross-region log retention setting for the case I described above for Global AWS regions that create logs in us-east-1.

Maybe we can repurpose this issue for that?

[nija-at]

@jogold - you're right that it is reusable. However, it creates a dependency on the aws-lambda module can be avoided.

If we switch the implementation to use core.CustomResourceProvider, we should be able to move this to the core module, this will avoid any unneeded bloat on the dependency graph for the module using it.

@eladb - what do you think?

The other thing we might try to contribute to (if we figure this out properly) is updating the construct to allow for cross-region log retention setting for the case I described above for Global AWS regions that create logs in us-east-1.

Maybe we can repurpose this issue for that?

@humanzz - since this is an orthogonal request, would be nice to open a separate issue and track this separately. Could you do that?

[humanzz]

@nija-at sure.

[eladb]

Feels like this should be in the @aws-cdk/aws-logs module.

[humanzz]

We'll work on #9703 first (likely a non-breaking change). We can then work on this one (moving the construct which is a breaking change) unless you plan to do it soon.

[nija-at]

This will have to be a non-breaking change as well.
You will need to move the construct to its new home, mark this as deprecated and try to delegate as much of its logic as possible to the new implementation.

(Suggested ordering) might make sense to move this before making the change to support global AWS services, given that lambda is not one of the global AWS services.

[humanzz]

@nija-at I think our preference is to do the other change first since it's a smaller one.
We'll probably have spare cycles to use for it this week.
For this one, it will probably take a bit longer... because as far as your comments and our initial scan of code see, some code will have to be reimplemented such that there's no dependency on lambda module; more specifically the SingletonFunction.
Also, this being our first contribution, it's easier for us to start with the other more straightforward change.

[humanzz]

@nija-at given i'll be off the next couple of days, I wanted to leave a draft PR so you can have some initial comments on it. I'll be addressing them on Monday
The main things I wanna call out

• I had to reimplement some singleton function functionality using CfnResource. This led to some changes in logical Ids
• There's some funky code in there about function dependencies on role/policy which I feel can be improved. I look forwards to your comments on that