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

feat(logs): generic service managed log group to replace log retention #26947

Closed
wants to merge 1 commit into from

Conversation

mrgrain
Copy link
Contributor

@mrgrain mrgrain commented Aug 30, 2023

A general purpose implementation for a LogGroup that is usually managed by a service directly.

Supports all features a LogGroup supports.
It looks like a LogGroup, swims like a LogGroup and quacks like a LogGroup.

Replaces #26049 to close #17533
Replaces #21820 to close #21804

Currently missing features, might end up dropping them intentionally

  • logGroupRegion
  • role for custom resource
  • retry config
  • Replace all uses of LogRetention

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

@github-actions github-actions bot added the p2 label Aug 30, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team August 30, 2023 18:12
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Aug 30, 2023
@github-actions github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1 and removed p2 labels Aug 30, 2023
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a 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.

@mrgrain mrgrain force-pushed the mrgrain/feat/service-log-group branch from 9b1634f to 158b290 Compare October 6, 2023 11:48
@mrgrain mrgrain force-pushed the mrgrain/feat/service-log-group branch from 158b290 to 9350375 Compare October 6, 2023 14:38
@aws-cdk-automation
Copy link
Collaborator

The pull request linter fails with the following errors:

❌ Features must contain a change to a README file.
❌ Features must contain a change to an integration test file and the resulting snapshot.

PRs must pass status checks before we can provide a meaningful review.

If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing Exemption Request and/or Clarification Request.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 9350375
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@humanzz
Copy link
Contributor

humanzz commented Jan 17, 2024

Hi @mrgrain,
I came across this a while back through the linked issues a while back but arrived here again as I was checking the latest commits and coming across #28737 and the note abt moving from LogRetention to LogGroup but needing to use a new log group name.

The change in here seems like a great follow up to actually ease the pain of such migration - as we've accumulated data in the lambda's log group that we wouldn't want to just drop, and this seems like a good option to continue using the same name we currently have.

Is the plan for this change to still happen?

@mrgrain
Copy link
Contributor Author

mrgrain commented Jan 17, 2024

Hey @humanzz thanks for reaching out. I'm not sure how this PR would ease any migration pain. Could you elaborate on this? To me it feels like this PR would almost contribute to the opposite and reduce pressure to migrate.

Also, you should not lose any log data from the old log groups. But new data would go into the new log group. Depending on how long you typically go back in your logs and your data retention setting, this might be a quite limited transition period. For example if you have a 14days log retention policy, after 14 days you will only have a single log group again.

On a related note if you are storing logs for a really long time, you might want to consider moving long term storage away from CloudWatch Logs, as there are cheaper options available.

@humanzz
Copy link
Contributor

humanzz commented Jan 18, 2024

So, here's some context and thoughts

  • I understand that the new logGroup lambda prop can allow one to specify a new log group, and with being careful abt log retention's removal policy, I can easily end up with 2 log groups, the old original one, and the new easily fully managed one via logs.LogGroup. I'm not super keen on this option, as there are features in Lambda/Insights/etc. that I guess will simply leverage the lambda declared log group to click through (think from Lambda Console, Lambda Insights Console, etc.). From time to time I do queries over these logs, to identify some historical trends, etc. I'm not saying it's impossible to do with 2 log groups, I'm just saying it becomes less convenient. We keep our logs for 18 months in general based on recommendations from ORRs/Security Reviews. Multiply that by working on a dozen lambda functions, and having 2 log groups for each function to keep that history becomes even more "less inconvenient"
  • I've noticed that in your implementation here class ServiceManagedLogGroup extends LogGroupBase implements ILogGroup, while FunctionOptions.logGroup is an ILogGroup. To me, this suggested that ServiceManagedLogGroup can be used as a value for logGroup if someone wants to customize the service managed log group, or a concrete LogGroup. Using the ServiceManagedLogGroup can therefore help us address the logRetention deprecation, while still keeping the same log group we have.
  • ServiceManagedLogGroup does seem like a good idea, as it's not just about Lambda - a service that manages its log group by default - but can be used in other L2 constructs for services that manage their own log groups - and like my point above, can really have a smooth path/consistent experience for when such services might introduces a similar support of specifying the log group to use

@humanzz
Copy link
Contributor

humanzz commented Jan 29, 2024

Hey @mrgrain, just wondering if u got a chance to checkout my earlier comment and if u have any thoughts abt it?

@polothy
Copy link
Contributor

polothy commented Jan 29, 2024

Likely unrelated'ish to this PR, but wanted to just say that LoggingConfig for Lambda isn't available in GovCloud yet. Please don't rush to change all the lambdas that CDK core defines to use the new LoggingConfig because it'll break in GovCloud. Maybe other partitions as well.

EDIT: I tried this last Friday to fix the CDK deprecations and this is the error I got:

Resource handler returned message: "LoggingConfig is not supported in us-gov-west-1. Remove LoggingConfig value from your request and try again

@mrgrain
Copy link
Contributor Author

mrgrain commented Jan 29, 2024

