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*

Author: rix0rrr

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);
+    }
   }
 
   /**

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<IWidget>();
+  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;
+}

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[] {

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;

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
    */

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<RegExpMatchArray>();
+  let m: RegExpExecArray | null;
+  while (m = re.exec(x)) {
+    ret.push(m);
+  }
+  return ret;
+}

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 ?? []));
+  }
 }

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 {

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"));
   });
 });
 

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,
       },
     }]);
-
-
   });
 });

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', () => {