fix(cloudwatch): `period` of each metric in `usingMetrics` for `MathExpression` is ignored #30986

go-to-k · 2024-07-31T09:34:26Z

Issue # (if applicable)

Closes #.

Reason for this change

The usingMetrics property (Record<string, IMetric>) in MathExpressionProps has Metric objects with a period.

On the other hand, in the MathExpression construct, the period of each metric in the usingMetrics is ignored and instead overridden by the period of the MathExpression. Even if the period of the MathExpression is not specified, it is overridden by its default value.

https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-cloudwatch/lib/metric.ts#L606-L608

However the statement is not written in the JSDoc.

https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-cloudwatch/lib/metric.ts#L566

new MathExpression({
  expression: "m1+m2",
  label: "AlbErrors",
  usingMetrics: {
    m1: metrics.custom("HTTPCode_ELB_500_Count", {
      period: Duration.minutes(1), // <- ignored and overridden by default value `Duration.minutes(5)` of `period` in the `MathExpressionOptions`
      statistic: "Sum",
      label: "HTTPCode_ELB_500_Count",
    }),
    m2: metrics.custom("HTTPCode_ELB_502_Count", {
      period: Duration.minutes(1), // <- ignored and overridden by default value `Duration.minutes(5)` of `period` in the `MathExpressionOptions`
      statistic: "Sum",
      label: "HTTPCode_ELB_502_Count",
    }),
  },
}),

Description of changes

The current documentation could be confusing to users, so add this description. Also added warnings in the situation.

Description of how you validated changes

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

…in math expression

kaizencc

In addition to this, what if we added a warning annotation anytime we find ourselves overwriting a different value? it's a little sinister to override a value and not tell the user when we do it.

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

Pull request has been modified.

fix for rosetta

go-to-k · 2024-09-05T12:01:45Z

@kaizencc

Thanks for your review.

In addition to this, what if we added a warning annotation anytime we find ourselves overwriting a different value? it's a little sinister to override a value and not tell the user when we do it.

I added: 712e937

I have two questions:

Should I change the PR title from docs(cloudwatch): ... to something else? (I thought this PR was not docs because warning process added.) Or, should PRs be separated?
We can't warn only if the period in usingMetrics is set to the same value as the default value (Duration.minutes(5)), is there a problem?

It is correct usage to not specify a value for period (in usingMetrics), so we do not want to raise a warning in that case. However, if the period is not specified, it is overwritten with a default value, so we cannot determine whether the user has specified that value or if it has been overwritten. Therefore, warnings cannot be raised only in this case.

712e937#diff-289bad9fd897ab7808c177ad955a23faf18615ff8434f82b50fa0aeb54985de6R941

for example:

new MathExpression({
  expression:  'm1',
  label: 'AlbErrors',
  usingMetrics: {
    m1: metrics.custom('HTTPCode_ELB_500_Count', {
      period: Duration.minutes(5), // <- This is equal to the default value for `period`. So it is the same as not having specified.
      statistic: 'Sum',
      label: 'HTTPCode_ELB_500_Count',
    }),
  },
  period: Duration.minutes(3),
});

go-to-k · 2024-09-12T05:28:53Z

Hi, @kaizencc

Have you had a chance to look at the comment?

#30986 (comment)

go-to-k · 2024-09-26T05:56:44Z

@kaizencc

Could you please take a look at the comment? Do you have any good ideas to share?

go-to-k · 2024-11-05T16:39:42Z

@kaizencc Could you take a look at it again?

kaizencc

hi @go-to-k :). sorry for the super long delay. to answer your questions:

i think the title should be now a fix. i'll leave it to you to update.
i think this is ok. it's better than nothing.

happy to approve once you update! i think the change to default.ts-fixture is unnecessary but i might be wrong.

kaizencc · 2024-12-03T20:00:14Z

packages/aws-cdk-lib/rosetta/aws_cloudwatch/default.ts-fixture

@@ -2,6 +2,7 @@
 import { Construct } from 'constructs';
 import { Stack, Duration } from 'aws-cdk-lib';
 import * as cloudwatch from 'aws-cdk-lib/aws-cloudwatch';
+import * as elbv2 from 'aws-cdk-lib/aws-elasticloadbalancingv2';


i think this is unnecessary?

Thank you @kaizencc !

I think it is necessary here, could you check it?

#30986 (comment)

go-to-k · 2024-12-04T04:35:01Z

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

+   * Example:
+   *
+   * ```ts
+   * declare const metrics: elbv2.IApplicationLoadBalancerMetrics;


got it! thanks :)

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.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

go-to-k · 2024-12-04T05:01:36Z

Exemption Request: This PR could be covered by unit tests.

go-to-k · 2024-12-04T06:58:06Z

@kaizencc

I also changed the PR title to fix....

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

mergify · 2024-12-09T15:43:30Z

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

kaizencc · 2024-12-09T20:56:56Z

@Mergifyio refresh

mergify · 2024-12-09T20:57:08Z

refresh

✅ Pull request refreshed

go-to-k · 2024-12-10T03:36:05Z

@Mergifyio update

mergify · 2024-12-10T03:36:26Z

update

✅ Branch has been successfully updated

codecov · 2024-12-10T03:49:19Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.67%. Comparing base (4937ee1) to head (9cbfba5).

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #30986      +/-   ##
==========================================
+ Coverage   78.66%   78.67%   +0.01%     
==========================================
  Files         107      107              
  Lines        7237     7237              
  Branches     1329     1329              
==========================================
+ Hits         5693     5694       +1     
+ Misses       1358     1357       -1     
  Partials      186      186

Flag	Coverage Δ
suite.unit	`78.67% <ø> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	`78.67% <ø> (+0.01%)`	⬆️

mergify · 2024-12-10T04:16:00Z

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

mergify · 2024-12-10T21:57:15Z

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 · 2024-12-11T00:28:45Z

AWS CodeBuild CI Report

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

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

mergify · 2024-12-11T00:29:18Z

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

github-actions · 2024-12-11T00:29:31Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

go-to-k added 2 commits July 31, 2024 17:56

docs(cloudwatch): add description of ignoring period of each metrics …

af0c176

…in math expression

change statements

91f64bb

aws-cdk-automation requested a review from a team July 31, 2024 09:34

github-actions bot added p2 distinguished-contributor [Pilot] contributed 50+ PRs to the CDK labels Jul 31, 2024

Merge branch 'main' into docs/cloudwatch-math-expression-usingmetrics

54040c2

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 31, 2024

go-to-k changed the title ~~docs(cloudwatch): add description that period of each metric is ignored in math expression~~ docs(cloudwatch): add description that period of each metric in usingMetrics for MathExpression is ignored Jul 31, 2024

This was referenced Aug 1, 2024

Monthly PR metrics report #30999

Closed

Weekly PR metrics report #31022

Closed

kaizencc previously requested changes Sep 4, 2024

View reviewed changes

packages/aws-cdk-lib/aws-cloudwatch/lib/metric.ts Outdated Show resolved Hide resolved

kaizencc changed the title ~~docs(cloudwatch): add description that period of each metric in usingMetrics for MathExpression is ignored~~ docs(cloudwatch): explain when period of each metric in usingMetrics for MathExpression is ignored Sep 4, 2024

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Sep 4, 2024

modify comments and add sample code

b8b2ae0

fix for rosetta

go-to-k force-pushed the docs/cloudwatch-math-expression-usingmetrics branch from 13f3459 to b8b2ae0 Compare September 5, 2024 09:27

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

go-to-k added 2 commits September 5, 2024 20:42

add warnings

712e937

Merge branch 'main' into docs/cloudwatch-math-expression-usingmetrics

56c657f

go-to-k requested a review from kaizencc September 29, 2024 17:37

kaizencc reviewed Dec 3, 2024

View reviewed changes

go-to-k commented Dec 4, 2024

View reviewed changes

go-to-k changed the title ~~docs(cloudwatch): explain when period of each metric in usingMetrics for MathExpression is ignored~~ fix(cloudwatch): period of each metric in usingMetrics for MathExpression is ignored Dec 4, 2024

aws-cdk-automation previously requested changes Dec 4, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Dec 4, 2024

aws-cdk-automation added pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Dec 4, 2024

go-to-k requested a review from kaizencc December 4, 2024 07:01

kaizencc added pr-linter/exempt-integ-test The PR linter will not require integ test changes and removed pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. labels Dec 9, 2024

kaizencc approved these changes Dec 9, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Dec 9, 2024

Merge branch 'main' into docs/cloudwatch-math-expression-usingmetrics

8cf6073

Merge branch 'main' into docs/cloudwatch-math-expression-usingmetrics

02c176b

mergify bot added 2 commits December 10, 2024 23:55

Merge branch 'main' into docs/cloudwatch-math-expression-usingmetrics

99ff51d

Merge branch 'main' into docs/cloudwatch-math-expression-usingmetrics

9cbfba5

mergify bot merged commit 59e96a3 into aws:main Dec 11, 2024
17 checks passed

github-actions bot locked as resolved and limited conversation to collaborators Dec 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cloudwatch): `period` of each metric in `usingMetrics` for `MathExpression` is ignored #30986

fix(cloudwatch): `period` of each metric in `usingMetrics` for `MathExpression` is ignored #30986

go-to-k commented Jul 31, 2024 •

edited

Loading

kaizencc left a comment

go-to-k commented Sep 5, 2024 •

edited

Loading

go-to-k commented Sep 12, 2024

go-to-k commented Sep 26, 2024

go-to-k commented Nov 5, 2024

kaizencc left a comment

kaizencc Dec 3, 2024

go-to-k Dec 4, 2024 •

edited

Loading

go-to-k Dec 4, 2024

kaizencc Dec 9, 2024

aws-cdk-automation left a comment

go-to-k commented Dec 4, 2024

go-to-k commented Dec 4, 2024 •

edited

Loading

mergify bot commented Dec 9, 2024

kaizencc commented Dec 9, 2024

mergify bot commented Dec 9, 2024

go-to-k commented Dec 10, 2024

mergify bot commented Dec 10, 2024

codecov bot commented Dec 10, 2024 •

edited

Loading

mergify bot commented Dec 10, 2024

mergify bot commented Dec 10, 2024

aws-cdk-automation commented Dec 11, 2024

mergify bot commented Dec 11, 2024

github-actions bot commented Dec 11, 2024

fix(cloudwatch): period of each metric in usingMetrics for MathExpression is ignored #30986

fix(cloudwatch): period of each metric in usingMetrics for MathExpression is ignored #30986

Conversation

go-to-k commented Jul 31, 2024 • edited Loading

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

kaizencc left a comment

Choose a reason for hiding this comment

go-to-k commented Sep 5, 2024 • edited Loading

go-to-k commented Sep 12, 2024

go-to-k commented Sep 26, 2024

go-to-k commented Nov 5, 2024

kaizencc left a comment

Choose a reason for hiding this comment

kaizencc Dec 3, 2024

Choose a reason for hiding this comment

go-to-k Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

go-to-k Dec 4, 2024

Choose a reason for hiding this comment

kaizencc Dec 9, 2024

Choose a reason for hiding this comment

aws-cdk-automation left a comment

Choose a reason for hiding this comment

go-to-k commented Dec 4, 2024

go-to-k commented Dec 4, 2024 • edited Loading

mergify bot commented Dec 9, 2024

kaizencc commented Dec 9, 2024

mergify bot commented Dec 9, 2024

✅ Pull request refreshed

go-to-k commented Dec 10, 2024

mergify bot commented Dec 10, 2024

✅ Branch has been successfully updated

codecov bot commented Dec 10, 2024 • edited Loading

Codecov Report

mergify bot commented Dec 10, 2024

mergify bot commented Dec 10, 2024

aws-cdk-automation commented Dec 11, 2024

AWS CodeBuild CI Report

mergify bot commented Dec 11, 2024

github-actions bot commented Dec 11, 2024

fix(cloudwatch): `period` of each metric in `usingMetrics` for `MathExpression` is ignored #30986

fix(cloudwatch): `period` of each metric in `usingMetrics` for `MathExpression` is ignored #30986

go-to-k commented Jul 31, 2024 •

edited

Loading

go-to-k commented Sep 5, 2024 •

edited

Loading

go-to-k Dec 4, 2024 •

edited

Loading

go-to-k commented Dec 4, 2024 •

edited

Loading

codecov bot commented Dec 10, 2024 •

edited

Loading