Skip to content

Commit

Permalink
fix(cloudwatch): cross-account metrics in env-agnostic stack (aws#5775)
Browse files Browse the repository at this point in the history
In order to avoid generating unnecessary diffs to currently-deployed
CloudWatch dashboards, in aws#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.
  • Loading branch information
rix0rrr authored Jan 13, 2020
1 parent e3305d8 commit 5292bd5
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 1 deletion.
4 changes: 3 additions & 1 deletion packages/@aws-cdk/aws-cloudwatch/lib/private/env-tokens.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,9 @@ class StackDependentToken implements cdk.IResolvable {
public resolve(context: cdk.IResolveContext) {
const stackValue = this.fn(cdk.Stack.of(context.scope));

if (cdk.Token.isUnresolved(stackValue) || stackValue === this.originalValue) {
// Don't render if the values are definitely the same. If the stack
// is unresolved we don't know, better output the value.
if (!cdk.Token.isUnresolved(stackValue) && stackValue === this.originalValue) {
return undefined;
}

Expand Down
32 changes: 32 additions & 0 deletions packages/@aws-cdk/aws-cloudwatch/test/test.cross-environment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,38 @@ export = {

test.done();
},

'metric with explicit account and region will render in environment agnostic stack'(test: Test) {
// GIVEN
const graph = new GraphWidget({
left: [
a.with({ account: '1234', region: 'us-north-5' })
],
});

// THEN
graphMetricsAre(test, new Stack(), graph, [
[ 'Test', 'ACount', { accountId: '1234', region: 'us-north-5' }],
]);

test.done();
},

'metric attached to agnostic stack will not render in agnostic stack'(test: Test) {
// GIVEN
const graph = new GraphWidget({
left: [
a.attachTo(new Stack()),
],
});

// THEN
graphMetricsAre(test, new Stack(), graph, [
[ 'Test', 'ACount' ],
]);

test.done();
},
},

'in alarms': {
Expand Down

0 comments on commit 5292bd5

Please sign in to comment.