feat(cloudwatch): dashboard variables #26285

Stacy-D · 2023-07-07T15:53:25Z

This change add the support for dashboard variables in CDK https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/cloudwatch_dashboard_variables.html. It allows to reduce the number of repeated CloudWatch dashboards by unifying them into one managed with variables.

Closes #26200

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

- Add dashboard variables implementation - Add tests - Add integ test - Add section in README on variables

gitpod-io · 2023-07-07T15:53:34Z

Stacy-D · 2023-07-07T16:03:29Z

Example dashboard with a search variable created as a result of added integ test:

humanzz · 2023-07-07T19:24:07Z

Hello reviewer,
Ana and I had reached out to the cloudwatch team to update the documentation describing the dashboard json to include dashboard variables to help you with reviewing these changes. The documentation update had just been released at https://docs.aws.amazon.com/AmazonCloudWatch/latest/APIReference/CloudWatch-Dashboard-Body-Structure.html#CloudWatch-Dashboard-Properties-Variables-Structure

kaizencc

Hi @Stacy-D. Thanks for this PR -- it is a great start! Just a few nits here and there and sorry that I did not like the phrase allow to :)

packages/aws-cdk-lib/aws-cloudwatch/README.md

packages/aws-cdk-lib/aws-cloudwatch/lib/variable.ts

packages/aws-cdk-lib/aws-cloudwatch/test/dashboard.test.ts

packages/@aws-cdk-testing/framework-integ/test/aws-cloudwatch/test/integ.dashboard-variables.ts

packages/aws-cdk-lib/aws-cloudwatch/lib/dashboard.ts

humanzz · 2023-07-08T08:16:29Z

Hey @kaizencc
It's great that this overall looks good to you!
@Stacy-D is taking this week off and is likely unable to address things during that time. I'll try to address them in her absence so expect changes from me soon :)

Co-authored-by: Kaizen Conroy <36202692+kaizencc@users.noreply.github.com>

Pull request has been modified.

Co-authored-by: Kaizen Conroy <36202692+kaizencc@users.noreply.github.com>

Stacy-D · 2023-07-08T10:49:25Z

Updated result of the integration test with both search and value variables

Stacy-D · 2023-07-08T10:55:05Z

Hey @kaizencc,
Thanks for your review! I addressed the requested changes and responded to your questions. So I'm now looking forward to a second review 😄

All the follow ups, should those be needed, will be addressed by @humanzz while I'm off.

humanzz · 2023-07-08T11:43:30Z

@kaizencc just a question, re the enums introduced in this PR, in previous PRs we sometimes got the feedback to prefer classes with static members to kinda future proof against introductions of new types e.g. https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-lambda/lib/architecture.ts.
Any thoughts on this?

kaizencc

This pretty much looks good to go to me @Stacy-D @humanzz! Just a few more minor comments.

packages/aws-cdk-lib/aws-cloudwatch/lib/variable.ts

kaizencc · 2023-07-10T22:18:08Z

packages/aws-cdk-lib/aws-cloudwatch/lib/variable.ts

