-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat(dynamodb): operation-specific convenience methods for successful request latency metrics #35527
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
…sfulRequestLatency metric in TableV2 - Add metricSuccessfulRequestLatencyForGetItem() - Add metricSuccessfulRequestLatencyForPutItem() - Add metricSuccessfulRequestLatencyForQuery() - Add metricSuccessfulRequestLatencyForScan() - Add metricSuccessfulRequestLatencyForOperation(operation: string) - Update ITable interface with optional methods for consistency - Add comprehensive unit tests for all new methods Fixes aws#34785: .NET CDK users can now create SuccessfulRequestLatency metrics without manually specifying Operation dimension through dimensionsMap
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.
(This review is outdated)
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
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.
let's fix them
[jsii-rosetta] [INFO] Loading 36 assemblies
[jsii-rosetta] [INFO] Translating
[jsii-rosetta] [INFO] Preparing dependency closure at /tmp/rosetta4U5BW8 (-vv for more details)
[jsii-rosetta] [INFO] Translating 3241 snippets using 16 workers
[jsii-rosetta] [INFO] Translated 3241 snippets in 681.724 seconds (0.210s/snippet)
[jsii-rosetta] [INFO] Adding translations to /root/.s3buildcache/rosetta-cache.tabl.json
Error: aws-cdk-lib.aws_dynamodb-README-L966.ts:27:5 - error TS2304: Cannot find name 'cloudwatch'.
27 new cloudwatch.Alarm(this, 'GetItemLatencyAlarm', {
~~~~~~~~~~
Error: aws-cdk-lib.aws_dynamodb-README-L966.ts:33:5 - error TS2304: Cannot find name 'cloudwatch'.
33 new cloudwatch.Alarm(this, 'QueryLatencyAlarm', {
~~~~~~~~~~
Error: aws-cdk-lib.aws_dynamodb-README-L996.ts:13:29 - error TS2304: Cannot find name 'table'.
13 const customLatencyMetric = table.metricSuccessfulRequestLatencyForGetItem({
~~~~~
[jsii-rosetta] [WARN] 3 diagnostics from assemblies with 'strict' mode on (and 75 more non-strict diagnostics)
/// !cdk-integ dynamodb-v2-alarm-metrics | ||
|
||
/** | ||
* Integration Test: DynamoDB TableV2 SuccessfulRequestLatency Convenience Methods |
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.
This all doesn't need to be in the code that we submit.
* | ||
* @param props properties of a metric | ||
*/ | ||
metricSuccessfulRequestLatencyForGetItem?(props?: cloudwatch.MetricOptions): cloudwatch.Metric; |
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.
These are not necessary anymore if we have the generic one below.
* @param operation The DynamoDB operation to get metrics for | ||
* @param props properties of a metric | ||
*/ | ||
metricSuccessfulRequestLatencyForOperation?(operation: string, props?: cloudwatch.MetricOptions): cloudwatch.Metric; |
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.
Better yet, this could just be metricSuccessfulRequestLatencyV2
, deprecating the old one.
The lack of ability to pass in an operation could be seen as a serious defect that needs to be rectified.
Please tell me we at least had TableName
in there already?
Issue # (if applicable)
Closes #34785.
Reason for this change
The
metricSuccessfulRequestLatency()
method in TableV2 throws a validation error requiring an "Operation" dimension, but .NET CDK users cannot easily provide this dimension through the available options. This creates a usability issue where .NET developers cannot create SuccessfulRequestLatency metrics for DynamoDB TableV2 instances without complex workarounds.The root cause is a cross-language compatibility limitation with JSII (JavaScript Interop Interface). While TypeScript users can easily pass complex
dimensionsMap
objects, .NET users struggle with the JSII translation of nested object structures, making it difficult or impossible to provide the requiredOperation
dimension.Description of changes
Added 5 JSII-friendly convenience methods to TableV2Base class for creating SuccessfulRequestLatency metrics:
metricSuccessfulRequestLatencyForGetItem(props?: MetricOptions): Metric
metricSuccessfulRequestLatencyForPutItem(props?: MetricOptions): Metric
metricSuccessfulRequestLatencyForQuery(props?: MetricOptions): Metric
metricSuccessfulRequestLatencyForScan(props?: MetricOptions): Metric
metricSuccessfulRequestLatencyForOperation(operation: string, props?: MetricOptions): Metric
These methods internally handle the required Operation dimension, providing a clean API for cross-language CDK users. The implementation follows the existing
metricXxxForOperation
pattern used throughout the CDK DynamoDB module and extends the ITable interface for consistency between Table and TableV2.Each method internally calls
metricSuccessfulRequestLatencyForOperation()
with the appropriate operation string, which constructs the properdimensionsMap
with bothTableName
andOperation
dimensions before delegating to the existing metric infrastructure.Describe any new or updated permissions being added
N/A - No IAM permissions changes. This is a client-side metric creation enhancement that uses existing CloudWatch metric functionality.
Description of how you validated changes
integ.dynamodb-v2.alarm-metrics.ts
that validates CloudFormation template generation for all 5 new methods, confirming proper AWS::CloudWatch::Alarm resource creation with correct dimensions, namespace, and statisticsThe integration test generates 6 AWS::CloudWatch::Alarm resources with proper SuccessfulRequestLatency metrics, each containing correct
Operation
dimensions (GetItem, PutItem, Query, Scan, BatchGetItem) and AWS/DynamoDB namespace configuration.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license