-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat(appsync): add EnhancedMetricsConfigProperty for GraphQL api #35328
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
base: main
Are you sure you want to change the base?
Conversation
6566b24 to
d62f669
Compare
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.
Thank you for the contribution.
Before reviewing the details, let me confirm.
Don’t we need to add the metricConfig property to Resolver and DataSource?
If the API’s config is set to per-resolver metrics, I think the resolver’s config is also needed.
|
There’s an existing issue, please associate it. |
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.
Thank you for you contribution! I've added some minor comments.
| }); | ||
|
|
||
| api.addNoneDataSource('NoneDS', { | ||
| name: cdk.Lazy.string({ produce(): string { return 'NoneDS'; } }), |
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.
Why is it necessary to use Lazy here?
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.
@badmintoncryer
Thanks.
Thinking it over again, I found I didn't need to use Lazy.
| new appsync.GraphqlApi(stack, 'minimal-enhanced-metrics', { | ||
| name: 'enhanced-metrics', | ||
| schema: appsync.SchemaFile.fromAsset(path.join(__dirname, 'appsync.test.graphql')), | ||
| enhancedMetricsConfig: {}, |
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.
Do other constructs also have arguments where we can pass {} like this?
Personally, I feel that having settings activated as a result of passing {} is not very intuitive.
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.
I found that the appsync.GraphqlApi.logConfig property was passing {}, but I think also passing {} is not very intuitive.
Suggest requiring the dataSourceLevelMetricsBehavior and resolverLevelMetricsBehavior property, because after setting those property the behavior changes.
ex)
EnhancedMetricsConfig {
readonly dataSourceLevelMetricsBehavior: DataSourceLevelMetricsBehavior;
readonly operationLevelMetricsEnabled?: boolean;
readonly resolverLevelMetricsBehavior: ResolverLevelMetricsBehavior;
}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.
Some of the existing arguments can also accept empty objects! In that case, I don’t think it’s a big issue, so please feel free to implement it in whichever way you prefer!
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.
@badmintoncryer
Implemented dataSourceLevelMetricsBehavior and resolverLevelMetricsBehavior as required properties.
@mazyu36 |
a2a41a7 to
d8b65ca
Compare
d8b65ca to
af5940a
Compare
…s for data source and resolver
af5940a to
4b19182
Compare
02248fb to
3f4abf1
Compare
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.
Thanks! I've added some minor comments.
Co-authored-by: Kazuho Cryer-Shinozuka <[email protected]>
Co-authored-by: Kazuho Cryer-Shinozuka <[email protected]>
Co-authored-by: Kazuho Cryer-Shinozuka <[email protected]>
Co-authored-by: Kazuho Cryer-Shinozuka <[email protected]>
Co-authored-by: Kazuho Cryer-Shinozuka <[email protected]>
Thanks! |
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.
Thanks!
Co-authored-by: Kazuho Cryer-Shinozuka <[email protected]>
Co-authored-by: Kazuho Cryer-Shinozuka <[email protected]>
Issue # (if applicable)
Closes #29933 .
Reason for this change
A property in L1 construct was not in L2 construct.
Description of changes
Add
enhancedMetricsConfigproperty toGraphqlApiPropsand set in theCfnGraphQLApiconstructor.enhanced Metrics Confignow includes theoperationLevelMetricsEnabledproperty, which has changed from an enum of "ENABLED" and "DISABLED" to a boolean.EnhancedMetricsConfigare not flattened because they interact with each other.For example, if
operationLevelMetricsConfigis enabled,dataSourceLevelMetricsBehaviorwill default toPER_DATA_SOURCE_METRICS.Describe any new or updated permissions being added
Description of how you validated changes
Added both unit and integration tests.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license