Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cloudwatch): automatic metric math label cannot be suppressed #17639

Merged
merged 16 commits into from
Apr 3, 2022

Conversation

NetaNir
Copy link
Contributor

@NetaNir NetaNir commented Nov 23, 2021

According to CloudWatch docs:

For the Label column of the expression, enter a name that describes what the expression is calculating.
If the result of an expression is an array of time series, each of those time series is displayed on the graph with a separate line, with different colors. Immediately under the graph is a legend for each line in the graph. For a single expression that produces multiple time series, the legend captions for those time series are in the format Expression-Label Metric-Label. For example, if the graph includes a metric with a label of Errors and an expression FILL(METRICS(), 0) that has a label of Filled With 0:, one line in the legend would be Filled With 0: Errors. To have the legend show only the original metric labels, set Expression-Label to be empty.

In the current implementation, if the label is left empty, the expression string is used, which makes the graph cumbersome. In multi widget dashboards where real estate is scarce, it becomes a real issue.

See my cats widget before the fix:

Screen Shot 2021-11-22 at 5 17 35 PM

My cats widget after the fix:

Screen Shot 2021-11-22 at 5 20 34 PM


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

@gitpod-io
Copy link

gitpod-io bot commented Nov 23, 2021

@github-actions github-actions bot added the @aws-cdk/aws-cloudwatch Related to Amazon CloudWatch label Nov 23, 2021
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Nov 23, 2021
@@ -129,7 +129,7 @@ export interface MathExpressionOptions {
/**
* Label for this metric when added to a Graph in a Dashboard
*
* @default - Expression value is used as label
* @default - The legend will only show the original metric labels
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we change the default here, won't customers be surprised that their stacks have changed upon update? That seems problematic for a stable module.

To have the legend show only the original metric labels, set Expression-Label to be empty.

Instead of omitting the label entirely, can you set label = ''? To me, that seems to follow the cloudformation docs of explicitly setting the label to be empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we change the default here, won't customers be surprised that their stacks have changed upon update? That seems problematic for a stable module.

I would argue that the current default behavior is unexpected, but I see your point about the stability of the API and limiting the change to a specific input

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like Neta's behavior better, actually, so I would recommend doing that. My apologies for implementing the original thing (shittily).

If we are concerned about introducing unexpected changes, I would recommend going with a feature flag instead.

@NetaNir NetaNir changed the title fix(cloudwatch): allow empty label in a math expression feat(cloudwatch): empty string passed as label in a math expression will use CloudWatch default - the original metric label Nov 27, 2021
@NetaNir NetaNir requested review from kaizencc and removed request for madeline-k November 27, 2021 23:53
Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do this with a magic value (label: ''), instead of a property (expressionAsLabel: false) ?

Also, can we autodetect when this might be applicable? Like, when the expression is a search expression?

@rix0rrr rix0rrr assigned rix0rrr and unassigned madeline-k Mar 30, 2022
@rix0rrr rix0rrr changed the title feat(cloudwatch): empty string passed as label in a math expression will use CloudWatch default - the original metric label fix(cloudwatch): automatic expression label cannot be suppressed Apr 1, 2022
@rix0rrr rix0rrr added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Apr 1, 2022
@github-actions github-actions bot added the p2 label Apr 1, 2022
@rix0rrr rix0rrr changed the title fix(cloudwatch): automatic expression label cannot be suppressed fix(cloudwatch): automatic metric math label cannot be suppressed Apr 1, 2022
Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After experimenting, changed my mind. It makes sense, sorry. Added some docs to clarify use cases.

@mergify
Copy link
Contributor

mergify bot commented Apr 3, 2022

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: d378bcc
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify mergify bot merged commit 7fa3bf2 into master Apr 3, 2022
@mergify mergify bot deleted the neta/allow-empty-label branch April 3, 2022 10:51
@mergify
Copy link
Contributor

mergify bot commented Apr 3, 2022

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

This was referenced Apr 7, 2022
mergify bot added a commit that referenced this pull request Apr 7, 2022
See [CHANGELOG](https://github.com/aws/aws-cdk/blob/bump/1.152.0/CHANGELOG.md)

For convenience, extracted the relevant CHANGELOG entry:

## [1.152.0](v1.151.0...v1.152.0) (2022-04-06)


### Features

* **cfnspec:** cloudformation spec v63.0.0 ([#19679](#19679)) ([dba96a9](dba96a9))
* **cfnspec:** cloudformation spec v65.0.0 ([#19745](#19745)) ([796fc64](796fc64))
* **cli:** add --build option ([#19663](#19663)) ([eb9b8e2](eb9b8e2)), closes [#19667](#19667)
* **cli:** preview of `cdk import` ([#17666](#17666)) ([4f12209](4f12209))
* **core:** throw error when stack name exceeds max length ([#19725](#19725)) ([1ffd45e](1ffd45e))
* **eks:** add k8s v1.22 ([#19756](#19756)) ([9a518c5](9a518c5))
* **opensearch:** Add latest Opensearch Version 1.2 ([#19749](#19749)) ([a2ac36e](a2ac36e))
* add new integration test runner ([#19754](#19754)) ([1b4d010](1b4d010))
* **eks:** alb-controller v2.4.1 ([#19653](#19653)) ([1ec08df](1ec08df))
* **lambda:** add support for ephemeral storage ([#19552](#19552)) ([f1d9b6a](f1d9b6a)), closes [#19605](#19605)
* **s3:** EventBridge bucket notifications ([#18614](#18614)) ([d8e602b](d8e602b)), closes [#18076](#18076)
* **synthetics:** new puppeteer 3.5 runtime ([#19673](#19673)) ([ce2b91b](ce2b91b)), closes [#19634](#19634)


### Bug Fixes

* **aws_applicationautoscaling:** Add missing members to PredefinedMetric enum ([#18978](#18978)) ([75a6fa7](75a6fa7)), closes [#18969](#18969)
* **cli:** apps with many resources scroll resource output offscreen ([#19742](#19742)) ([053d22c](053d22c)), closes [#19160](#19160)
* **cli:** support attributes of DynamoDB Tables for hotswapping ([#19620](#19620)) ([2321ece](2321ece)), closes [#19421](#19421)
* **cloudwatch:** automatic metric math label cannot be suppressed ([#17639](#17639)) ([7fa3bf2](7fa3bf2))
* **codedeploy:** add name validation for Application, Deployment Group and Deployment Configuration ([#19473](#19473)) ([9185042](9185042))
* **codedeploy:** the Service Principal is wrong in isolated regions ([#19729](#19729)) ([7e9a43d](7e9a43d)), closes [#19399](#19399)
* **core:** `Fn.select` incorrectly short-circuits complex expressions ([#19680](#19680)) ([7f26fad](7f26fad))
* **core:** detect and resolve stringified number tokens ([#19578](#19578)) ([7d9ab2a](7d9ab2a)), closes [#19546](#19546) [#19550](#19550)
* **core:** reduce CFN template indent size to save bytes ([#19656](#19656)) ([fd63ca3](fd63ca3))
* **ecs:** 'desiredCount' and 'ephemeralStorageGiB' cannot be tokens ([#19453](#19453)) ([c852239](c852239)), closes [#16648](#16648)
* **ecs:** remove unnecessary error when adding volume to external task definition ([#19774](#19774)) ([5446ded](5446ded)), closes [#19259](#19259)
* **iam:** policies aren't minimized as far as possible ([#19764](#19764)) ([876ed8a](876ed8a)), closes [#19751](#19751)
* **logs:** Faulty Resource Policy Generated ([#19640](#19640)) ([1fdf122](1fdf122)), closes [#17544](#17544)
mergify bot added a commit that referenced this pull request Apr 7, 2022
See [CHANGELOG](https://github.com/aws/aws-cdk/blob/bump/2.20.0/CHANGELOG.md)

For convenience, extracted the relevant CHANGELOG entry:

## [2.20.0](v2.19.0...v2.20.0) (2022-04-07)


### Features

* **cfnspec:** cloudformation spec v63.0.0 ([#19679](#19679)) ([dba96a9](dba96a9))
* **cfnspec:** cloudformation spec v65.0.0 ([#19745](#19745)) ([796fc64](796fc64))
* **cli:** add --build option ([#19663](#19663)) ([eb9b8e2](eb9b8e2)), closes [#19667](#19667)
* **cli:** preview of `cdk import` ([#17666](#17666)) ([4f12209](4f12209))
* **core:** throw error when stack name exceeds max length ([#19725](#19725)) ([1ffd45e](1ffd45e))
* **eks:** add k8s v1.22 ([#19756](#19756)) ([9a518c5](9a518c5))
* **opensearch:** Add latest Opensearch Version 1.2 ([#19749](#19749)) ([a2ac36e](a2ac36e))
* add new integration test runner ([#19754](#19754)) ([1b4d010](1b4d010))
* **eks:** alb-controller v2.4.1 ([#19653](#19653)) ([1ec08df](1ec08df))
* **lambda:** add support for ephemeral storage ([#19552](#19552)) ([f1d9b6a](f1d9b6a)), closes [#19605](#19605)
* **s3:** EventBridge bucket notifications ([#18614](#18614)) ([d8e602b](d8e602b)), closes [#18076](#18076)


### Bug Fixes

* **aws_applicationautoscaling:** Add missing members to PredefinedMetric enum ([#18978](#18978)) ([75a6fa7](75a6fa7)), closes [#18969](#18969)
* **cli:** apps with many resources scroll resource output offscreen ([#19742](#19742)) ([053d22c](053d22c)), closes [#19160](#19160)
* **cli:** support attributes of DynamoDB Tables for hotswapping ([#19620](#19620)) ([2321ece](2321ece)), closes [#19421](#19421)
* **cloudwatch:** automatic metric math label cannot be suppressed ([#17639](#17639)) ([7fa3bf2](7fa3bf2))
* **codedeploy:** add name validation for Application, Deployment Group and Deployment Configuration ([#19473](#19473)) ([9185042](9185042))
* **codedeploy:** the Service Principal is wrong in isolated regions ([#19729](#19729)) ([7e9a43d](7e9a43d)), closes [#19399](#19399)
* **core:** `Fn.select` incorrectly short-circuits complex expressions ([#19680](#19680)) ([7f26fad](7f26fad))
* **core:** detect and resolve stringified number tokens ([#19578](#19578)) ([7d9ab2a](7d9ab2a)), closes [#19546](#19546) [#19550](#19550)
* **core:** reduce CFN template indent size to save bytes ([#19656](#19656)) ([fd63ca3](fd63ca3))
* **ecs:** 'desiredCount' and 'ephemeralStorageGiB' cannot be tokens ([#19453](#19453)) ([c852239](c852239)), closes [#16648](#16648)
* **ecs:** remove unnecessary error when adding volume to external task definition ([#19774](#19774)) ([5446ded](5446ded)), closes [#19259](#19259)
* **iam:** policies aren't minimized as far as possible ([#19764](#19764)) ([876ed8a](876ed8a)), closes [#19751](#19751)
* **logs:** Faulty Resource Policy Generated ([#19640](#19640)) ([1fdf122](1fdf122)), closes [#17544](#17544)
StevePotter pushed a commit to StevePotter/aws-cdk that referenced this pull request Apr 27, 2022
…s#17639)

According to CloudWatch [docs](https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/using-metric-math.html):

> For the Label column of the expression, enter a name that describes what the expression is calculating.
If the result of an expression is an array of time series, each of those time series is displayed on the graph with a separate line, with different colors. Immediately under the graph is a legend for each line in the graph. For a single expression that produces multiple time series, the legend captions for those time series are in the format Expression-Label Metric-Label. For example, if the graph includes a metric with a label of Errors and an expression FILL(METRICS(), 0) that has a label of Filled With 0:, one line in the legend would be Filled With 0: Errors. **To have the legend show only the original metric labels, set Expression-Label to be empty.**

In the current implementation, if the label is left empty, the expression string is used, which makes the graph cumbersome. In multi widget dashboards where real estate is scarce, it becomes a real issue.

See my cats widget before the fix:


<img width="1432" alt="Screen Shot 2021-11-22 at 5 17 35 PM" src="https://user-images.githubusercontent.com/8578043/142959081-f3c28ea9-dd36-431f-b123-262eed0b2625.png">

My cats widget after the fix:

<img width="1250" alt="Screen Shot 2021-11-22 at 5 20 34 PM" src="https://user-images.githubusercontent.com/8578043/142959125-2ea5f7b3-9170-43bc-ba8b-233dc1af9700.png">



 


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-cloudwatch Related to Amazon CloudWatch contribution/core This is a PR that came from AWS. p2 pr-linter/exempt-integ-test The PR linter will not require integ test changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants