feat(cloudwatch): make Metric objects region-aware #5628

rix0rrr · 2020-01-03T13:50:41Z

Commit Message

feat(cloudwatch): make Metric objects region-aware

Metric objects always could have region and account fields, but it
was the user's responsibility to set them. They can now automatically
copy the region and account fields from a Construct anywhere in the
Construct tree (under a Stack) by calling the attachTo() method.

Predefined Metric objects returned by .metricsXxx() functions of the
AWS Construct Library will automatically have the scope of the
originating construct attached.

In this way, cross-environment dashboards can automatically be created.

`Metric` objects always could have `region` and `account` fields, but it was the user's responsibility to set them. They can now automatically copy the `region` and `account` fields from a Construct anywhere in the Construct tree (under a `Stack`) by calling the `attachTo()` method. Predefined `Metric` objects returned by `.metricsXxx()` functions of the AWS Construct Library will automatically have the scope of the originating construct attached. In this way, cross-platform dashboards can automatically be created.

aws-cdk-automation · 2020-01-03T14:19:05Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

nija-at · 2020-01-03T15:23:37Z

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

-    const ret = new Metric({
+    // Short-circuit creating a new object if there would be no effective change
+    if ((props.label === undefined || props.label === this.label)
+      && (props.color === undefined || props.color === this.color)
+      && (props.statistic === undefined || props.statistic === this.statistic)
+      && (props.unit === undefined || props.unit === this.unit)
+      && (props.account === undefined || props.account === this.account)
+      && (props.region === undefined || props.region === this.region)
+      // For these we're not going to do deep equality, misses some opportunity for optimization
+      // but that's okay.
+      && (props.dimensions === undefined)
+      && (props.period === undefined)) {
+      return this;
+    }


How likely is it that a new property is added to the Metric and MetricOptions classes (new CW feature) and the code change misses to update this check and now there's a bug?

I'm not familiar with this code, but is there something we can do better to ensure this is not missed? Either better code structuring so its obvious or some kind of regression test?

There will never be a bug, just a missed opportunity for an optimization. Metric classes are immutable, always getting a new one (even if you could have reused the old one) is perfectly valid.

They are never checked for object identity equality either, which wouldn't make sense as I can make equivalent objects and they should behave the same in all cases.

What you're saying is correct, but apart from not knowing how I would write an effective regression test in this way, it's also not that important. I just wanted to give the garbage collector (and jsii) an easier time.

I can add a test for period, given that that's the one that's most likely to change which I wrote this behavior for.

I understand the bit about optimization.

However, when there’s a new property in the Metric class (say propertyX) which was missed as part of this if statement, it would return an existing Metric object without checking whether propertyX were equal as well, would you not?

nija-at · 2020-01-03T15:59:40Z

packages/@aws-cdk/aws-cloudwatch/lib/env-tokens.ts

+ * Necessary to prevent options objects that only contain "region" and "account" keys
+ * that evaluate to "undefined" from showing up in the rendered JSON.
+ */
+export class DropEmptyGraphOptions implements cdk.IResolvable, cdk.IPostProcessor {


There's nothing in this class that is specific to graph options, is there? Should this just be called DropEmptyElements or something like that?

I'm finding this code organization somewhat confusing. This class is part of env-tokens - not clear to my what about this is about env; while dropUndefined is part of metric-util. Both seem to be related functionally, and I would assume it would sit beside each other.

On a side note regarding metric-util, I'd say we should avoid util classes or files. It's going to be the trash bin of where we put things that don't belong anywhere else. Based on a cursory reading, it seems to me that this file is already getting to hold all sorts of things. (If you agree and intend to refactor, I suggest a separate PR)

There's nothing in this class that is specific to graph options, is there?

There is. It knows to interpret its argument as an array which optionally can have an object as its last element, and we operate on the last element. That pattern is only used in CloudWatch graphs.

while dropUndefined is part of metric-util

dropUndefined is more generic and also used in other places. I can rename this file, but the behavior in this class is so... "special" (given it uses Token magic and all) that I wanted to keep it separate. It's called env-tokens, because it makes Tokens that are environment-related.

I'd say we should avoid util classes or files

I don't really agree. I make utility functions for functionality that should have been in a JavaScript's standard library all the time (see dropUndefined) and I tend to put them in a file util.ts or similar. The alternatives to such a util file are:

taking a dependency on underscore (please god no)

copy/pasting the implementation of dropUndefined into every file that needs it (also, please god no? but ymmv)

I agree that I mixed up two different classes of utilities in the current metric-util, which is bad on my part. I will split it out into a proper util and something about metric expansion.

There's nothing in this class that is specific to graph options, is there?

There is. It knows to interpret its argument as an array which optionally can have an object as its last element, and we operate on the last element. That pattern is only used in CloudWatch graphs.

We should be naming classes based on what it does and what it operates on; not where it is used - that's what namespaces are for (not typescript namespaces, but conceptual) and it's already correctly placed under aws-cloudwatch.

Further, it's not an env-token so that's not the right spot either. I still think it should be called something like DropLastEmptyElement and place it under a similarly named file?

packages/@aws-cdk/aws-cloudwatch/lib/env-tokens.ts

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

aws-cdk-automation · 2020-01-06T09:22:56Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-01-06T09:28:33Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-01-06T11:10:18Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

nija-at · 2020-01-06T11:31:19Z

packages/@aws-cdk/aws-cloudwatch/lib/private/util.ts

@@ -0,0 +1,10 @@
+export function dropUndefined<T extends object>(x: T): T {


Let's name this file object.ts and it can hold any utility functions that operate on Javascript objects.

Extending this everywhere honestly sounds kind of exhausting to me, and I'm not sure I see the benefit. Are you really proposing to have a file called object.ts with a single function in it, and a file called string.ts with a single function in it, because you're opposed to having a file called util.ts with both?

I know that the argument against this is "wild growth" of functions and before you know it there will be 3,351 functions in there, but consider:

Our modules are fairly small and they don't share utility functions; they're all copy/pasted around. If a module is going to have 10-odd utility functions, that's going to be at the upper end of the scale.

Even if there will be many more, the effort of splitting up the single file into multiple ones is going to be trivial, because of this local use. The effect is guaranteed to not permeate into any other package.

My personal opinion: the level of scrutiny given to this issue is not proportional to its impact, nor would the proposed name be clearer to me (personally). Given just the filename object.ts and nothing else, I would struggle to think what could be in there. Nevertheless, I will apply the change.

packages/@aws-cdk/aws-cloudwatch/lib/private/util.statistic.ts

nija-at · 2020-01-06T12:05:10Z

packages/@aws-cdk/aws-cloudwatch/lib/env-tokens.ts

+ * Necessary to prevent options objects that only contain "region" and "account" keys
+ * that evaluate to "undefined" from showing up in the rendered JSON.
+ */
+export class DropEmptyGraphOptions implements cdk.IResolvable, cdk.IPostProcessor {


There's nothing in this class that is specific to graph options, is there?

There is. It knows to interpret its argument as an array which optionally can have an object as its last element, and we operate on the last element. That pattern is only used in CloudWatch graphs.

We should be naming classes based on what it does and what it operates on; not where it is used - that's what namespaces are for (not typescript namespaces, but conceptual) and it's already correctly placed under aws-cloudwatch.

Further, it's not an env-token so that's not the right spot either. I still think it should be called something like DropLastEmptyElement and place it under a similarly named file?

nija-at

This needs at least one integration test that does cross-stack and cross-account metrics rendered into a CW dashboard.

nija-at · 2020-01-06T12:09:21Z

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

+import { MathExpression } from "../metric";
+import { IMetric, MetricConfig, MetricExpressionConfig, MetricStatConfig } from "../metric-types";
+
+export function validateNoIdConflicts(expression: MathExpression) {


Can this just be a private method on MathExpression? It doesn't look like it's used anywhere else but that class.

nija-at · 2020-01-06T12:16:44Z

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

+}
+
+// tslint:disable-next-line:max-line-length
+export function dispatchMetric<A, B>(metric: IMetric, fns: { withStat: (x: MetricStatConfig, c: MetricConfig) => A, withExpression: (x: MetricExpressionConfig, c: MetricConfig) => B }): A | B {


Documentation. What this does or should be used for is not obvious.

rix0rrr · 2020-01-06T12:33:49Z

We should be naming classes based on what it does and what it operates on

It is: it operates on CloudWatch graph metric definitions and their options. The fact that we can't really type that properly in TypeScript and therefore doesn't have a type name doesn't mean it just operates on arbitrary objects.

This needs at least one integration test that does cross-stack and cross-account metrics rendered into a CW dashboard.

Our integration tests are not setup to do cross-account tests, otherwise I would have.

aws-cdk-automation · 2020-01-06T13:40:51Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2020-01-06T14:29:51Z

Thank you for contributing! Your pull request is now being automatically merged.

aws-cdk-automation · 2020-01-06T15:26:28Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2020-01-06T15:27:07Z

Thank you for contributing! Your pull request is now being automatically merged.

In order to avoid generating unnecessary diffs to currently-deployed CloudWatch dashboards, in #5628 when adding support for cross-region/cross-account metrics, we only selectively render the new attributes into the graph (only when we estimate it will make a difference). The method chosen was: Render account/region if they're *definitely* different. However, this has the side effect that the new region and account attributes don't work at all in environment-agnostic stacks (because we won't know whether they'll be different or not, and we assume they will be). Whether the original behavior was wrong or not can be debated, but it's unintuitive for sure: users put in values that don't come back out in the usual, getting-started case. In this PR, change the decision to: Don't render account/region if they're *definitely* the same. This will fix the case of manual input to `Metric`, and since `attachTo()` won't take account and region from environment-agnostic stacks anyway, it also won't introduce unwanted diffs in most cases.

rix0rrr requested review from RomainMuller, skinny85, eladb, MrArnoldPalmer and nija-at January 3, 2020 13:50

rix0rrr requested a review from SoManyHs as a code owner January 3, 2020 13:50

rix0rrr self-assigned this Jan 3, 2020

mergify bot added the contribution/core This is a PR that came from AWS. label Jan 3, 2020

nija-at suggested changes Jan 3, 2020

View reviewed changes

Rico Huijbers added 2 commits January 6, 2020 10:13

Add test on with() w/ same period not creating a new object

42cc6d4

Reorganize utility functions

652ed4e

rix0rrr requested a review from nija-at January 6, 2020 09:21

Fix build

177c211

nija-at reviewed Jan 6, 2020

View reviewed changes

Review comments

eb850d9

rix0rrr requested a review from nija-at January 6, 2020 14:06

nija-at approved these changes Jan 6, 2020

View reviewed changes

Merge branch 'master' into huijbers/attached-metrics

bafa4f7

mergify bot merged commit 212687c into master Jan 6, 2020

mergify bot deleted the huijbers/attached-metrics branch January 6, 2020 15:27

netroy mentioned this pull request Jan 10, 2020

Cloudwatch GraphWidget.to_json() does not return a valid widget json in 1.20.0 but does in 1.19.0 #5735

Closed

rix0rrr mentioned this pull request Jan 13, 2020

fix(cloudwatch): cross-account metrics in env-agnostic stack #5775

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cloudwatch): make Metric objects region-aware #5628

feat(cloudwatch): make Metric objects region-aware #5628

rix0rrr commented Jan 3, 2020 •

edited

Loading

aws-cdk-automation commented Jan 3, 2020

nija-at Jan 3, 2020 •

edited

Loading

rix0rrr Jan 6, 2020

nija-at Jan 6, 2020

nija-at Jan 3, 2020

rix0rrr Jan 6, 2020

nija-at Jan 6, 2020 •

edited

Loading

aws-cdk-automation commented Jan 6, 2020

aws-cdk-automation commented Jan 6, 2020

aws-cdk-automation commented Jan 6, 2020

nija-at Jan 6, 2020

rix0rrr Jan 6, 2020

nija-at Jan 6, 2020 •

edited

Loading

nija-at left a comment

nija-at Jan 6, 2020

nija-at Jan 6, 2020

rix0rrr commented Jan 6, 2020

aws-cdk-automation commented Jan 6, 2020

mergify bot commented Jan 6, 2020

aws-cdk-automation commented Jan 6, 2020

mergify bot commented Jan 6, 2020

		@@ -0,0 +1,10 @@
		export function dropUndefined<T extends object>(x: T): T {

feat(cloudwatch): make Metric objects region-aware #5628

feat(cloudwatch): make Metric objects region-aware #5628

Conversation

rix0rrr commented Jan 3, 2020 • edited Loading

Commit Message

aws-cdk-automation commented Jan 3, 2020

AWS CodeBuild CI Report

nija-at Jan 3, 2020 • 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

nija-at Jan 6, 2020 • edited Loading

Choose a reason for hiding this comment

aws-cdk-automation commented Jan 6, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jan 6, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jan 6, 2020

AWS CodeBuild CI Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nija-at Jan 6, 2020 • edited Loading

Choose a reason for hiding this comment

nija-at left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr commented Jan 6, 2020

aws-cdk-automation commented Jan 6, 2020

AWS CodeBuild CI Report

mergify bot commented Jan 6, 2020

aws-cdk-automation commented Jan 6, 2020

AWS CodeBuild CI Report

mergify bot commented Jan 6, 2020

rix0rrr commented Jan 3, 2020 •

edited

Loading

nija-at Jan 3, 2020 •

edited

Loading

nija-at Jan 6, 2020 •

edited

Loading

nija-at Jan 6, 2020 •

edited

Loading