-
Notifications
You must be signed in to change notification settings - Fork 123
CloudWatch Log Group not automagically created #38
Comments
Hi. You can create the log group yourself with https://www.terraform.io/docs/providers/aws/r/cloudwatch_log_group.html and Lambda will just log to that if the name matches. Does I'm in 2 minds about adding the resource to this module. Terraform can get pretty slow when there are a lot of resources in a project. On the other hand, having Lambda create log groups means there are things not managed by Terraform. But ultimately it would be nice to have |
It does fail unfortunately:
Creating the CloudWatch Log Group yourself with the same name that would be generated by the module works just fine. It doesn't actually take up a lot of time (2s creation time), though obviously it'd be another resource to create. I agree that having retention_in_days would be a nice-to-have though :-) |
I'm leaning towards yes for creating the log group resource in this module. The only problem I can think of is that enabling it might break existing deployments, because Terraform would want to create a log group that the Lambda function has already created. I suspect that it would break, but haven't tested it. Maybe it needs a |
We can always import the resource when we update to the new version. Folks ought to know to pin module versions. |
Maybe something like var.use_existing_cloudwatch_log_group? This would allow users who've already used the module to simply specify the existing CloudWatch Log Group, would allow new users to specify their own CloudWatch Log Group (for whatever reason), and if not defined would explicitly create a CloudWatch Log Group. It's a shame there's no plural data source for aws_cloudwatch_log_group allowing you to check whether the CloudWatch Log Group already exists, and make the decision based on that... edit: Speak of the devil - hashicorp/terraform-provider-aws#7928 |
I'm thinking about whether to add this as part of the next release that adds Terraform 0.12.x support. Since it's a big breaking change, we can worry less about this breaking things. But there's one thing I hadn't considered about this before: I don't necessarily want to delete the log group when deleting the Lambda function. There may be compliance issues, e.g. PCI DSS has log retention rules.
This would get around that problem, but I'm still not sure about it. So I need to think about this some more! |
Such requirements could use kinesis streaming to forward logs for retention compliance to wherever they need to store them. |
That's assuming that CloudWatch Logs is not the desired place to store them. There may be some wisdom behind the way that Lambda functions create their own log groups that never get deleted. The default behaviour is very safe. I think I would rather it just output the log group name, and then creating the log group is still pretty easy and most importantly optional: module "lambda" { ... }
resource "aws_cloudwatch_log_group" "lambda" {
name = module.lambda.log_group_name
retention_in_days = 7
} |
Wouldn't that fail if the CloudWatch Log Group already exists, because the Lambda function has somehow already been triggered? |
It would, but if you’re creating the function right next to it then it’s unlikely to have been triggered. If it were created as part of the module then I suppose we could add the log group to depends_on for all out the outputs, which would make it even less likely for something to trigger it. |
Hey all, just wanted to add I think I have a use case which encounters this issue in a 😢 way: So we have some I'm trying to create a I wouldn't mind just putting my
Is this a symptom of the same issue as is discussed in this thread? |
There's a small problem I'm having when using the module: The CloudWatch Log Group is only created when the Lambda Function tries to send its first logs. This poses a challenge when you want to use that Log Group somewhere else which relies on its existence (in my use case, Kinesis Data Stream).
I'll gladly put in a PR to create the Log Group if enable_cloudwatch_logs is true, but was just wondering whether this was something that has come up before?
Code example:
The text was updated successfully, but these errors were encountered: