Skip to content

Commit

Permalink
fix(cloudwatch): Use region and accountId when directly set on metrics
Browse files Browse the repository at this point in the history
Closes aws#28731
  • Loading branch information
Trevor Burnham committed Apr 23, 2024
1 parent 31492c6 commit 81ca531
Show file tree
Hide file tree
Showing 5 changed files with 76 additions and 19 deletions.
18 changes: 10 additions & 8 deletions packages/aws-cdk-lib/aws-cloudwatch/lib/alarm.ts
Expand Up @@ -307,7 +307,7 @@ export class Alarm extends AlarmBase {
unit: stat.unitFilter,
},
id: 'm1',
accountId: self.requiresAccountId(stat) ? stat.account : undefined,
accountId: self.requiresAccountId(stat) ? (stat.account || stat.stackAccount) : undefined,
label: conf.renderingProperties?.label,
returnData: true,
} as CfnAlarm.MetricDataQueryProperty,
Expand Down Expand Up @@ -342,7 +342,7 @@ export class Alarm extends AlarmBase {
unit: stat.unitFilter,
},
id: entry.id || uniqueMetricId(),
accountId: self.requiresAccountId(stat) ? stat.account : undefined,
accountId: self.requiresAccountId(stat) ? (stat.account || stat.stackAccount) : undefined,
label: conf.renderingProperties?.label,
returnData: entry.tag ? undefined : false, // entry.tag evaluates to true if the metric is the math expression the alarm is based on.
};
Expand Down Expand Up @@ -375,10 +375,11 @@ export class Alarm extends AlarmBase {
* Validate that if a region is in the given stat config, they match the Alarm
*/
private validateMetricStat(stat: MetricStatConfig, metric: IMetric) {
const stack = Stack.of(this);
const stackRegion = Stack.of(this).region;
const statRegion = stat.region || stat.stackRegion;

if (definitelyDifferent(stat.region, stack.region)) {
throw new Error(`Cannot create an Alarm in region '${stack.region}' based on metric '${metric}' in '${stat.region}'`);
if (definitelyDifferent(statRegion, stackRegion)) {
throw new Error(`Cannot create an Alarm in region '${stackRegion}' based on metric '${metric}' in '${statRegion}'`);
}
}

Expand All @@ -397,16 +398,17 @@ export class Alarm extends AlarmBase {
*/
private requiresAccountId(stat: MetricStatConfig): boolean {
const stackAccount = Stack.of(this).account;
const statAccount = stat.account || stat.stackAccount;

// if stat.account is undefined, it's by definition in the same account
if (stat.account === undefined) {
// if stat.account and stat.stackAccount are undefined, it's by definition in the same account
if (statAccount === undefined) {
return false;
}

// Return true if they're different. The ACCOUNT_ID token is interned
// so will always have the same string value (and even if we guess wrong
// it will still work).
return stackAccount !== stat.account;
return stackAccount !== statAccount;
}
}

Expand Down
20 changes: 15 additions & 5 deletions packages/aws-cdk-lib/aws-cloudwatch/lib/metric-types.ts
Expand Up @@ -310,19 +310,29 @@ export interface MetricStatConfig {
*/
readonly unitFilter?: Unit;

/**
* Account which this metric comes from.
*
* @default - The account of the attached stack.
*/
readonly account?: string;

/**
* Region which this metric comes from.
*
* @default Deployment region.
* @default - The region of the attached stack.
*/
readonly region?: string;

/**
* Account which this metric comes from.
*
* @default Deployment account.
* Account of the stack this metric is attached to.
*/
readonly account?: string;
readonly stackAccount?: string;

/**
* Region of the stack this metric is attached to.
*/
readonly stackRegion?: string;
}

/**
Expand Down
32 changes: 28 additions & 4 deletions packages/aws-cdk-lib/aws-cloudwatch/lib/metric.ts
Expand Up @@ -105,16 +105,26 @@ export interface CommonMetricOptions {
/**
* Account which this metric comes from.
*
* @default - Deployment account.
* @default - The account of the attached stack.
*/
readonly account?: string;

/**
* Region which this metric comes from.
*
* @default - Deployment region.
* @default - The region of the attached stack.
*/
readonly region?: string;

/**
* Account of the stack this metric is attached to.
*/
readonly stackAccount?: string;

/**
* Region of the stack this metric is attached to.
*/
readonly stackRegion?: string;
}

/**
Expand Down Expand Up @@ -283,6 +293,12 @@ export class Metric implements IMetric {
/** Region which this metric comes from. */
public readonly region?: string;

/** Account of the stack this metric is attached to. */
public readonly stackAccount?: string;

/** Region of the stack this metric is attached to. */
public readonly stackRegion?: string;

/**
* Warnings attached to this metric.
* @deprecated - use warningsV2
Expand Down Expand Up @@ -325,6 +341,8 @@ export class Metric implements IMetric {
this.unit = props.unit;
this.account = props.account;
this.region = props.region;
this.stackAccount = props.stackAccount;
this.stackRegion = props.stackRegion;
}

/**
Expand All @@ -342,6 +360,8 @@ export class Metric implements IMetric {
&& (props.unit === undefined || props.unit === this.unit)
&& (props.account === undefined || props.account === this.account)
&& (props.region === undefined || props.region === this.region)
&& (props.stackAccount === undefined || props.stackAccount === this.stackAccount)
&& (props.stackRegion === undefined || props.stackRegion === this.stackRegion)
// For these we're not going to do deep equality, misses some opportunity for optimization
// but that's okay.
&& (props.dimensions === undefined)
Expand All @@ -361,6 +381,8 @@ export class Metric implements IMetric {
color: ifUndefined(props.color, this.color),
account: ifUndefined(props.account, this.account),
region: ifUndefined(props.region, this.region),
stackAccount: ifUndefined(props.stackAccount, this.stackAccount),
stackRegion: ifUndefined(props.stackRegion, this.stackRegion),
});
}

Expand All @@ -380,8 +402,8 @@ export class Metric implements IMetric {
const stack = cdk.Stack.of(scope);

return this.with({
region: cdk.Token.isUnresolved(stack.region) ? undefined : stack.region,
account: cdk.Token.isUnresolved(stack.account) ? undefined : stack.account,
stackRegion: cdk.Token.isUnresolved(stack.region) ? undefined : stack.region,
stackAccount: cdk.Token.isUnresolved(stack.account) ? undefined : stack.account,
});
}

Expand All @@ -397,6 +419,8 @@ export class Metric implements IMetric {
unitFilter: this.unit,
account: this.account,
region: this.region,
stackAccount: this.stackAccount,
stackRegion: this.stackRegion,
},
renderingProperties: {
color: this.color,
Expand Down
12 changes: 10 additions & 2 deletions packages/aws-cdk-lib/aws-cloudwatch/lib/private/rendering.ts
Expand Up @@ -47,8 +47,16 @@ function metricGraphJson(metric: IMetric, yAxis?: string, id?: string) {
}

// Metric attributes that are rendered to graph options
if (stat.account) { options.accountId = accountIfDifferentFromStack(stat.account); }
if (stat.region) { options.region = regionIfDifferentFromStack(stat.region); }
if (stat.account) {
options.accountId = stat.account;
} else if (stat.stackAccount) {
options.accountId = accountIfDifferentFromStack(stat.stackAccount);
}
if (stat.region) {
options.region = stat.region;
} else if (stat.stackRegion) {
options.region = regionIfDifferentFromStack(stat.stackRegion);
}
if (stat.period && stat.period.toSeconds() !== 300) { options.period = stat.period.toSeconds(); }
if (stat.statistic && stat.statistic !== 'Average') { options.stat = stat.statistic; }
},
Expand Down
13 changes: 13 additions & 0 deletions packages/aws-cdk-lib/aws-cloudwatch/test/cross-environment.test.ts
Expand Up @@ -59,7 +59,20 @@ describe('cross environment', () => {
graphMetricsAre(new Stack(), graph, [
['Test', 'ACount', { accountId: '1234', region: 'us-north-5' }],
]);
});

test('metric with explicit account and region that match stack will render as-is', () => {
// GIVEN
const graph = new GraphWidget({
left: [
a.with({ account: '1234', region: 'us-north-5' }),
],
});

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

test('metric attached to agnostic stack will not render in agnostic stack', () => {
Expand Down

0 comments on commit 81ca531

Please sign in to comment.