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 #21418

Merged
merged 11 commits into from
Aug 4, 2022

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.

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


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 Aug 2, 2022

@github-actions github-actions bot added the p2 label Aug 2, 2022
@aws-cdk-automation aws-cdk-automation requested a review from a team August 2, 2022 08:08
@nikovirtala
Copy link
Contributor Author

I have a problem executing the integration tests – it doesn't recognize my AWS account credential configuration and thus fails to deploy the test stack.

I tried to look around but couldn't find any hints on how the environment should be configured to make these tests work. So, what should I do?

As you can see, the CDK thinks my account and region are unknown:

yarn integ-runner --update-on-failed
yarn run v1.22.15
$ /Users/nikovirtala/src/github.com/nikovirtala/aws-cdk/node_modules/.bin/integ-runner --update-on-failed

Verifying integration test snapshots...

  NEW        integ.log-retention 0.559s
  UNCHANGED  integ.api-import 1.265s
  UNCHANGED  integ.appsync-lambda 1.324s
  UNCHANGED  integ.auth-apikey 2.514s
  UNCHANGED  integ.graphql-iam 2.495s
  UNCHANGED  integ.graphql-schema 2.51s
  UNCHANGED  integ.graphql-elasticsearch 2.536s
  UNCHANGED  integ.graphql-opensearch 2.675s
  UNCHANGED  integ.graphql 2.86s

Snapshot Results:

Tests:    1 failed, 9 total

Running integration tests for failed tests...

Running in parallel across regions: us-east-1, us-east-2, us-west-2
Running test /Users/nikovirtala/src/github.com/nikovirtala/aws-cdk/packages/@aws-cdk/aws-appsync/test/integ.log-retention.js in us-east-1
Selecting stack by identifier "IntegDefaultTestDeployAssert4E6713E1". This identifier is deprecated and will be removed in v2. Please use "Integ/DefaultTest/DeployAssert" instead.
Run "cdk ls" to see a list of all stack identifiers

✨  Synthesis time: 0.19s

AppSyncIntegLogRetention
AppSyncIntegLogRetention: deploying...

 ❌  AppSyncIntegLogRetention failed: Error: This stack uses assets, so the toolkit stack must be deployed to the environment (Run "cdk bootstrap aws://unknown-account/unknown-region")
    at Object.addMetadataAssetsToManifest (/Users/nikovirtala/src/github.com/nikovirtala/aws-cdk/packages/aws-cdk/lib/assets.ts:27:11)
    at Object.deployStack (/Users/nikovirtala/src/github.com/nikovirtala/aws-cdk/packages/aws-cdk/lib/api/deploy-stack.ts:237:29)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at CdkToolkit.deploy (/Users/nikovirtala/src/github.com/nikovirtala/aws-cdk/packages/aws-cdk/lib/cdk-toolkit.ts:219:24)
    at initCommandLine (/Users/nikovirtala/src/github.com/nikovirtala/aws-cdk/packages/aws-cdk/lib/cli.ts:347:12)

This stack uses assets, so the toolkit stack must be deployed to the environment (Run "cdk bootstrap aws://unknown-account/unknown-region")
  FAILED     integ.log-retention-Integ/DefaultTest (undefined/us-east-1) 4.26s
      Integration test failed: Error: Command exited with status 1

Test Results:

Tests:    1 failed, 1 total
Error: Some integration tests failed!
    at main (/Users/nikovirtala/src/github.com/nikovirtala/aws-cdk/packages/@aws-cdk/integ-runner/lib/cli.js:105:23)
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

@nikovirtala nikovirtala force-pushed the appsync-log-retention branch 3 times, most recently from 03273a0 to 0fa8467 Compare August 2, 2022 08:28
@mrgrain
Copy link
Contributor

mrgrain commented Aug 2, 2022

Hi @nikovirtala and thanks for looking into the integration tests! It depends a bit on how your credentials are configured locally, but I reckon the easiest way to influence the behaviour of running the test, will be env vars.

E.g. you can run it like this:

AWS_PROFILE=your_profile AWS_REGION=us-east-1 yarn integ

There are a few more variables you can set depending on your exact setup. Please let me know if that helps.

