From f3596eb26f1e4ab360875bf5f79a7de991d2a9ec Mon Sep 17 00:00:00 2001 From: Parker Van Roy Date: Fri, 24 Feb 2023 04:22:00 -0500 Subject: [PATCH] fix(cloudwatch): math expressions incorrectly warn about search and metrics (#24313) Closes [#20136](https://github.com/aws/aws-cdk/issues/20136). It is intended that all metric identifiers referenced in a MathExpression are included in the usingMetrics map and we will raise warnings if the customer does not follow this contract. However for SEARCH and METRICS queries, we can refer directly to metrics attribute values inside the query. Therefore we should not raise warnings. Change made based on work done in https://github.com/aws/aws-cdk/commit/55108b969a94f671a492b4536d2ad9d13d11cf9d with regex extended to a few other expressions. Looks like integ requests will not be required based on that commit. I had some firsthand experience getting thousands of this warning message after upgrading to CDK 2 and decided it would be easier to fix than suppress the excessive warnings. ---- *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/metric.ts | 4 ++-- .../aws-cloudwatch/test/metric-math.test.ts | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts index 5068187f9b9ae..770c3604c3c5a 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts @@ -609,9 +609,9 @@ export class MathExpression implements IMetric { // we can add warnings. const missingIdentifiers = allIdentifiersInExpression(this.expression).filter(i => !this.usingMetrics[i]); - const warnings = []; + const warnings: string[] = []; - if (!this.expression.toUpperCase().match('\\s*SELECT\\s.*') && missingIdentifiers.length > 0) { + if (!this.expression.toUpperCase().match('\\s*SELECT|SEARCH|METRICS\\s.*') && missingIdentifiers.length > 0) { warnings.push(`Math expression '${this.expression}' references unknown identifiers: ${missingIdentifiers.join(', ')}. Please add them to the 'usingMetrics' map.`); } 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 72e61cbd0a19a..24ffebfcfb76f 100644 --- a/packages/@aws-cdk/aws-cloudwatch/test/metric-math.test.ts +++ b/packages/@aws-cdk/aws-cloudwatch/test/metric-math.test.ts @@ -75,6 +75,24 @@ describe('Metric Math', () => { expect(m.warnings).toContainEqual(expect.stringContaining("'m1 + m2' references unknown identifiers")); }); + test('metrics METRICS expression does not produce warning for unknown identifier', () => { + const m = new MathExpression({ + expression: 'SUM(METRICS())', + usingMetrics: {}, + }); + + expect(m.warnings).toBeUndefined(); + }); + + test('metrics search expression does not produce warning for unknown identifier', () => { + const m = new MathExpression({ + expression: "SEARCH('{dimension_one, dimension_two} my_metric', 'Average', 300)", + usingMetrics: {}, + }); + + expect(m.warnings).toBeUndefined(); + }); + test('metrics insights expression does not produce warning for unknown identifier', () => { const m = new MathExpression({ expression: "SELECT AVG(CpuUsage) FROM EC2 WHERE Instance = '123456'",