aws / chalice

Python Serverless Microframework for AWS
Apache License 2.0
10.61k stars 1.01k forks source link

Add log retention setting for lambdas #2019

Closed hamishfagg closed 1 year ago

hamishfagg commented 1 year ago

Adds #943

Adds a stage-specific setting log_retention_in_days. When this is set, a log group will be created for every lambda function in the stage, and the log retention policy will be set to the configured duration.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

jamesls commented 1 year ago

Hi, thanks for the PR. I'm looking over it now. Just a heads up, the test failures look unrelated to this PR and should be fixed in https://github.com/aws/chalice/pull/2021.

jamesls commented 1 year ago

Quick update, I added support for cloudformation template generation. Looks like the common way to do this is to create a log group that matches the expected log group name (https://awslabs.github.io/serverless-rules/rules/lambda/log_retention/). A couple things I noticed:

Otherwise looking good, should be able to merge this one those action items are finished.

jamesls commented 1 year ago

@stealthycoin ready to be looked at. The main thing I think's worth reviewing is the choice of whether or not to be consistent with CFN when removing the log_retention_in_days config option. To summarize, for CFN template generation, the log retention in days config is a property of a log group:

"MyFunc": ...,
"MyFuncLogGroup": {
  "Type": "AWS::Logs::LogGroup",
  "Properties": {
    "RetentionInDays": 7
  }
},

So when you remove this config option on a subsequent deploy, the entire MyFuncLogGroup is removed, instead of just the policy. Additionally, you can't add this config option to a cfn template if the log group is auto-created by Lambda, because this is a resource that's not managed by cfn.

So for the built-in deployer, I've made it so if you remove this config option, we just remove the retention policy on the log group, but don't actually delete the log group. You can also add this config option after the log group resource has been created.

While inconsistent, I think the built-in deployer is closer to the behavior that we want so I think this is the option we should go with.

hamishfagg commented 1 year ago

Thank you very much for your work on this, much appreciated :)

hamishfagg commented 1 year ago

Is there any chance of getting this released so we can start using it? :)