From 60253a319d6f185cf807ca45dac4ce0be4ab5777 Mon Sep 17 00:00:00 2001
From: Rico Huijbers <rix0rrr@gmail.com>
Date: Fri, 20 Mar 2020 14:21:25 +0100
Subject: [PATCH] fix(cloudwatch): unhelpful error when reusing metric IDs
 (#6892)

Reuse of the same metric ID was already checked upon construction of the
`MathExpression` instance and so we thought this error couldn't occur.
However, reuse can also occur (and is also prohibited) between different
`MathExpression`s, even on different axes.

Make the error message describe the situation in a way that is
actionable.
---
 packages/@aws-cdk/aws-cloudwatch/lib/graph.ts |  9 ++--
 .../aws-cloudwatch/lib/private/rendering.ts   |  2 +-
 .../aws-cloudwatch/test/test.metric-math.ts   | 52 +++++++++++++++++++
 3 files changed, 59 insertions(+), 4 deletions(-)

diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/graph.ts b/packages/@aws-cdk/aws-cloudwatch/lib/graph.ts
index 456a26bb7be3d..a642359399231 100644
--- a/packages/@aws-cdk/aws-cloudwatch/lib/graph.ts
+++ b/packages/@aws-cdk/aws-cloudwatch/lib/graph.ts
@@ -168,8 +168,11 @@ export class GraphWidget extends ConcreteWidget {
   }
 
   public toJson(): any[] {
-    const horizontalAnnoations =  (this.props.leftAnnotations || []).map(mapAnnotation('left')).concat(
-      (this.props.rightAnnotations || []).map(mapAnnotation('right')));
+    const horizontalAnnotations = [
+      ...(this.props.leftAnnotations || []).map(mapAnnotation('left')),
+      ...(this.props.rightAnnotations || []).map(mapAnnotation('right')),
+    ];
+
     const metrics = allMetricsGraphJson(this.props.left || [], this.props.right || []);
     return [{
       type: 'metric',
@@ -183,7 +186,7 @@ export class GraphWidget extends ConcreteWidget {
         region: this.props.region || cdk.Aws.REGION,
         stacked: this.props.stacked,
         metrics: metrics.length > 0 ? metrics : undefined,
-        annotations: horizontalAnnoations.length > 0 ? { horizontal: horizontalAnnoations } : undefined,
+        annotations: horizontalAnnotations.length > 0 ? { horizontal: horizontalAnnotations } : undefined,
         yAxis: {
           left: this.props.leftYAxis !== undefined ? this.props.leftYAxis : undefined,
           right: this.props.rightYAxis !== undefined ? this.props.rightYAxis : undefined,
diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/private/rendering.ts b/packages/@aws-cdk/aws-cloudwatch/lib/private/rendering.ts
index b52ca5a143308..7bcaef16bd2f0 100644
--- a/packages/@aws-cdk/aws-cloudwatch/lib/private/rendering.ts
+++ b/packages/@aws-cdk/aws-cloudwatch/lib/private/rendering.ts
@@ -142,7 +142,7 @@ export class MetricSet<A> {
     if (id) {
       existingEntry = this.metricById.get(id);
       if (existingEntry && metricKey(existingEntry.metric) !== key) {
-        throw new Error(`Can't happen, already checked elsewhere`);
+        throw new Error(`Cannot have two different metrics share the same id ('${id}') in one Alarm or Graph. Rename one of them.`);
       }
     }
 
diff --git a/packages/@aws-cdk/aws-cloudwatch/test/test.metric-math.ts b/packages/@aws-cdk/aws-cloudwatch/test/test.metric-math.ts
index 12300f6bac1b6..c4849dad1d653 100644
--- a/packages/@aws-cdk/aws-cloudwatch/test/test.metric-math.ts
+++ b/packages/@aws-cdk/aws-cloudwatch/test/test.metric-math.ts
@@ -268,6 +268,58 @@ export = {
 
       test.done();
     },
+
+    'can reuse the same metric between left and right axes'(test: Test) {
+      // GIVEN
+      const graph = new GraphWidget({
+        left: [
+          new MathExpression({
+            expression: 'a + 1',
+            usingMetrics: { a }
+          })
+        ],
+        right: [
+          new MathExpression({
+            expression: 'a + 2',
+            usingMetrics: { a }
+          })
+        ]
+      });
+
+      // THEN
+      graphMetricsAre(test, graph, [
+        [ { label: 'a + 1', expression: 'a + 1' } ],
+        [ 'Test', 'ACount', { visible: false, id: 'a' } ],
+        [ { label: 'a + 2', expression: 'a + 2', yAxis: 'right' } ]
+      ]);
+
+      test.done();
+    },
+
+    'detect name conflicts between left and right axes'(test: Test) {
+      // GIVEN
+      const graph = new GraphWidget({
+        left: [
+          new MathExpression({
+            expression: 'm1 + 1',
+            usingMetrics: { m1: a }
+          })
+        ],
+        right: [
+          new MathExpression({
+            expression: 'm1 + 1',
+            usingMetrics: { m1: b }
+          })
+        ]
+      });
+
+      // THEN
+      test.throws(() => {
+        graphMetricsAre(test, graph, []);
+      }, /Cannot have two different metrics share the same id \('m1'\)/);
+
+      test.done();
+    },
   },
 
   'in alarms': {