feat(cloudwatch): validate parameters for a metric dimensions (closes #3116) #14365

OksanaH · 2021-04-25T00:38:50Z

As per #3116, the changes in this PR validate metric dimension values (length, and checking if the value is null or undefined) and throw errors if the values are not valid. I've also corrected a comment in the metric-types.ts to use the correct method name

…ws#3116)

gitpod-io · 2021-04-25T00:38:52Z

NetaNir · 2021-04-30T02:20:54Z

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

@@ -58,6 +58,10 @@ const metric = new Metric({
  }
 });
 ```
+### Metric dimensions


There is no need to add it in the README, documentation on the property is sufficient. The README is more suited for features/code samples.

NetaNir · 2021-04-30T02:23:07Z

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

@@ -395,6 +397,26 @@ export class Metric implements IMetric {

    return list;
  }
+
+  //rules from here: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-cw-dimension.html


add this link to the dimension property documentation, so it would show up in the development time.

It can be removed from here if it's on the prop itslef. If you want to keep it, add as a method documentation.

NetaNir · 2021-04-30T02:24:06Z

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

+  //rules from here: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-cw-dimension.html
+  private validateDimensions(dims: DimensionHash): void {
+    if (Object.keys(dims)?.length > 10) {
+      throw new Error('The maximum number of dimensions is 10');


Suggested change

throw new Error('The maximum number of dimensions is 10');

throw new Error(`The maximum number of dimensions is 10, received ${dims.size}`);

it says that dims.size is undefined, so I've used length of Object.keys(dims)

NetaNir · 2021-04-30T02:26:11Z

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

+      throw new Error('The maximum number of dimensions is 10');
+    }
+
+    Object.keys(dims).sort().map(key => {


why the sort?

NetaNir · 2021-04-30T02:42:34Z

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

+    Object.keys(dims).sort().map(key => {
+      if (dims[key] === undefined || dims[key] === null) {
+        throw new Error(`Dimension value of '${dims[key]}' is invalid`);


I didn't see in the docs that undefined is not allowed, assuming you verified it in deployment?

I ran the code sample in the original issue (#3116), and it failed. I think it is because of the undefined value

gitpod-io · 2021-04-30T13:53:05Z

Pull request has been modified.

NetaNir · 2021-05-04T04:25:32Z

packages/@aws-cdk/aws-cloudwatch/test/test.metrics.ts

+  },
+
+  'cannot use long dimension values'(test: Test) {
+    const invalidDimensionValue = 'SomeLongValueSomeLongValueSomeLongValueSomeLongValueSomeLongValueSomeLongValue\


Lets try something like this to make it more readable:

const arr = new Array(256); const invalidDimensionValue = arr.fill('A', 0).join('');

NetaNir · 2021-05-04T04:27:39Z

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

@@ -395,6 +397,26 @@ export class Metric implements IMetric {

    return list;
  }
+
+  //rules from here: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-cw-dimension.html


It can be removed from here if it's on the prop itslef. If you want to keep it, add as a method documentation.

…commented out line

Pull request has been modified.

mergify · 2021-05-07T20:49:03Z

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 · 2021-05-07T21:16:03Z

AWS CodeBuild CI Report

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

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

mergify · 2021-05-07T21:16:29Z

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

…ws#3116) (aws#14365) As per aws#3116, the changes in this PR validate metric dimension values (length, and checking if the value is null or undefined) and throw errors if the values are not valid. I've also corrected a comment in the metric-types.ts to use the correct method name

* chore(cloudfront): remove the use of calculateFunctionHash (#14583) `calculateFunctionHash()` was used to compute the 'refresh token' of the custom resource for the EdgeFunction construct. This method is private to the lambda module and is deemed to be changed. Instead, use the lambda function version's logical id. The logical id of the version includes computing the function hash (among others) and is a more reliable determinant of whether the underlying function version changed. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* * feat(cloudwatch): validate parameters for a metric dimensions (closes #3116) (#14365) As per #3116, the changes in this PR validate metric dimension values (length, and checking if the value is null or undefined) and throw errors if the values are not valid. I've also corrected a comment in the metric-types.ts to use the correct method name * feat(appmesh): change HealthChecks to use protocol-specific union-like classes (#14432) BREAKING CHANGE: HealthChecks require use of static factory methods fixes #11640 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* Co-authored-by: Niranjan Jayakar <nija@amazon.com> Co-authored-by: OksanaH <34384274+OksanaH@users.noreply.github.com> Co-authored-by: Dominic Fezzie <fezzid@amazon.com> Co-authored-by: Elad Ben-Israel <benisrae@amazon.com>

@BenChaimberg

* chore(cloudfront): remove the use of calculateFunctionHash (#14583) `calculateFunctionHash()` was used to compute the 'refresh token' of the custom resource for the EdgeFunction construct. This method is private to the lambda module and is deemed to be changed. Instead, use the lambda function version's logical id. The logical id of the version includes computing the function hash (among others) and is a more reliable determinant of whether the underlying function version changed. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* * feat(cloudwatch): validate parameters for a metric dimensions (closes #3116) (#14365) As per #3116, the changes in this PR validate metric dimension values (length, and checking if the value is null or undefined) and throw errors if the values are not valid. I've also corrected a comment in the metric-types.ts to use the correct method name * feat(appmesh): change HealthChecks to use protocol-specific union-like classes (#14432) BREAKING CHANGE: HealthChecks require use of static factory methods fixes #11640 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* * feat(msk): Cluster L2 Construct (#9908) L2 Construct for a MSK Cluster. I wrote this for internal use and thought I'd share it. I tried to follow the [example resource](https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/example-construct-library/lib/example-resource.ts) and [design guidelines](https://github.com/aws/aws-cdk/blob/master/DESIGN_GUIDELINES.md) as much as I could. Default properties were chosen either based on defaults when creating a cluster in the console or defaults set from CloudFormation. Closes #9603 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* * feat(kms): allow specifying key spec and key usage (#14478) This allows specifying key spec and key usage, so you can create asymmetric keys. closes #5639 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* * chore: add `@types/jest` to a package that was missing it (#14609) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* * chore: issue template for bugs to have an SSCCE example (#14615) Ask customers to provide reproducible code snippets to reduce the amount of triage time required. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* * fix(cli): synth fails if there was an error when synthesizing the stack (#14613) All stacks created inside a pipeline stage will be flagged for validation. After synth is done, the CLI will validate all flagged stacks plus the stacks that were explicitly specified. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* * chore: annotate `aws-lambda-go` with `docker` requirement (#14618) The `nozem` build tool needs to know that `docker` is required to build/test this particular package. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* * feat(elbv2): preserveClientIp for NetworkTargetGroup (#14589) Allows users to configure client IP preservation for network target groups. See: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-elasticloadbalancingv2-targetgroup-targetgroupattribute.html ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* * chore: npm-check-updates && yarn upgrade (#14620) Ran npm-check-updates and yarn upgrade to keep the `yarn.lock` file up-to-date. * chore(mergify): add @BenChaimberg to team roster * chore(release): 1.103.0 * chore: mark "otaviomacedo" as core contributor (#14619) Co-authored-by: Otavio Macedo <otaviomacedo@protonmail.com> * chore(cli): add npm command to upgrade notice (#14621) A colleague had to go look up the command to update the CDK CLI. It occurred to me that is probably common with developers who don't work with NPM on a daily basis, such as anyone who isn't developing in TypeScript or JavaScript. Put the necessary command right in the upgrade notice. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* * fix(lambda): custom resource fails to connect to efs filesystem (#14431) Fixes: #14430 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* * feat(cfnspec): cloudformation spec v35.2.0 (#14610) * feat: cloudformation spec v35.2.0 * add spec patches Co-authored-by: AWS CDK Team <aws-cdk@amazon.com> Co-authored-by: Elad Ben-Israel <benisrae@amazon.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> * fix(lambda-nodejs): handler filename missing from error message (#14564) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* * chore(build): clarify prerequisites in CONTRIBUTING and verify before build (#14642) The build script at the top level of the repository performs a prerequisites check before beginning the build. This verification is not complete as it does not include the required checks for .NET and Python executables. Further, the prerequisites documentation in the contributing guide does not note the Docker requirement. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* Co-authored-by: Niranjan Jayakar <nija@amazon.com> Co-authored-by: OksanaH <34384274+OksanaH@users.noreply.github.com> Co-authored-by: Dominic Fezzie <fezzid@amazon.com> Co-authored-by: Curtis <curtis.eppel@cultureamp.com> Co-authored-by: Shinya Tsuda <shinya@dacci.org> Co-authored-by: Rico Huijbers <rix0rrr@gmail.com> Co-authored-by: Otavio Macedo <otaviomacedo@protonmail.com> Co-authored-by: Griffin Byatt <byatt.griffin@gmail.com> Co-authored-by: Ben Chaimberg <chaimber@amazon.com> Co-authored-by: AWS CDK Team <aws-cdk@amazon.com> Co-authored-by: Mitchell Valine <valinm@amazon.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Jerry Kindall <52084730+Jerry-AWS@users.noreply.github.com> Co-authored-by: Daniel Neilson <53624638+ddneilson@users.noreply.github.com> Co-authored-by: Elad Ben-Israel <benisrae@amazon.com>

…ws#3116) (aws#14365) As per aws#3116, the changes in this PR validate metric dimension values (length, and checking if the value is null or undefined) and throw errors if the values are not valid. I've also corrected a comment in the metric-types.ts to use the correct method name

OksanaH added 3 commits April 24, 2021 23:46

feat(cloudwatch): validate parameters for a Metric dimension (closes a…

6e253f1

…ws#3116)

feat(cloudwatch): rework error logic (closes aws#3116)

e988e93

feat(cloudwatch): further corrections to unit tests closes aws#3116

8589b7c

github-actions bot added the @aws-cdk/aws-cloudwatch Related to Amazon CloudWatch label Apr 25, 2021

github-actions bot assigned NetaNir Apr 25, 2021

feat(cloudwatch): add information about metric dimensions to README

4ba576c

NetaNir previously requested changes Apr 30, 2021

View reviewed changes

feat(cloudwatch): address PR comments

4c0b71c

OksanaH changed the title ~~feat(cloudwatch): validate parameters for a metric dimensions~~ feat(cloudwatch): validate parameters for a metric dimensions (fixes #3116) Apr 30, 2021

OksanaH changed the title ~~feat(cloudwatch): validate parameters for a metric dimensions (fixes #3116)~~ feat(cloudwatch): validate parameters for a metric dimensions (closes #3116) Apr 30, 2021

NetaNir previously requested changes May 4, 2021

View reviewed changes

feat(cloudwatch): make variable value creation more readable, remove …

91bcd04

…commented out line

NetaNir approved these changes May 7, 2021

View reviewed changes

NetaNir added pr-linter/exempt-test The PR linter will not require test changes pr-linter/exempt-readme The PR linter will not require README changes and removed pr-linter/exempt-test The PR linter will not require test changes labels May 7, 2021

Merge branch 'master' into cloudwatch

38290cd

mergify bot merged commit 4a24d61 into aws:master May 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cloudwatch): validate parameters for a metric dimensions (closes #3116) #14365

feat(cloudwatch): validate parameters for a metric dimensions (closes #3116) #14365

OksanaH commented Apr 25, 2021

gitpod-io bot commented Apr 25, 2021 •

edited

Loading

NetaNir Apr 30, 2021

NetaNir Apr 30, 2021

NetaNir May 4, 2021

NetaNir Apr 30, 2021

OksanaH May 3, 2021

NetaNir Apr 30, 2021

NetaNir Apr 30, 2021

OksanaH Apr 30, 2021

gitpod-io bot commented Apr 30, 2021

NetaNir May 4, 2021

NetaNir May 4, 2021

mergify bot commented May 7, 2021

aws-cdk-automation commented May 7, 2021

mergify bot commented May 7, 2021

	throw new Error('The maximum number of dimensions is 10');
	throw new Error(`The maximum number of dimensions is 10, received ${dims.size}`);

feat(cloudwatch): validate parameters for a metric dimensions (closes #3116) #14365

feat(cloudwatch): validate parameters for a metric dimensions (closes #3116) #14365

Conversation

OksanaH commented Apr 25, 2021

gitpod-io bot commented Apr 25, 2021 • edited Loading

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

gitpod-io bot commented Apr 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented May 7, 2021

aws-cdk-automation commented May 7, 2021

AWS CodeBuild CI Report

mergify bot commented May 7, 2021

gitpod-io bot commented Apr 25, 2021 •

edited

Loading