feat(rds): support Database Insights for RDS instances #34854
Conversation
github-actions
bot
added
effort/medium
|
|
||
| const enablePerformanceInsights = props.enablePerformanceInsights | ||
| || props.performanceInsightRetention !== undefined || props.performanceInsightEncryptionKey !== undefined; | ||
| if (enablePerformanceInsights && props.enablePerformanceInsights === false) { |
There was a problem hiding this comment.
I consolidated the validation into the following.
aws-cdk-automation
added
the
pr/needs-community-review
| }); | ||
| }); | ||
|
|
||
| test('instance with the standard mode of database insights', () => { |
There was a problem hiding this comment.
Just to be safe, could you please add a test to verify that databaseInsightsMode.STANDARD can be set even when Performance Insights is disabled?
aws-cdk-automation
removed
the
pr/needs-community-review
Co-authored-by: Kazuho Cryer-Shinozuka <[email protected]>
|
Thanks always! |
aws-cdk-automation
added
the
pr/needs-maintainer-review
mazyu36
changed the title
| * Validates database instance properties | ||
| */ | ||
| export function validateDatabaseInstanceProps(scope: Construct, props: DatabaseInstanceProps): void { | ||
| validateAllProps(scope, DatabaseInstance.name, props, databaseInsightsRules as ValidationRule<DatabaseInstanceProps>[]); |
There was a problem hiding this comment.
Can we add a new validation rule to validate that Standard mode of Database Insights supports only a retention period of 7 days, as this will be the behavior after November 30 2025 ? See the warning: https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/USER_PerfInsights.Enabling.html
(We should have done the same for validateDatabseClusterProps but that would be a breaking change)
There was a problem hiding this comment.
In my opinion, this should be a warning since users can set flexible retention periods until November 30, 2025, and some users may have already implemented this for RDS Instances using Escape Hatches.
If a warning is appropriate, it should apply to both RDS Clusters and Instances, so I will implement this in a new PR.
What do you think?
There was a problem hiding this comment.
This is a good callout, let's not add a validation rule. Regarding the warning, this is an option, another one would be to simply add it in the description block and when the new behavior kicks in at the end of November, we can evaluate if these props should be deprecated.
|
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
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). |
|
Comments on closed issues and PRs are hard for our team to see. |
4 participants
Issue # (if applicable)
Closes #34743
Reason for this change
Database insights for Aurora Cluster was previously implemented in #32851.
Database insights is also supported for RDS instances, but the current L2 construct
does not support it.
https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/Database-Insights.html
Description of changes
Describe any new or updated permissions being added
N/A
Description of how you validated changes
Add unit tests and integ tests.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license