From 5472b11ab1d10514dd5f67dfaf5e21eba979d572 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Tue, 12 Apr 2022 02:50:12 +0200 Subject: [PATCH] fix(cloudwatch): MathExpression `id` contract is not clear (#19825) It is intended that all metric identifiers referenced in a MathExpression are included in the `usingMetrics` map. This allows passing around complex metrics as a single object, because the math expression object carries around its dependencies with it. This is slightly different than what people might be used to from raw CloudWatch, where there is no hierarchy and all metrics are supposed to be listed in the graph widget or alarm with a unique ID, and then referenced by ID. We can't make this contract obvious anymore by adding a hard validation, but we can add warnings to hint people at the right way to reference metrics in math expressions. Fixes #13942, closes #17126. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts | 6 ++- .../@aws-cdk/aws-cloudwatch/lib/dashboard.ts | 24 ++++++++- packages/@aws-cdk/aws-cloudwatch/lib/graph.ts | 4 ++ .../@aws-cdk/aws-cloudwatch/lib/layout.ts | 4 +- .../aws-cloudwatch/lib/metric-types.ts | 9 ++++ .../@aws-cdk/aws-cloudwatch/lib/metric.ts | 52 ++++++++++++++++++- .../@aws-cdk/aws-cloudwatch/lib/widget.ts | 16 ++++++ .../aws-cloudwatch/test/alarm.test.ts | 18 ++++++- .../aws-cloudwatch/test/dashboard.test.ts | 19 ++++++- .../aws-cloudwatch/test/graphs.test.ts | 4 -- .../aws-cloudwatch/test/metric-math.test.ts | 19 +++++++ 11 files changed, 162 insertions(+), 13 deletions(-) diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts b/packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts index 62d717f33dc54..eacafe3d1c1ab 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts @@ -1,4 +1,4 @@ -import { ArnFormat, Lazy, Stack, Token } from '@aws-cdk/core'; +import { ArnFormat, Lazy, Stack, Token, Annotations } from '@aws-cdk/core'; import { Construct } from 'constructs'; import { IAlarmAction } from './alarm-action'; import { AlarmBase, IAlarm } from './alarm-base'; @@ -203,6 +203,10 @@ export class Alarm extends AlarmBase { label: `${this.metric} ${OPERATOR_SYMBOLS[comparisonOperator]} ${props.threshold} for ${datapoints} datapoints within ${describePeriod(props.evaluationPeriods * metricPeriod(props.metric).toSeconds())}`, value: props.threshold, }; + + for (const w of this.metric.warnings ?? []) { + Annotations.of(this).addWarning(w); + } } /** diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/dashboard.ts b/packages/@aws-cdk/aws-cloudwatch/lib/dashboard.ts index 91813d592463c..10f1251b53d91 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/dashboard.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/dashboard.ts @@ -1,4 +1,4 @@ -import { Lazy, Resource, Stack, Token } from '@aws-cdk/core'; +import { Lazy, Resource, Stack, Token, Annotations } from '@aws-cdk/core'; import { Construct } from 'constructs'; import { CfnDashboard } from './cloudwatch.generated'; import { Column, Row } from './layout'; @@ -127,7 +127,29 @@ export class Dashboard extends Resource { return; } + const warnings = allWidgetsDeep(widgets).flatMap(w => w.warnings ?? []); + for (const w of warnings) { + Annotations.of(this).addWarning(w); + } + const w = widgets.length > 1 ? new Row(...widgets) : widgets[0]; this.rows.push(w); } } + +function allWidgetsDeep(ws: IWidget[]) { + const ret = new Array(); + ws.forEach(recurse); + return ret; + + function recurse(w: IWidget) { + ret.push(w); + if (hasSubWidgets(w)) { + w.widgets.forEach(recurse); + } + } +} + +function hasSubWidgets(w: IWidget): w is IWidget & { widgets: IWidget[] } { + return 'widgets' in w; +} diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/graph.ts b/packages/@aws-cdk/aws-cloudwatch/lib/graph.ts index af23010e166c8..32a7b2aefef5c 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/graph.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/graph.ts @@ -256,6 +256,7 @@ export class GraphWidget extends ConcreteWidget { this.props = props; this.leftMetrics = props.left ?? []; this.rightMetrics = props.right ?? []; + this.copyMetricWarnings(...this.leftMetrics, ...this.rightMetrics); } /** @@ -265,6 +266,7 @@ export class GraphWidget extends ConcreteWidget { */ public addLeftMetric(metric: IMetric) { this.leftMetrics.push(metric); + this.copyMetricWarnings(metric); } /** @@ -274,6 +276,7 @@ export class GraphWidget extends ConcreteWidget { */ public addRightMetric(metric: IMetric) { this.rightMetrics.push(metric); + this.copyMetricWarnings(metric); } public toJson(): any[] { @@ -343,6 +346,7 @@ export class SingleValueWidget extends ConcreteWidget { constructor(props: SingleValueWidgetProps) { super(props.width || 6, props.height || 3); this.props = props; + this.copyMetricWarnings(...props.metrics); } public toJson(): any[] { diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/layout.ts b/packages/@aws-cdk/aws-cloudwatch/lib/layout.ts index 2798bdf5460e2..e9efdd856b053 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/layout.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/layout.ts @@ -14,7 +14,7 @@ export class Row implements IWidget { /** * List of contained widgets */ - private readonly widgets: IWidget[]; + public readonly widgets: IWidget[]; /** * Relative position of each widget inside this row @@ -70,7 +70,7 @@ export class Column implements IWidget { /** * List of contained widgets */ - private readonly widgets: IWidget[]; + public readonly widgets: IWidget[]; constructor(...widgets: IWidget[]) { this.widgets = widgets; diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts index e8506fde53140..1a968781b18d9 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts @@ -4,6 +4,15 @@ import { Duration } from '@aws-cdk/core'; * Interface for metrics */ export interface IMetric { + /** + * Any warnings related to this metric + * + * Should be attached to the consuming construct. + * + * @default - None + */ + readonly warnings?: string[]; + /** * Inspect the details of the metric object */ diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts index 28868b33d9883..8f959f37ea201 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts @@ -553,6 +553,11 @@ export class MathExpression implements IMetric { */ public readonly searchRegion?: string; + /** + * Warnings generated by this math expression + */ + public readonly warnings?: string[]; + constructor(props: MathExpressionProps) { this.period = props.period || cdk.Duration.minutes(5); this.expression = props.expression; @@ -568,6 +573,27 @@ export class MathExpression implements IMetric { } this.validateNoIdConflicts(); + + // Check that all IDs used in the expression are also in the `usingMetrics` map. We + // can't throw on this anymore since we didn't use to do this validation from the start + // and now there will be loads of people who are violating the expected contract, but + // we can add warnings. + const missingIdentifiers = allIdentifiersInExpression(this.expression).filter(i => !this.usingMetrics[i]); + + const warnings = []; + + if (missingIdentifiers.length > 0) { + warnings.push(`Math expression '${this.expression}' references unknown identifiers: ${missingIdentifiers.join(', ')}. Please add them to the 'usingMetrics' map.`); + } + + // Also copy warnings from deeper levels so graphs, alarms only have to inspect the top-level objects + for (const m of Object.values(this.usingMetrics)) { + warnings.push(...m.warnings ?? []); + } + + if (warnings.length > 0) { + this.warnings = warnings; + } } /** @@ -677,15 +703,27 @@ export class MathExpression implements IMetric { }); } } - } -const VALID_VARIABLE = new RegExp('^[a-z][a-zA-Z0-9_]*$'); +/** + * Pattern for a variable name. Alphanum starting with lowercase. + */ +const VARIABLE_PAT = '[a-z][a-zA-Z0-9_]*'; + +const VALID_VARIABLE = new RegExp(`^${VARIABLE_PAT}$`); +const FIND_VARIABLE = new RegExp(VARIABLE_PAT, 'g'); function validVariableName(x: string) { return VALID_VARIABLE.test(x); } +/** + * Return all variable names used in an expression + */ +function allIdentifiersInExpression(x: string) { + return Array.from(matchAll(x, FIND_VARIABLE)).map(m => m[0]); +} + /** * Properties needed to make an alarm from a metric */ @@ -842,3 +880,13 @@ interface IModifiableMetric { function isModifiableMetric(m: any): m is IModifiableMetric { return typeof m === 'object' && m !== null && !!m.with; } + +// Polyfill for string.matchAll(regexp) +function matchAll(x: string, re: RegExp): RegExpMatchArray[] { + const ret = new Array(); + let m: RegExpExecArray | null; + while (m = re.exec(x)) { + ret.push(m); + } + return ret; +} \ No newline at end of file diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/widget.ts b/packages/@aws-cdk/aws-cloudwatch/lib/widget.ts index 27873ea9421ed..a56d7c57d9e5a 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/widget.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/widget.ts @@ -1,3 +1,5 @@ +import { IMetric } from './metric-types'; + /** * The width of the grid we're filling */ @@ -17,6 +19,11 @@ export interface IWidget { */ readonly height: number; + /** + * Any warnings that are produced as a result of putting together this widget + */ + readonly warnings?: string[]; + /** * Place the widget at a given position */ @@ -39,6 +46,8 @@ export abstract class ConcreteWidget implements IWidget { protected x?: number; protected y?: number; + public readonly warnings: string[] | undefined = []; + constructor(width: number, height: number) { this.width = width; this.height = height; @@ -54,4 +63,11 @@ export abstract class ConcreteWidget implements IWidget { } public abstract toJson(): any[]; + + /** + * Copy the warnings from the given metric + */ + protected copyMetricWarnings(...ms: IMetric[]) { + this.warnings?.push(...ms.flatMap(m => m.warnings ?? [])); + } } diff --git a/packages/@aws-cdk/aws-cloudwatch/test/alarm.test.ts b/packages/@aws-cdk/aws-cloudwatch/test/alarm.test.ts index 85e6460bb0e08..6a2a9eb19885b 100644 --- a/packages/@aws-cdk/aws-cloudwatch/test/alarm.test.ts +++ b/packages/@aws-cdk/aws-cloudwatch/test/alarm.test.ts @@ -1,4 +1,4 @@ -import { Match, Template } from '@aws-cdk/assertions'; +import { Match, Template, Annotations } from '@aws-cdk/assertions'; import { Duration, Stack } from '@aws-cdk/core'; import { Construct } from 'constructs'; import { Alarm, IAlarm, IAlarmAction, Metric, MathExpression, IMetric } from '../lib'; @@ -252,6 +252,22 @@ describe('Alarm', () => { ExtendedStatistic: 'tm99.9999999999', }); }); + + test('metric warnings are added to Alarm', () => { + const stack = new Stack(undefined, 'MyStack'); + const m = new MathExpression({ expression: 'oops' }); + + // WHEN + new Alarm(stack, 'MyAlarm', { + metric: m, + evaluationPeriods: 1, + threshold: 1, + }); + + // THEN + const template = Annotations.fromStack(stack); + template.hasWarning('/MyStack/MyAlarm', Match.stringLikeRegexp("Math expression 'oops' references unknown identifiers")); + }); }); class TestAlarmAction implements IAlarmAction { diff --git a/packages/@aws-cdk/aws-cloudwatch/test/dashboard.test.ts b/packages/@aws-cdk/aws-cloudwatch/test/dashboard.test.ts index 201f0158c11c3..2d0943ceed962 100644 --- a/packages/@aws-cdk/aws-cloudwatch/test/dashboard.test.ts +++ b/packages/@aws-cdk/aws-cloudwatch/test/dashboard.test.ts @@ -1,6 +1,6 @@ -import { Template } from '@aws-cdk/assertions'; +import { Template, Annotations, Match } from '@aws-cdk/assertions'; import { App, Stack } from '@aws-cdk/core'; -import { Dashboard, GraphWidget, PeriodOverride, TextWidget } from '../lib'; +import { Dashboard, GraphWidget, PeriodOverride, TextWidget, MathExpression } from '../lib'; describe('Dashboard', () => { test('widgets in different adds are laid out underneath each other', () => { @@ -175,8 +175,23 @@ describe('Dashboard', () => { // THEN expect(() => toThrow()).toThrow(/field dashboardName contains invalid characters/); + }); + + test('metric warnings are added to dashboard', () => { + const app = new App(); + const stack = new Stack(app, 'MyStack'); + const m = new MathExpression({ expression: 'oops' }); + // WHEN + new Dashboard(stack, 'MyDashboard', { + widgets: [ + [new GraphWidget({ left: [m] }), new TextWidget({ markdown: 'asdf' })], + ], + }); + // THEN + const template = Annotations.fromStack(stack); + template.hasWarning('/MyStack/MyDashboard', Match.stringLikeRegexp("Math expression 'oops' references unknown identifiers")); }); }); diff --git a/packages/@aws-cdk/aws-cloudwatch/test/graphs.test.ts b/packages/@aws-cdk/aws-cloudwatch/test/graphs.test.ts index 0c1adfb58b1f8..bb2366cd18f79 100644 --- a/packages/@aws-cdk/aws-cloudwatch/test/graphs.test.ts +++ b/packages/@aws-cdk/aws-cloudwatch/test/graphs.test.ts @@ -681,8 +681,6 @@ describe('Graphs', () => { setPeriodToTimeRange: true, }, }]); - - }); test('GraphWidget supports stat and period', () => { @@ -710,7 +708,5 @@ describe('Graphs', () => { period: 172800, }, }]); - - }); }); diff --git a/packages/@aws-cdk/aws-cloudwatch/test/metric-math.test.ts b/packages/@aws-cdk/aws-cloudwatch/test/metric-math.test.ts index d4220f83d1b50..480a25e3d748e 100644 --- a/packages/@aws-cdk/aws-cloudwatch/test/metric-math.test.ts +++ b/packages/@aws-cdk/aws-cloudwatch/test/metric-math.test.ts @@ -65,8 +65,27 @@ describe('Metric Math', () => { expect(m.with({ period: Duration.minutes(10) })).toEqual(m); expect(m.with({ period: Duration.minutes(5) })).not.toEqual(m); + }); + + test('math expression referring to unknown expressions produces a warning', () => { + const m = new MathExpression({ + expression: 'm1 + m2', + }); + expect(m.warnings).toContainEqual(expect.stringContaining("'m1 + m2' references unknown identifiers")); + }); + + test('math expression referring to unknown expressions produces a warning, even when nested', () => { + const m = new MathExpression({ + expression: 'e1 + 5', + usingMetrics: { + e1: new MathExpression({ + expression: 'm1 + m2', + }), + }, + }); + expect(m.warnings).toContainEqual(expect.stringContaining("'m1 + m2' references unknown identifiers")); }); describe('in graphs', () => {