+export class SearchDashboardVariable extends DashboardVariable {
+  private readonly props: ISearchDashboardVariable;
+
+  constructor(props: ISearchDashboardVariable) {


From the API docs, search and populateFrom are required "if inputType is select or radio and you are not specifying values."

Just want to confirm that SearchDashboardVariable will still work with inputType input right?

So, I went to try a different combo of parameters manually, and what would essentially happen if input inputType and we have a search variable, is that it will just be a normal input. However, on saving one is faced with the nice error

The dashboard body is invalid, there are 2 validation errors: [ { "dataPath": "/variables/0/search", "message": "Should NOT be valid" }, { "dataPath": "/variables/0/populateFrom", "message": "Should NOT be valid" } ]

So, to me there are 2 things to do here

SearchDashboardVariable should validate that inputType is not input

ISearchDashboardVariable.populateFrom should not be optional

I think ValueDashboardVariable might need some similar validations as well

Lovely. Thanks for the investigation. That's what I was afraid of, but the docs don't really say it...

packages/aws-cdk-lib/aws-cloudwatch/lib/variable.ts

kaizencc · 2023-07-10T22:29:10Z

packages/aws-cdk-lib/aws-cloudwatch/lib/variable.ts

+  /**
+   * Optional label for the selected item
+   *
+   * @default - value


What does value mean here? It's kind of an oversaturated word... if you mean the value you specify in the 'value' prop, then lets be specific :)

kaizencc · 2023-07-10T22:30:29Z

packages/aws-cdk-lib/aws-cloudwatch/lib/variable.ts

+   */
+  readonly searchExpression: string;
+  /**
+   * Optional dimension name from the search


needs a @default. Not sure why this wouldn't throw an error somewhere in our PR build. Will have to take a look later.

packages/aws-cdk-lib/aws-cloudwatch/lib/variable.ts

Pull request has been modified.

humanzz · 2023-07-11T09:28:34Z

@kaizencc made a change at 6e6bdb5 and 4213942 where

I addressed the comments
made some renames
added some validations + tests

Hopefully this will make things a bit clearer for the users.

kaizencc

@humanzz Looks awesome! 3 last minute comments. Otherwise I'm happy with this :).

packages/aws-cdk-lib/aws-cloudwatch/lib/variable.ts

kaizencc · 2023-07-11T14:21:57Z

packages/aws-cdk-lib/aws-cloudwatch/test/dashboard.test.ts

+
+    // WHEN
+    dashboard.addVariable(new SearchDashboardVariable({
+      defaultValue: '__FIRST',


I just realized that __FIRST is a special value. Can we make that into a constant so that it can be referenced like:

defaultValue: `DefaultValue.FIRST`

so just a class with a static constant?

WDYT? Maybe its unnecessary.

Well, I made it take a bit more responsibility in d8cc14a, let me know what you think

I like it. Will make DefaultValue.FIRST more discoverable, and I expect that it will be a popular default value for most use cases.

kaizencc · 2023-07-11T14:27:38Z

packages/aws-cdk-lib/aws-cloudwatch/lib/variable.ts

+   *
+   * For example `{AWS/EC2,InstanceId} MetricName=\"CPUUtilization\"`
+   */
+  readonly searchExpression: string;


Have you given any thought to strongly typing this expression? How unique can this string be? The examples in documentation all reference a namespace, dimension, and metricName in some format. Maybe with stronger types we can build this expression out for the user and not have to have them worry about typos.

I'm thinking something like:

class searchExpression { public static FROM_COMPONENTS( namespace: string, dimension: string, metricName: string, ) { return new searchExpression(`{${namespace},${dimension}} MetricName=\"${metricName\"`); } // users can still supply raw expressions if they want public constructor(public readonly expression: string) {} }

WDYT?

So, I'd be a bit hesitant here, without further investigation, abt how metrics with multiple dimensions work - if they're supported or not.
If you think it's needed, I can give that a try and see if that's supported, in which case the search expression would need to allow for number of dimensions >= 1

multiple dimensions are supported... I'll work to create a class for this

Great! I think if possible, a class would be a better customer experience than a raw string.

I blame you for the more code - now it needs more reviewing 😆

d8cc14a

I ended up refactoring how search values are modeled!

In fact, with this new change, I'm now wondering if we really need a separate ValueDashboardVariable and SearchDashboardVariable. The difference can really be just about the values prop. Something like Values.fromSearch and Values.fromStatic (or any better name).

Worth pursuing?

I went ahead and did that. I believe it looks better.
Let me know what you think.
I also believe docs/README/error messages might need a good look to ensure they make sense for users.

packages/aws-cdk-lib/aws-cloudwatch/README.md

Pull request has been modified.

…nto dashboard_vars

- make DashbordVariable a concrete class - introduce Values abstract class with static methods for creating search-based and static values

kaizencc

Okay! I think this is the last review. We're almost there. Thanks for bearing with me @humanzz

kaizencc · 2023-07-11T16:57:33Z

packages/aws-cdk-lib/aws-cloudwatch/test/dashboard.test.ts

+
+    // WHEN
+    dashboard.addVariable(new SearchDashboardVariable({
+      defaultValue: '__FIRST',


I like it. Will make DefaultValue.FIRST more discoverable, and I expect that it will be a popular default value for most use cases.

kaizencc · 2023-07-11T19:01:43Z

packages/aws-cdk-lib/aws-cloudwatch/lib/variable.ts

+   * @param metricName the metric name to be used in the search expression
+   * @param populateFrom the dimension name, that the search expression retrieves, whose values will be used to populate the values to choose from
+   */
+  public static fromSearchComponents(namespace: string, dimensions: string[], metricName: string, populateFrom: string): Values {


export interface SearchComponentProps { readonly namespace: string, readonly dimensions: string[], readonly metricName: string, readonly populateFrom: string, } export class Values { public static fromSearchComponents(props: SearchComponentProps): Values { } }

packages/aws-cdk-lib/aws-cloudwatch/lib/variable.ts

kaizencc · 2023-07-11T19:08:12Z

packages/aws-cdk-lib/aws-cloudwatch/README.md

+    label: 'Function',
+    inputType: cw.VariableInputType.RADIO,
+    value: 'originalFuncNameInDashboard',
+    values: cw.Values.fromSearchComponents('AWS/Lambda', ['FunctionName'], 'Duration', 'FunctionName'), // equivalent to cw.Values.fromSearch('{AWS/Lambda,FunctionName} MetricName=\"Duration\"', 'FunctionName'),


Suggested change

values: cw.Values.fromSearchComponents('AWS/Lambda', ['FunctionName'], 'Duration', 'FunctionName'), // equivalent to cw.Values.fromSearch('{AWS/Lambda,FunctionName} MetricName=\"Duration\"', 'FunctionName'),

// equivalent to cw.Values.fromSearch('{AWS/Lambda,FunctionName} MetricName=\"Duration\"', 'FunctionName'),

values: cw.Values.fromSearchComponents('AWS/Lambda', ['FunctionName'], 'Duration', 'FunctionName'),

packages/aws-cdk-lib/aws-cloudwatch/lib/variable.ts

kaizencc · 2023-07-11T19:13:59Z

packages/aws-cdk-lib/aws-cloudwatch/lib/variable.ts

+      throw new Error(`Variable with inputType (${options.inputType}) requires values to be set`);
+    }
+    if (options.inputType == VariableInputType.INPUT && options.values) {
+      throw new Error('Unsupported inputType INPUT. Please choose either SELECT or RADIO');


Suggested change

throw new Error('Unsupported inputType INPUT. Please choose either SELECT or RADIO');

throw new Error('inputType INPUT cannot be combined with values. Please choose either SELECT or RADIO or remove \'values\' from the configuration.');

kaizencc · 2023-07-11T19:16:25Z

packages/aws-cdk-lib/aws-cloudwatch/README.md

+
+The following example generates a Lambda function variable, with a radio button for each function. Functions are discovered by a metric query search.
+The `values` with `cw.Values.fromSearchComponents` indicates that the values will be populated from `FunctionName` values retrieved from the search expression `{AWS/Lambda,FunctionName} MetricName=\"Duration\"`.
+The `defaultValue` with `cw.DefaultValue.FIRST` indicates that the  default value will be the first value returned from the search.


Suggested change

The `defaultValue` with `cw.DefaultValue.FIRST` indicates that the default value will be the first value returned from the search.

The `defaultValue` with `cw.DefaultValue.FIRST` indicates that the default value will be the first value returned from the search.

packages/aws-cdk-lib/aws-cloudwatch/lib/variable.ts

kaizencc · 2023-07-11T19:28:19Z

packages/aws-cdk-lib/aws-cloudwatch/lib/variable.ts

+  /**
+   * Create a default value
+   * @param value the value to be used as default
+   */
+  public static of(value: any) {
+    return new DefaultValue(value);
+  }


I don't love of. Reason being that we're not creating a default value 'of' something. We're just referencing a value. Some options:

DefaultValue.string('us-east-1') DefaultValue.value('us-east-1') // contributes to overkill of the word value though

I don't really care. I just dont think of is right.

I honestly can't think of a better option than of - except maybe the overkill value you suggested :)
string doesn't align well with the fact that it's actually any, with the documentation saying Type: String, Number, or Boolean, depending on the type value for this variable

I think I'll go with value

Pull request has been modified.

humanzz · 2023-07-11T19:38:51Z

Okay! I think this is the last review. We're almost there. Thanks for bearing with me @humanzz

no worries at all. I think we're making good progress here.

kaizencc

Sweet @humanzz! How wonderful to get multiple rounds of review bashed out in a day :). Really appreciate the fast turnaround time!

humanzz · 2023-07-11T22:06:42Z

Awesome @kaizencc!
Thanks for the quick and actionable reviews!
Let's hope we catch the next version train 🚄 ✨

mergify · 2023-07-11T22:28:02Z

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-cdk-automation · 2023-07-11T22:53:59Z

AWS CodeBuild CI Report

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

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

mergify · 2023-07-11T22:54:19Z

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).

This change add the support for dashboard variables in CDK https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/cloudwatch_dashboard_variables.html. It allows to reduce the number of repeated CloudWatch dashboards by unifying them into one managed with variables. Closes aws#26200 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

feat(cloudwatch): introduce dashboard variables

f95e757

- Add dashboard variables implementation - Add tests - Add integ test - Add section in README on variables

github-actions bot added repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 labels Jul 7, 2023

aws-cdk-automation requested a review from a team July 7, 2023 15:53

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jul 7, 2023

Merge branch 'main' into dashboard_vars

8228942

kaizencc previously requested changes Jul 7, 2023

View reviewed changes

kaizencc self-assigned this Jul 7, 2023

chore: update README

96e5b2e

Co-authored-by: Kaizen Conroy <36202692+kaizencc@users.noreply.github.com>

Stacy-D and others added 4 commits July 8, 2023 10:14

chore: apply suggestions

5591be2

Co-authored-by: Kaizen Conroy <36202692+kaizencc@users.noreply.github.com>

chore(cloudwatch): fix formatting

d94c526

chore(cloudwatch): update README and unit tests

946d61a

chore(cloudwatch): update variables integ tests

6aebdbc

humanzz added 2 commits July 10, 2023 10:50

reformat

378577c

Merge branch 'main' into dashboard_vars

8282f84

kaizencc previously requested changes Jul 10, 2023

View reviewed changes

Apply suggestions from code review

44593a2

kaizencc changed the title ~~feat(cloudwatch): introduce dashboard variables~~ feat(cloudwatch): dashboard variables Jul 10, 2023

address comments, add validations, refactor and rename

6e6bdb5

Merge branch 'main' into dashboard_vars

cf4b749

humanzz added 3 commits July 11, 2023 10:29

Merge branch 'main' into dashboard_vars

c9fcbef

empty commit to force build

516a5e4

renames

4213942

kaizencc previously requested changes Jul 11, 2023

View reviewed changes

Update packages/aws-cdk-lib/aws-cloudwatch/README.md

f6bc1c2

humanzz added 7 commits July 11, 2023 17:05

introduce SearchValues and DefaultValue

d8cc14a

Merge branch 'dashboard_vars' of https://github.com/Stacy-D/aws-cdk i…

3fd2721

…nto dashboard_vars

update docs

bcd91be

Merge branch 'main' into dashboard_vars

6469ac3

set DefaultValue.FIRST to be readonly

84ac9ae

Merge branch 'dashboard_vars' of https://github.com/Stacy-D/aws-cdk i…

1871a19

…nto dashboard_vars

drop ValueDashboardVariable and SearchDashboardVariable

8d1d235

- make DashbordVariable a concrete class - introduce Values abstract class with static methods for creating search-based and static values

kaizencc previously requested changes Jul 11, 2023

View reviewed changes

Apply suggestions from code review

0042dd1

humanzz and others added 2 commits July 11, 2023 21:08

address comments

d8a9389

readme

ba9dbbc

kaizencc approved these changes Jul 11, 2023

View reviewed changes

Merge branch 'main' into dashboard_vars

7912c6d

mergify bot merged commit 73f2741 into aws:main Jul 11, 2023

github-actions bot mentioned this pull request Aug 1, 2023

Monthly PRs metrics report - July 2023 #26583

Closed

	throw new Error('Unsupported inputType INPUT. Please choose either SELECT or RADIO');
	throw new Error('inputType INPUT cannot be combined with values. Please choose either SELECT or RADIO or remove \'values\' from the configuration.');

	The `defaultValue` with `cw.DefaultValue.FIRST` indicates that the default value will be the first value returned from the search.
	The `defaultValue` with `cw.DefaultValue.FIRST` indicates that the default value will be the first value returned from the search.

feat(cloudwatch): dashboard variables #26285

feat(cloudwatch): dashboard variables #26285

Conversation

Stacy-D commented Jul 7, 2023

gitpod-io bot commented Jul 7, 2023

Stacy-D commented Jul 7, 2023

humanzz commented Jul 7, 2023

kaizencc left a comment

Choose a reason for hiding this comment

humanzz commented Jul 8, 2023

Stacy-D commented Jul 8, 2023

Stacy-D commented Jul 8, 2023 • edited Loading

humanzz commented Jul 8, 2023

kaizencc 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

humanzz commented Jul 11, 2023 • edited Loading

kaizencc 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaizencc 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

humanzz commented Jul 11, 2023

kaizencc left a comment

Choose a reason for hiding this comment

humanzz commented Jul 11, 2023

mergify bot commented Jul 11, 2023

aws-cdk-automation commented Jul 11, 2023

AWS CodeBuild CI Report

mergify bot commented Jul 11, 2023

Stacy-D commented Jul 8, 2023 •

edited

Loading

humanzz commented Jul 11, 2023 •

edited

Loading