@mrgrain mrgrain changed the title feat(appsync): allow user to configure log retention time feat(aws-appsync): allow user to configure log retention time Aug 2, 2022
@mrgrain mrgrain changed the title feat(aws-appsync): allow user to configure log retention time feat(appsync): allow user to configure log retention time Aug 2, 2022
@otaviomacedo
Copy link
Contributor

@nikovirtala When I run the new integration test, the deployment fails with "The fieldLogLevel is invalid".

I believe this is a bug in the AppSync control plane. The documentation says that FieldLogLevel is optional, but if you provide a LogConfig in the CloudFormation template without passing a FieldLogLevel, you get this error. And this is what happens with your change:

"LogConfig": {
  "CloudWatchLogsRoleArn": {
    "Fn::GetAtt": [
      "GraphqlApiApiLogsRoleBB9E6BAD",
      "Arn"
    ]
  }
}

Despite being a bug in the backend, we can default the value of this field to NONE, to avoid a failure.

packages/@aws-cdk/aws-appsync/README.md Outdated Show resolved Hide resolved
@mergify mergify bot dismissed otaviomacedo’s stale review August 2, 2022 10:37

Pull request has been modified.

@nikovirtala
Copy link
Contributor Author

Despite being a bug in the backend, we can default the value of this field to NONE, to avoid a failure.

I noticed that too but was focused to get the integration tests to run against my environment. It is fixed now.

@TheRealAmazonKendra
Copy link
Contributor

If you have too much trouble getting the tests to run, we can run it for you and have you double check that the output is what you'd expect. Granted, it's much better to get your setup working so that you can run tests in the future as well, but feel free to @ me if you need some additional guidance/assistance on that.

@TheRealAmazonKendra
Copy link
Contributor

@nikovirtala If you haven't already done so, follow these instructions for your local setup Configuring the AWS CLI.

Copy link
Contributor

@TheRealAmazonKendra TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

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

Just a few small comments from me inline.

@nikovirtala
Copy link
Contributor Author

If you have too much trouble getting the tests to run, we can run it for you and have you double check that the output is what you'd expect. Granted, it's much better to get your setup working so that you can run tests in the future as well, but feel free to @ me if you need some additional guidance/assistance on that.

@TheRealAmazonKendra It's not a huge problem for me to spend some time to figure out why I cannot make the tests run – I seriously doubt that this won't be the last time I need to touch 'em here or on some other package 😉

@mrgrain gave me some ideas on what to try next at cdk.dev Slack yesterday ... going to try those next.

@mergify mergify bot dismissed TheRealAmazonKendra’s stale review August 3, 2022 07:12

Pull request has been modified.

@mergify mergify bot dismissed TheRealAmazonKendra’s stale review August 3, 2022 14:42

Pull request has been modified.

packages/@aws-cdk/aws-appsync/README.md Outdated Show resolved Hide resolved
@TheRealAmazonKendra
Copy link
Contributor

FWIW, I ran the integ tests on my machine, out of curiosity, and they all passed.

@mergify mergify bot dismissed TheRealAmazonKendra’s stale review August 4, 2022 06:37

Pull request has been modified.

@TheRealAmazonKendra
Copy link
Contributor

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Aug 4, 2022

update

☑️ Nothing to do

  • -closed [:pushpin: update requirement]
  • #commits-behind>0 [:pushpin: update requirement]

@nikovirtala
Copy link
Contributor Author

FWIW, I ran the integ tests on my machine, out of curiosity, and they all passed.

I know the test I added passes in its current state but IMO such a test without an assertion doesn't make much sense 😄

Now that I know how to successfully deploy the changed tests, I can add the assertion.

@nikovirtala
Copy link
Contributor Author

nikovirtala commented Aug 4, 2022

The integration test result is now verified using AwsApiCall - which is pretty neat feature, to be honest 👌🏼

@mergify
Copy link
Contributor

mergify bot commented Aug 4, 2022

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify mergify bot merged commit a2bb263 into aws:main Aug 4, 2022
@mergify
Copy link
Contributor

mergify bot commented Aug 4, 2022

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@nikovirtala nikovirtala deleted the appsync-log-retention branch August 5, 2022 10:04
@mrgrain mrgrain added the effort/medium Medium work item – several days of effort label Aug 17, 2022
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
effort/medium Medium work item – several days of effort p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants