feat(cloudwatch): parse all metrics statistics and support long format #23095

rigwild · 2022-11-26T02:11:05Z

Description

Previous PR added support for missing statistics #23074
This PR implements a proper parsing of all these statistics.

Support "short" format ts99
Support "long" format
- TS(10%:90%) | TS(10:90)
- TS(:90) | TS(:90%)
- TS(10:) | TS(10%:)
Formats are case insensitive (no breaking changes)
If "long" format and only upper boundary% TS(:90%), can be translated to "short" format ts90 (stat.asSingleStatStr)

Note

I noticed that the following code expected the parsing to throw if it failed, but it actually will not fail in any case (it just return GenericStatistic if no format matched).
I will not change this behavior here as I'm not willing to spend more effort testing if this breaks stuff elsewhere.

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

Lines 295 to 296 in 47943d2

    
           // Try parsing, this will throw if it's not a valid stat 
        
           this.statistic = normalizeStatistic(props.statistic || 'Average');

Followup work

As is, this PR does not change any customer-facing logic. To make use of it, make the parsing throw if no format is recognized.

At the end of the parser function, just replace

return {
  type: 'generic',
  statistic: stat,
} as GenericStatistic;

with

throw new UnrecognizedStatisticFormatError()

You can see all tested inputs here: https://regexr.com/7351s

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

gitpod-io · 2022-11-26T02:11:10Z

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.

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

rigwild · 2022-11-28T17:57:26Z

This is ready for review

rix0rrr · 2022-11-30T11:37:21Z

Hi @rigwild!

Heads up, that enum you modified in your previous PR was exposed by accident, and was never supposed to be public! 🫣

I've moved your new enum values to a class with factory functions in this PR here: #23172

Thanks for updating the parsing logic as well, but I'm unclear on whether this changes any output. Is it just for completeness of the internal typing?

rigwild · 2022-11-30T11:50:11Z

Thanks for updating the parsing logic as well, but I'm unclear on whether this changes any output. Is it just for completeness of the internal typing?

See quote

Followup work
As is, this PR does not change any customer-facing logic. To make use of it, make the parsing throw if no format is recognized.

The current parser was supposed to throw on parsing error, but that's not the case.

This specific PR only does internal stuff, a future change has to be implemented to make it actually throw and be useful

rigwild · 2022-11-30T11:52:11Z

Your new factories would solve this, but most code is currently passing raw strings, so this may still be useful to detect errors early before deploying stacks

rix0rrr · 2022-11-30T12:41:21Z

I'm pretty sure we used to do validation, and then the generic case got added because CloudWatch was adding new metrics we didn't recognize yet, and it was added as a catch-all to handle "whatever else" (I would need to do software archeology to confirm that).

So the question is: how valuable is validation, and is it going to get in the way of future additions to CloudWatch? I think we're going to have to resign ourselves to the validation only ever being able to be, at best, a warning: we can't block infra because it might be using some statistic we don't recognize yet. It might be a valid stat we just haven't gotten around to adding quite yet.

I wouldn't be opposed to adding validation like that, but I'm going to have to ask to make that the feature and main focus of the change. Changes the parsing logic may or may not be a part of it, but that does not deliver value by itself.

rigwild · 2022-11-30T16:38:40Z

That makes sense. That's why I didn't add the error throw! 😁

I can add this warning. I don't know if there is a logging system/lib? How to log it? console.warn?

aws-cdk-automation · 2022-12-01T00:11:50Z

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.

rigwild · 2022-12-22T13:23:37Z

Btw, this pr is ready

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.

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

TheRealAmazonKendra

Looks like this is not using Annotations.of(construct).addWarning(...):. Please use this kind of warning instead of a console log statement. With this you can also write a test for the warning showing up or not.

Pull request has been modified.

rigwild · 2023-02-14T00:37:17Z

Oh ok, when rix0rrr@ said:

so maybe it fits in neatly there.

I thought it meant that the console warning was ok 😁

Updated

TheRealAmazonKendra

Just one small requested change so that the warning is grammatically correct with or without the label.

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

Co-authored-by: Kendra Neil <53584728+TheRealAmazonKendra@users.noreply.github.com>

Pull request has been modified.

TheRealAmazonKendra · 2023-02-20T22:12:36Z

@Mergifyio update

mergify · 2023-02-20T22:12:40Z

update

❌ Base branch update has failed

refusing to allow a GitHub App to create or update workflow .github/workflows/yarn-upgrade-v1main.yml without workflows permission
err-code: 491EB

TheRealAmazonKendra

Soooo... do you want to write all of our READMEs? This one is fantastic!

Thank you for your contribution!

aws-cdk-automation · 2023-02-20T22:53:32Z

AWS CodeBuild CI Report

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

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

mergify · 2023-02-20T22:53:40Z

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

Since #23095 (which was released in 2.66.0), the `p100` statistic is no longer rendered into Alarms. Fixes #24976.

Since #23095 (which was released in 2.66.0), the `p100` statistic is no longer rendered into Alarms. Fixes #24976. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

aws-cdk-automation requested a review from a team November 26, 2022 02:11

github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 labels Nov 26, 2022

aws-cdk-automation previously requested changes Nov 26, 2022

View reviewed changes

rigwild force-pushed the main branch 5 times, most recently from 6ac61a4 to 1314195 Compare November 26, 2022 19:02

rigwild added 12 commits December 1, 2022 01:25

Correctly parse metric statistic

2aea37e

Update usages

d1e5845

Add statistics parsing tests

45b20f0

Explain statistics in README

c2dc018

Fix build fail

0d3882b

Lint errors

d5bd361

Lint errors lmeow

eb18f5e

Remove breaking change, allow case insensitive

019733a

make it work lmeow

1b90c57

Add integration tests

1bbbf35

Add missing IQM

be2e11e

Use Stats instead of private Statistic - no breaking changes

d356ea6

Merge branch 'main' into main

729d145

aws-cdk-automation previously requested changes Jan 19, 2023

View reviewed changes

rigwild changed the title ~~feat(aws-cloudwatch): parse all metrics statistics and support long format~~ feat(cloudwatch): parse all metrics statistics and support long format Jan 19, 2023

Merge branch 'main' into main

6e7f950

TheRealAmazonKendra previously requested changes Feb 13, 2023

View reviewed changes

rigwild added 4 commits February 13, 2023 23:11

Merge branch 'main' of github.com:aws/aws-cdk into main

6654d01

Use proper warning

34843f9

Add warning check test

3b4dc19

Merge branch 'main' of github.com:rigwild/aws-cdk into main

79b0bea

Merge branch 'main' into main

7c57780

rigwild requested a review from TheRealAmazonKendra February 14, 2023 00:37

TheRealAmazonKendra previously requested changes Feb 20, 2023

View reviewed changes

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

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

rigwild and others added 2 commits February 20, 2023 20:36

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

699261d

Co-authored-by: Kendra Neil <53584728+TheRealAmazonKendra@users.noreply.github.com>

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

05a16a8

Co-authored-by: Kendra Neil <53584728+TheRealAmazonKendra@users.noreply.github.com>

Merge branch 'main' into main

2391b6a

TheRealAmazonKendra approved these changes Feb 20, 2023

View reviewed changes

mergify bot merged commit 853e3d6 into aws:main Feb 20, 2023

rix0rrr added a commit that referenced this pull request Apr 7, 2023

fix(cloudwatch): p100 statistic is no longer recognized

4e715e9

Since #23095 (which was released in 2.66.0), the `p100` statistic is no longer rendered into Alarms. Fixes #24976.

rix0rrr mentioned this pull request Apr 7, 2023

fix(cloudwatch): p100 statistic is no longer recognized #24981

Merged

filletofish mentioned this pull request Jul 2, 2023

feat(scheduler): ScheduleGroup #26196

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cloudwatch): parse all metrics statistics and support long format #23095

feat(cloudwatch): parse all metrics statistics and support long format #23095

rigwild commented Nov 26, 2022 •

edited

gitpod-io bot commented Nov 26, 2022

aws-cdk-automation left a comment

rigwild commented Nov 28, 2022

rix0rrr commented Nov 30, 2022 •

edited

rigwild commented Nov 30, 2022

rigwild commented Nov 30, 2022

rix0rrr commented Nov 30, 2022

rigwild commented Nov 30, 2022

aws-cdk-automation commented Dec 1, 2022

rigwild commented Dec 22, 2022

aws-cdk-automation left a comment

TheRealAmazonKendra left a comment

rigwild commented Feb 14, 2023

TheRealAmazonKendra left a comment

TheRealAmazonKendra commented Feb 20, 2023

mergify bot commented Feb 20, 2023

TheRealAmazonKendra left a comment

aws-cdk-automation commented Feb 20, 2023

mergify bot commented Feb 20, 2023

	// Try parsing, this will throw if it's not a valid stat
	this.statistic = normalizeStatistic(props.statistic \|\| 'Average');

feat(cloudwatch): parse all metrics statistics and support long format #23095

feat(cloudwatch): parse all metrics statistics and support long format #23095

Conversation

rigwild commented Nov 26, 2022 • edited

Description

Note

Followup work

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Nov 26, 2022

aws-cdk-automation left a comment

Choose a reason for hiding this comment

rigwild commented Nov 28, 2022

rix0rrr commented Nov 30, 2022 • edited

rigwild commented Nov 30, 2022

rigwild commented Nov 30, 2022

rix0rrr commented Nov 30, 2022

rigwild commented Nov 30, 2022

aws-cdk-automation commented Dec 1, 2022

rigwild commented Dec 22, 2022

aws-cdk-automation left a comment

Choose a reason for hiding this comment

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

rigwild commented Feb 14, 2023

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

TheRealAmazonKendra commented Feb 20, 2023

mergify bot commented Feb 20, 2023

❌ Base branch update has failed

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Feb 20, 2023

AWS CodeBuild CI Report

mergify bot commented Feb 20, 2023

rigwild commented Nov 26, 2022 •

edited

rix0rrr commented Nov 30, 2022 •

edited