feat(rds): add support for monitoring to DatabaseCluster #2828

rpanfili · 2019-06-11T20:39:17Z

Lets a user specify a monitoring interval period. This change will also auto-generate a valid Role to manage DB instances monitoring.

This change adds an optional prop monitoringInterval

closes #2826

BREAKING CHANGE:
* rds: monitoringInterval in DatabaseInstanceNewProps has been renamed to monitoringIntervalSec

Pull Request Checklist

Testing
- Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
- CLI change?: coordinate update of integration tests with team
- cdk-init template change?: coordinated update of integration tests with team
Docs
- jsdocs: All public APIs documented
- README: README and/or documentation topic updated
- Design: For significant features, design document added to design folder
Title and Description
- Change type: title prefixed with fix, feat and module name in parens, which will appear in changelog
- Title: use lower-case and doesn't end with a period
- Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
- Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
Sensitive Modules (requires 2 PR approvers)
- IAM Policy Document (in @aws-cdk/aws-iam)
- EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
- Grant APIs (only if not based on official documentation with a reference)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

rix0rrr · 2019-06-12T12:07:47Z

packages/@aws-cdk/aws-rds/lib/cluster.ts

+   *
+   * @default no enhanced monitoring
+   */
+  readonly monitoringInterval?: number;


Please annotate this property with a unit. Recommended: monitoringIntervalSec.

Although @RomainMuller is currently working on a Duration class which will be the recommended practice going forward.

could be also adapted in DatabaseInstance then.

https://github.com/awslabs/aws-cdk/blob/e0718ef7cc26798ec2401451be9d7b7894f0b8bc/packages/%40aws-cdk/aws-rds/lib/instance.ts#L375

(breaking)

I renamed that property in DatabaseClusterProps and in DatabaseInstanceNewProps as suggested.
I also added the Breaking advise and rebased to master!

@rix0rrr I've seen that the Duration class has landed on master branch so I updated this PR with those required changes. Let me know if you need anything else!

rix0rrr · 2019-06-12T12:08:51Z

Other than that, looks good!

rix0rrr · 2019-06-21T14:51:14Z

Needs a merge from master, unfortunately :(

rpanfili · 2019-06-21T16:20:12Z

@rix0rrr rebased!

rpanfili · 2019-06-24T10:07:54Z

@rix0rrr rebased!

and rebased again :)

skinny85

Looks great! Just one tiny addition :)

skinny85 · 2019-08-07T00:40:50Z

packages/@aws-cdk/aws-rds/lib/cluster.ts

+
+    let monitoringRole;
+    if (props.monitoringInterval && props.monitoringInterval.toSeconds()) {
+      monitoringRole = new Role(this, "MonitoringRole", {


It's nice that we default this, but can we also allow specifying this in props? monitoringRole is probably a good name for that property.

mergify · 2019-09-11T10:10:54Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

jogold · 2019-09-11T10:32:36Z

packages/@aws-cdk/aws-rds/lib/cluster.ts

+
+    let monitoringRole;
+    if (props.monitoringInterval && props.monitoringInterval.toSeconds()) {
+      monitoringRole = props.monitoringRole || new Role(this, "MonitoringRole", {


It would be nice to have the same optional monitoringRole for an instance.

Let a user define a monitoring interval period. It will be also possible to specify a monitoring IAM Role to manage instances monitoring, otherwise a valid Role will be auto-generated. This change adds optional props `monitoringInterval` and `monitoringRole` to `DatabaseClusterProps` and optional `monitoringRole` to `DatabaseInstanceProps` closes aws#2826

skinny85

Thanks for the contribution @rpanfili , and apologies for approving this so late!

mergify · 2019-10-03T21:59:14Z

Thank you for contributing! Your pull request is now being automatically merged.

aws-cdk-automation · 2019-10-03T23:22:22Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

rpanfili requested a review from a team as a code owner June 11, 2019 20:39

rpanfili changed the title ~~feat(rds): add support for monitoring to database cluster~~ feat(rds): add support for monitoring to DatabaseCluster Jun 11, 2019

rix0rrr reviewed Jun 12, 2019

View reviewed changes

rpanfili force-pushed the master branch 8 times, most recently from b5e78f4 to 6e626c5 Compare June 20, 2019 16:25

rpanfili force-pushed the master branch from 6e626c5 to 90530f9 Compare June 21, 2019 15:33

rpanfili force-pushed the master branch from 90530f9 to 9199027 Compare June 24, 2019 08:46

NGL321 assigned skinny85 Aug 1, 2019

skinny85 reviewed Aug 7, 2019

View reviewed changes

skinny85 added the small-review label Aug 7, 2019

rpanfili force-pushed the master branch from 9199027 to f10504c Compare September 11, 2019 10:10

mergify bot added the automerge-enabled label Sep 11, 2019

jogold reviewed Sep 11, 2019

View reviewed changes

rpanfili force-pushed the master branch from f10504c to 06f1bdb Compare September 11, 2019 10:49

NGL321 removed the automerge-enabled label Sep 13, 2019

mergify bot added the automerge-enabled label Sep 13, 2019

NGL321 removed the automerge-enabled label Sep 13, 2019

skinny85 approved these changes Oct 3, 2019

View reviewed changes

Merge branch 'master' into master

0494ced

mergify bot merged commit 910c8bf into aws:master Oct 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rds): add support for monitoring to DatabaseCluster #2828

feat(rds): add support for monitoring to DatabaseCluster #2828

rpanfili commented Jun 11, 2019 •

edited

Loading

rix0rrr Jun 12, 2019

jogold Jun 12, 2019

rpanfili Jun 14, 2019

rpanfili Jun 21, 2019

rix0rrr commented Jun 12, 2019

rix0rrr commented Jun 21, 2019

rpanfili commented Jun 21, 2019

rpanfili commented Jun 24, 2019

skinny85 left a comment

skinny85 Aug 7, 2019

rpanfili Sep 11, 2019

mergify bot commented Sep 11, 2019

jogold Sep 11, 2019

rpanfili Sep 11, 2019

skinny85 left a comment

mergify bot commented Oct 3, 2019

aws-cdk-automation commented Oct 3, 2019

feat(rds): add support for monitoring to DatabaseCluster #2828

feat(rds): add support for monitoring to DatabaseCluster #2828

Conversation

rpanfili commented Jun 11, 2019 • edited Loading

Pull Request Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr commented Jun 12, 2019

rix0rrr commented Jun 21, 2019

rpanfili commented Jun 21, 2019

rpanfili commented Jun 24, 2019

skinny85 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Sep 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinny85 left a comment

Choose a reason for hiding this comment

mergify bot commented Oct 3, 2019

aws-cdk-automation commented Oct 3, 2019

AWS CodeBuild CI Report

rpanfili commented Jun 11, 2019 •

edited

Loading