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(appsync): allow user to configure log retention time #20536

Closed
wants to merge 1 commit into from
Closed

feat(appsync): allow user to configure log retention time #20536

wants to merge 1 commit into from

Conversation

nikovirtala
Copy link
Contributor

This pull request implements a feature that allows users to configure the log retention period for App Sync logs.

AWS AppSync doesn't allow users to use user-defined log groups, leaving the option to create the log group for the user and set the retention time accordingly. Fortunately, the service always creates its log groups according to a pre-defined naming convention.

The log group is named following the /aws/appsync/apis/{graphql_api_id} format

ref. https://docs.aws.amazon.com/appsync/latest/devguide/monitoring.html

At the same time, AWS CDK provides a stable construct to set log retention times for any log group.

Thus, it is completely unnecessary to force every user to learn this detail when it can be abstracted to the construct creating the resource for them – that is what this pull request aims to do.


All Submissions:

Adding new Unconventional Dependencies:

  • This PR adds new unconventional dependencies following the process described here

New Features

  • Have you added the new feature to an integration test?
    • Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

@gitpod-io
Copy link

gitpod-io bot commented May 29, 2022

@aws-cdk-automation aws-cdk-automation requested a review from a team May 29, 2022 07:32
@github-actions github-actions bot added the p2 label May 29, 2022
@nikovirtala
Copy link
Contributor Author

The pull request validation workflow fails on: Error: Features must contain a change to a README file.

Currently, the README file doesn't say a word about logging 😄 – do you have any specific wishes for the content I should add there? Just a couple of words about the log retention or something generally about the logging options (and log retention)?

@corymhall
Copy link
Contributor

The pull request validation workflow fails on: Error: Features must contain a change to a README file.

Currently, the README file doesn't say a word about logging 😄 – do you have any specific wishes for the content I should add there? Just a couple of words about the log retention or something generally about the logging options (and log retention)?

I would add something like what we have for aws-lambda https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_lambda-readme.html#log-group

Copy link
Contributor

@corymhall corymhall left a comment

Choose a reason for hiding this comment

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

Looks great! I think the only thing that is missing is an integration test.

@nikovirtala
Copy link
Contributor Author

Looks great! I think the only thing that is missing is an integration test.

I guess it is time to learn to write integration tests then! There seem to be plenty of examples available, so it shouldn't be a big problem.

@TheRealAmazonKendra TheRealAmazonKendra changed the base branch from v1-main to main June 2, 2022 09:06
@mergify mergify bot dismissed corymhall’s stale review June 8, 2022 14:29

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@TheRealAmazonKendra
Copy link
Contributor

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

mergify bot pushed a commit that referenced this pull request Aug 4, 2022
This pull request implements a feature that allows users to configure the log retention period for App Sync logs.

AWS AppSync doesn't allow users to use user-defined log groups, leaving the option to create the log group for the user and set the retention time accordingly. Fortunately, the service always creates its log groups according to a pre-defined naming convention.

> The log group is named following the /aws/appsync/apis/{graphql_api_id} format

ref. https://docs.aws.amazon.com/appsync/latest/devguide/monitoring.html

At the same time, AWS CDK provides a stable [construct](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_logs.LogRetention.html) to set log retention times for any log group. 

Thus, it is completely unnecessary to force every user to learn this detail when it can be abstracted to the construct creating the resource for them – that is what this pull request aims to do.

This is the continuation of work done in #20536 – this time with documentation and integration test 😄 

----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
josephedward pushed a commit to josephedward/aws-cdk that referenced this pull request Aug 30, 2022
This pull request implements a feature that allows users to configure the log retention period for App Sync logs.

AWS AppSync doesn't allow users to use user-defined log groups, leaving the option to create the log group for the user and set the retention time accordingly. Fortunately, the service always creates its log groups according to a pre-defined naming convention.

> The log group is named following the /aws/appsync/apis/{graphql_api_id} format

ref. https://docs.aws.amazon.com/appsync/latest/devguide/monitoring.html

At the same time, AWS CDK provides a stable [construct](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_logs.LogRetention.html) to set log retention times for any log group. 

Thus, it is completely unnecessary to force every user to learn this detail when it can be abstracted to the construct creating the resource for them – that is what this pull request aims to do.

This is the continuation of work done in aws#20536 – this time with documentation and integration test 😄 

----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants