-
Notifications
You must be signed in to change notification settings - Fork 79
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
perf(graphql-model-transformer): minimal provider framework and inline policies #2490
Conversation
...-model-transformer/src/__tests__/__snapshots__/amplify-dynamodb-table-generator.test.ts.snap
Show resolved
Hide resolved
c5c58bf
to
b309ca2
Compare
packages/amplify-graphql-model-transformer/src/resources/dynamo-model-resource-generator.ts
Show resolved
Hide resolved
...ransformer/src/resources/amplify-dynamodb-table/amplify-table-manager-lambda/cfn-response.ts
Show resolved
Hide resolved
...el-transformer/src/resources/amplify-dynamodb-table/amplify-table-manager-lambda/outbound.ts
Show resolved
Hide resolved
...plify-graphql-model-transformer/src/resources/amplify-dynamodb-table/waiter-state-machine.ts
Outdated
Show resolved
Hide resolved
packages/amplify-graphql-model-transformer/src/resources/dynamo-model-resource-generator.ts
Show resolved
Hide resolved
...ransformer/src/resources/amplify-dynamodb-table/amplify-table-manager-lambda/cfn-response.ts
Show resolved
Hide resolved
// The contents of this file were taken from the AWS CDK provider framework. | ||
// https://github.com/aws/aws-cdk/blob/c52ff08/packages/aws-cdk-lib/custom-resources/lib/provider-framework/runtime/outbound.ts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modifications:
- Removed
defaultInvokeFunction
/invokeFunction
. These are used by the CDK's provider framework to invoke the Lambda functions we defined. The new design doesn't involve Lambda functions invoking other Lambda functions.
...-model-transformer/src/resources/amplify-dynamodb-table/amplify-table-manager-lambda/util.ts
Show resolved
Hide resolved
Co-authored-by: Zeyu Li <[email protected]>
// The contents of this file were adapted from the AWS CDK provider framework. | ||
// https://github.com/aws/aws-cdk/blob/c52ff08cfd1515d35feb93bcba34a3231a94985c/packages/aws-cdk-lib/custom-resources/lib/provider-framework/waiter-state-machine.ts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modifications:
- 'framework-onTimeout-task': {
- End: true,
- Type: 'Task',
- Resource: props.timeoutHandler.functionArn,
- },
We're depending on the CloudFormation custom resource default (and unchangeable) timeout of 1 hour. We no longer need a timeout function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it simplify our future maintenance (by making it easier to compare changes from the CDK) to leave this unchanged, and simply add a noop timeout handler and default timeout value high enough to never be invoked?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed that it would likely help with maintainability, but that comes at the cost of the "fake" timeout value and timeout task state (framework-onTimeout-task
) being potentially misinterpreted by future readers as "real."
I don't have a strong opinion either way. If you think it's worthwhile to add back, happy to do it.
packages/amplify-graphql-model-transformer/src/resources/dynamo-model-resource-generator.ts
Show resolved
Hide resolved
...-transformer/src/resources/amplify-dynamodb-table/amplify-dynamo-model-resource-generator.ts
Outdated
Show resolved
Hide resolved
...-transformer/src/resources/amplify-dynamodb-table/amplify-dynamo-model-resource-generator.ts
Show resolved
Hide resolved
...sources/amplify-dynamodb-table/amplify-table-manager-lambda/amplify-table-manager-handler.ts
Show resolved
Hide resolved
...sources/amplify-dynamodb-table/amplify-table-manager-lambda/amplify-table-manager-handler.ts
Outdated
Show resolved
Hide resolved
...sources/amplify-dynamodb-table/amplify-table-manager-lambda/amplify-table-manager-handler.ts
Show resolved
Hide resolved
...sources/amplify-dynamodb-table/amplify-table-manager-lambda/amplify-table-manager-handler.ts
Show resolved
Hide resolved
packages/amplify-graphql-model-transformer/src/resources/amplify-dynamodb-table/provider.ts
Outdated
Show resolved
Hide resolved
packages/amplify-graphql-model-transformer/src/resources/amplify-dynamodb-table/provider.ts
Show resolved
Hide resolved
// The contents of this file were adapted from the AWS CDK provider framework. | ||
// https://github.com/aws/aws-cdk/blob/c52ff08cfd1515d35feb93bcba34a3231a94985c/packages/aws-cdk-lib/custom-resources/lib/provider-framework/waiter-state-machine.ts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it simplify our future maintenance (by making it easier to compare changes from the CDK) to leave this unchanged, and simply add a noop timeout handler and default timeout value high enough to never be invoked?
Description of changes
Deploy Time Improvements
Deployment time depends on many factors outside our direct control. That said, multiple runs have demonstrated fairly precise benchmarks.
Simple Todo (
npm create amplify@latest
)Current: ✨ Total time: 254.85s
With Changes: ✨ Total time: 178.64s
Schema
Lots of GSIs w/ Composite Keys
Current: ✨ Total time: 514.53s
With Changes: ✨ Total time: 249.24s
Schema
Types, Models, and Relationships
Current: ✨ Total time: 282.97s
With Changes: ✨ Total time: 200.23s
Schema
Changes
1. Replace usage of AWS CDK's custom resource provider framework by
Custom::AmplifyDynamoDBTable
with a minimal provider withinamplify-graphql-model-transformer
Extension / replacement of (see for context):
Current Architecture
Currently, our usage of the CDK's
CustomResources.Provider
construct leads to excessive resources that slow down initial deployment times. For theCustom::AmplifyDynamoDBTable
custom resource, we deploy the following resources, which takes approximately 2 minutes on the initial deploy.AWS::IAM::Role
AWS::IAM::Policy
AWS::Lambda::Function
AWS:StepFunctions::StateMachine
New Architecture
With this change, we're reducing that custom resource supporting infrastructure to the minimum needed. This reduces the initial deploy time of the custom resource provider resources by approximately 60 seconds:
AWS::IAM::Role
AWS::IAM::Policy
AWS::Lambda::Function
AWS:StepFunctions::StateMachine
2. Prevent default policy generation for IAM roles generated per model for DynamoDB table access by AppSync
The IAM role created for AppSync to access DynamoDB tables currently has two policies attached to it.
DynamoDBAccess
we define when creating the role.The second policy is unnecessary and generated with its own Logical ID in the CFN stack. This leads to an additional ~15-20 seconds deployment time per model. Because resources are generated in parallel, this doesn't save 15 * models.count seconds, but it does save approximately 25 seconds for the initial deployment of a typical schema with 15-20 models.
This change consolidates those two policies into a single inline policy, and prevents the CDK from generating a default policy via the
withoutPolicyUpdates()
method.See Opting out of automatic permissions management for more information.
CDK / CloudFormation Parameters Changed
Issue #, if available
N/A
Description of how you validated changes
Checklist
yarn test
passesRelevant documentation is changed or added (and PR referenced)New AWS SDK calls or CloudFormation actions have been added to relevant test and service IAM policiesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.