Skip to content

Commit

Permalink
fix(cloudwatch): math expressions incorrectly warn about search and m…
Browse files Browse the repository at this point in the history
…etrics (#24313)

Closes [#20136](#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 55108b9 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*
  • Loading branch information
ParkerVR committed Feb 24, 2023
1 parent 5a0c627 commit f3596eb
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 2 deletions.
4 changes: 2 additions & 2 deletions packages/@aws-cdk/aws-cloudwatch/lib/metric.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.`);
}

Expand Down
18 changes: 18 additions & 0 deletions packages/@aws-cdk/aws-cloudwatch/test/metric-math.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'",
Expand Down

0 comments on commit f3596eb

Please sign in to comment.