feat(s3tables): add metrics configuration support for TableBucket#37275
feat(s3tables): add metrics configuration support for TableBucket#37275mergify[bot] merged 10 commits intoaws:mainfrom
Conversation
|
|
||||||||||||||
|
|
||||||||||||||
| * | ||
| * @default - Metrics are disabled | ||
| */ | ||
| readonly metricsConfiguration?: MetricsConfiguration; |
There was a problem hiding this comment.
The JSDoc here (and in the README too) can be improved by stating that this is a configuration specifically for request metrics and not all S3 table metrics: https://docs.aws.amazon.com/AmazonS3/latest/userguide/s3-tables-cloudwatch-metrics.html
The default looks good
There was a problem hiding this comment.
Done. Updated to clarify this is specifically for request metrics:
- Renamed enum to RequestMetricsStatus
- Renamed prop to requestMetricsStatus
- Updated JSDoc and README to say "request metrics"
| * | ||
| * @default MetricsConfigurationStatus.DISABLED | ||
| */ | ||
| readonly status?: MetricsConfigurationStatus; |
There was a problem hiding this comment.
status is the only property of this interface, and we are marking it optional?
Also, CDK prefers to have flat structures when there is just 1 property, so I think we have 2 options here:
- Keep this nested object, make
statusnon optional to have the user to actively choose what they want - Drop this nested object completely, and have the prop flat in the
TableBucketPropsinterface. So something like:
/**
* CloudWatch configuration for the request metrics of the table bucket.
*
* When enabled, S3 Tables publishes CloudWatch request metrics for the table bucket.
*
* @default - Request metrics are not enabled
*/
readonly requestMetricsStatus?: MetricsConfigurationStatus;And when passing them to the L1 resource in line 641, you construct there the CFN shape it needs.
I prefer option 2, seems leaner and more developer friendly, but up to you.
Any option you choose will need an improvement on the JS doc to clrearly mark that the configuration will only be applied to request metrics, and not all metrics
There was a problem hiding this comment.
Addressed with Option 2: flattened to requestMetricsStatus?:
- RequestMetricsStatus directly on props. Removed the nested interface.
- JSDoc updated to clarify this is for request metrics only.
|
Also, rosetta is failing the build, something is missing on the config so the README changes can indentify the new types you added and referenced there. |
…tus per CR feedback
☑️ The pull request is not queued |
|
@Mergifyio queue |
Merge Queue Status
Required conditions to enter a queue
|
|
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). |
Merge Queue Status
This pull request spent 3 hours 21 minutes 19 seconds in the queue, with no time running CI. ReasonThe pull request can't be updated
HintYou should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again. |
|
Hi @olenarostotskyy |
|
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). |
Merge Queue Status
This pull request spent 7 seconds in the queue, with no time running CI. Required conditions to merge
ReasonPull request #37275 has been dequeued The pull request could not be merged. This could be related to an activated branch protection or ruleset rule that prevents us from merging. At least 1 approving review is required by reviewers with write access. HintYou should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. |
|
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). |
|
@Mergifyio queue |
Merge Queue Status
This pull request spent 9 seconds in the queue, with no time running CI. Required conditions to merge
|
|
Comments on closed issues and PRs are hard for our team to see. |
Reason for this change
This PR adds request metrics configuration support to the S3 Tables TableBucket L2 construct, enabling users to enable or disable CloudWatch request metrics for their table buckets.
Request metrics provide insight into Amazon S3 Tables requests, helping users monitor and optimize their table bucket usage.
Description of changes
Enhanced TableBucket construct with request metrics configuration:
Documentation:
Description of how you validated changes
Unit tests: added 3 new test cases for request metrics (enabled, disabled, and not specified). All 193 tests passing.
Integration test: created integ.table-bucket-metrics.ts to validate metrics configuration deployment
Rosetta: verified with yarn rosetta:extract - passes successfully
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license