So, here's some context and thoughts

  • I understand that the new logGroup lambda prop can allow one to specify a new log group, and with being careful abt log retention's removal policy, I can easily end up with 2 log groups, the old original one, and the new easily fully managed one via logs.LogGroup. I'm not super keen on this option, as there are features in Lambda/Insights/etc. that I guess will simply leverage the lambda declared log group to click through (think from Lambda Console, Lambda Insights Console, etc.). From time to time I do queries over these logs, to identify some historical trends, etc. I'm not saying it's impossible to do with 2 log groups, I'm just saying it becomes less convenient. We keep our logs for 18 months in general based on recommendations from ORRs/Security Reviews. Multiply that by working on a dozen lambda functions, and having 2 log groups for each function to keep that history becomes even more "less inconvenient"

That's very understandable. But this also implies you will never be able to migrate due to this inconvenience. (Which is fine, it should 100% be your choice.)

  • I've noticed that in your implementation here class ServiceManagedLogGroup extends LogGroupBase implements ILogGroup, while FunctionOptions.logGroup is an ILogGroup. To me, this suggested that ServiceManagedLogGroup can be used as a value for logGroup if someone wants to customize the service managed log group, or a concrete LogGroup. Using the ServiceManagedLogGroup can therefore help us address the logRetention deprecation, while still keeping the same log group we have.

If completed that would have worked. Maybe. There would have still be a circular dependency.

  • ServiceManagedLogGroup does seem like a good idea, as it's not just about Lambda - a service that manages its log group by default - but can be used in other L2 constructs for services that manage their own log groups - and like my point above, can really have a smooth path/consistent experience for when such services might introduces a similar support of specifying the log group to use

We will definitely keep considering this given that other services don't have the flexibility lambda offers. Please open a new issue for the specific services you are interested in so we can prioritize this appropriately.


Re Migration

Since all your well-named log groups already exists, you could important them into your stack using the existing name.
But I'm not sure if that is much less work.


Fundamentally the issue with this comes down to Function name being part of Log group name. This creates a circular dependency that can only be resolved in of two ways:

  • Not have the Function name in Log group name
  • Allow to pass-through all features that a log group supports via the Lambda function

The Lambda team decided to go with the first option. While I personally agree with this choice anyway, it also very much means that the CDK will have to go with this choice. You are rightfully asking us to maintain the second option as a mitigation to complex migration. That's a fair ask, but I can't tell you what the likelihood is that we will do this. In general we are reluctant to create duplication with native CFN features.

However none of that answers your very valid concerns about migration. So we'll have to think more about this.

@mrgrain
Copy link
Contributor Author

mrgrain commented Jan 29, 2024

Likely unrelated'ish to this PR, but wanted to just say that LoggingConfig for Lambda isn't available in GovCloud yet. Please don't rush to change all the lambdas that CDK core defines to use the new LoggingConfig because it'll break in GovCloud. Maybe other partitions as well.

EDIT: I tried this last Friday to fix the CDK deprecations and this is the error I got:

Resource handler returned message: "LoggingConfig is not supported in us-gov-west-1. Remove LoggingConfig value from your request and try again

Well 💩 That's one is on me. Can you please open a new issue @polothy

@mrgrain
Copy link
Contributor Author

mrgrain commented Jan 29, 2024

@polothy I'm trying to get confirmation if LoggingConfig is supposed to be available in GovtCloud or not. If you can and don't mind, I'd also recommend you put a support ticket in. I have so far found the announcement that states "all commercial regions" and a blog post that states "all regions". 😐

KmsKeyId: this.props.encryptionKey?.keyArn,
RetentionInDays: this.props.retention,
Tags: this.tags.renderedTags,
Tagging: options.tagging,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to get tags from the CloudFormation stack? We stopped using tags in our templates and moved to tagging stacks due to some resources not allowing tag updates and/or differences with GovCloud.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's one reason why this PR isn't finished. 😓

@polothy
Copy link
Contributor

polothy commented Jan 29, 2024

@polothy I'm trying to get confirmation if LoggingConfig is supposed to be available in GovtCloud or not. If you can and don't mind, I'd also recommend you put a support ticket in. I have so far found the announcement that states "all commercial regions" and a blog post that states "all regions". 😐

I'm working with our AWS TAM to check into it and add our +1 to it going to GovCloud.

@polothy
Copy link
Contributor

polothy commented Jan 29, 2024

Well 💩 That's one is on me. Can you please open a new issue @polothy

Created #28919

@mrgrain
Copy link
Contributor Author

mrgrain commented Jan 29, 2024

@polothy I'm trying to get confirmation if LoggingConfig is supposed to be available in GovtCloud or not. If you can and don't mind, I'd also recommend you put a support ticket in. I have so far found the announcement that states "all commercial regions" and a blog post that states "all regions". 😐

I'm working with our AWS TAM to check into it and add our +1 to it going to GovCloud.

I've got confirmation that LoggingConfig is not available in GovCloud yet. Your TAM will be your best path to get a date.

@mrgrain
Copy link
Contributor Author

mrgrain commented Jan 31, 2024

Reverting the deprecation: #28934

@mrgrain mrgrain closed this May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS. effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1
Projects
None yet
4 participants