feat(cloudwatch): add verticalAnnotations property to GraphWidget #22568

brendo-m · 2022-10-19T22:25:31Z

Adds a verticalAnnotation property to GraphWidget. See https://docs.aws.amazon.com/AmazonCloudWatch/latest/APIReference/CloudWatch-Dashboard-Body-Structure.html#CloudWatch-Dashboard-Properties-Annotation-Format.

Also see #7622

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

gitpod-io · 2022-10-20T15:12:46Z

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

TheRealAmazonKendra

Thank you for your contribution! This looks very close to being ready to go. Just a few comments inline.

TheRealAmazonKendra · 2022-10-21T04:19:51Z

packages/@aws-cdk/aws-cloudwatch/lib/graph.ts

+  /**
+   * The date and time in the graph where the vertical annotation line is to appear
+   */
+  readonly value: string;


If this is supposed to be a timestamp, I think we should type it that way instead of as a string.

Also, should this be called date instead?

TheRealAmazonKendra · 2022-10-21T04:46:27Z

packages/@aws-cdk/aws-cloudwatch/lib/graph.ts

@@ -413,6 +420,11 @@ export class GraphWidget extends ConcreteWidget {
      ...(this.props.leftAnnotations || []).map(mapAnnotation('left')),
      ...(this.props.rightAnnotations || []).map(mapAnnotation('right')),
    ];
+    const verticalAnnotations = this.props.verticalAnnotations || [];


This line doesn't seem needed. When you check against verticalAnnotations.length you could instead just check against this.props.verticalAnnotations?.length > 0.

that doesn't compile, and we are actually referencing verticalAnnotations 3 times so this seems cleaner

TheRealAmazonKendra · 2022-10-21T04:47:24Z

packages/@aws-cdk/aws-cloudwatch/test/integ.alarm-and-dashboard.ts

@@ -53,6 +53,13 @@ dashboard.addWidgets(new cloudwatch.GraphWidget({
  title: 'More messages in queue with alarm annotation',
  left: [numberOfMessagesVisibleMetric],
  leftAnnotations: [alarm.toAnnotation()],
+  verticalAnnotations: [


I'd prefer to see a new test that includes these annotations instead of editing an existing test.

may I ask why? that would involve a whole lot of boilerplate setting up a new test, this was trivial to add

Because when we pile too much functionality into one test, it's harder to decipher if we're getting the output we're looking for and harder to troubleshoot when a test fails.

Also, was this test run and deployed? If so, I'd expect to see changes to more than these two files. It appears that this was run with --dry-run or that it was manually edited.

aws-cdk-automation · 2022-11-01T15:02:10Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2022-11-01T15:02:11Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2022-11-02T00:11:06Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2022-11-02T16:08:53Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

Pull request has been modified.

TheRealAmazonKendra

Back into changes requested for the integ test update I requested.

TheRealAmazonKendra · 2022-11-02T17:25:46Z

packages/@aws-cdk/aws-cloudwatch/test/integ.alarm-and-dashboard.ts

@@ -53,6 +53,13 @@ dashboard.addWidgets(new cloudwatch.GraphWidget({
  title: 'More messages in queue with alarm annotation',
  left: [numberOfMessagesVisibleMetric],
  leftAnnotations: [alarm.toAnnotation()],
+  verticalAnnotations: [


Also, was this test run and deployed? If so, I'd expect to see changes to more than these two files. It appears that this was run with --dry-run or that it was manually edited.

aws-cdk-automation · 2022-11-02T17:32:21Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 19dd676
Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2022-11-24T00:09:51Z

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

aws-cdk-automation · 2022-12-01T00:10:27Z

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

mschlaipfer · 2023-06-16T10:20:33Z

Would be great to have vertical annotations in CDK widgets.

github-actions bot added the p2 label Oct 19, 2022

aws-cdk-automation requested a review from a team October 19, 2022 22:25

github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Oct 19, 2022

This was referenced Oct 19, 2022

feat(cloudwatch): add verticalAnnotations property to GraphWidget #15809

Closed

Vertical annotations for CloudWatch GraphWidgets #7622

Closed

aws-cdk-automation previously requested changes Oct 19, 2022

View reviewed changes

brendo-m force-pushed the vertical-annotation branch 2 times, most recently from f6ca465 to 2054e46 Compare October 20, 2022 15:12

TheRealAmazonKendra previously requested changes Oct 21, 2022

View reviewed changes

TheRealAmazonKendra changed the title ~~feat(aws-cloudwatch): add verticalAnnotations property to GraphWidget~~ feat(cloudwatch): add verticalAnnotations property to GraphWidget Oct 21, 2022

brendo-m force-pushed the vertical-annotation branch from 2054e46 to d759a24 Compare November 2, 2022 16:14

feat(aws-cloudwatch): add verticalAnnotations property to GraphWidget

19dd676

brendo-m force-pushed the vertical-annotation branch from d759a24 to 19dd676 Compare November 2, 2022 16:54

TheRealAmazonKendra requested changes Nov 2, 2022

View reviewed changes

aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Dec 1, 2022

aws-cdk-automation closed this Dec 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cloudwatch): add verticalAnnotations property to GraphWidget #22568

feat(cloudwatch): add verticalAnnotations property to GraphWidget #22568

brendo-m commented Oct 19, 2022 •

edited

Loading

aws-cdk-automation left a comment

gitpod-io bot commented Oct 20, 2022

TheRealAmazonKendra left a comment

TheRealAmazonKendra Oct 21, 2022

TheRealAmazonKendra Oct 21, 2022

brendo-m Nov 2, 2022

TheRealAmazonKendra Oct 21, 2022

brendo-m Oct 21, 2022

TheRealAmazonKendra Oct 21, 2022

brendo-m Nov 2, 2022

TheRealAmazonKendra Nov 2, 2022

TheRealAmazonKendra Nov 2, 2022

aws-cdk-automation commented Nov 1, 2022

aws-cdk-automation commented Nov 1, 2022

aws-cdk-automation commented Nov 2, 2022

aws-cdk-automation commented Nov 2, 2022

TheRealAmazonKendra left a comment

TheRealAmazonKendra Nov 2, 2022

aws-cdk-automation commented Nov 2, 2022

aws-cdk-automation commented Nov 24, 2022

aws-cdk-automation commented Dec 1, 2022

mschlaipfer commented Jun 16, 2023

feat(cloudwatch): add verticalAnnotations property to GraphWidget #22568

feat(cloudwatch): add verticalAnnotations property to GraphWidget #22568

Conversation

brendo-m commented Oct 19, 2022 • edited Loading

All Submissions:

Adding new Unconventional Dependencies:

New Features

aws-cdk-automation left a comment

Choose a reason for hiding this comment

gitpod-io bot commented Oct 20, 2022

TheRealAmazonKendra left a comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Nov 1, 2022

aws-cdk-automation commented Nov 1, 2022

aws-cdk-automation commented Nov 2, 2022

aws-cdk-automation commented Nov 2, 2022

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Nov 2, 2022

AWS CodeBuild CI Report

aws-cdk-automation commented Nov 24, 2022

aws-cdk-automation commented Dec 1, 2022

mschlaipfer commented Jun 16, 2023

brendo-m commented Oct 19, 2022 •

edited

Loading