From 712e937e0a5c5fd69e6e582320cdff5cd839ea81 Mon Sep 17 00:00:00 2001 From: go-to-k <24818752+go-to-k@users.noreply.github.com> Date: Thu, 5 Sep 2024 20:42:00 +0900 Subject: [PATCH] add warnings --- .../aws-cdk-lib/aws-cloudwatch/lib/metric.ts | 44 +++++++--- .../aws-cloudwatch/test/metric-math.test.ts | 84 +++++++++++++++++++ 2 files changed, 118 insertions(+), 10 deletions(-) diff --git a/packages/aws-cdk-lib/aws-cloudwatch/lib/metric.ts b/packages/aws-cdk-lib/aws-cloudwatch/lib/metric.ts index e0594987cbd42..428598e9602b5 100644 --- a/packages/aws-cdk-lib/aws-cloudwatch/lib/metric.ts +++ b/packages/aws-cdk-lib/aws-cloudwatch/lib/metric.ts @@ -634,12 +634,19 @@ export class MathExpression implements IMetric { constructor(props: MathExpressionProps) { this.period = props.period || cdk.Duration.minutes(5); this.expression = props.expression; - this.usingMetrics = changeAllPeriods(props.usingMetrics ?? {}, this.period); this.label = props.label; this.color = props.color; this.searchAccount = props.searchAccount; this.searchRegion = props.searchRegion; + const { record, overridden } = changeAllPeriods(props.usingMetrics ?? {}, this.period); + this.usingMetrics = record; + + const warnings: { [id: string]: string } = {}; + if (overridden) { + warnings['CloudWatch:Math:MetricsPeriodsOverridden'] = `Periods of metrics in 'usingMetrics' for Math expression '${this.expression}' have been overridden to ${this.period.toSeconds()} seconds.`; + } + const invalidVariableNames = Object.keys(this.usingMetrics).filter(x => !validVariableName(x)); if (invalidVariableNames.length > 0) { throw new Error(`Invalid variable names in expression: ${invalidVariableNames}. Must start with lowercase letter and only contain alphanumerics.`); @@ -653,7 +660,6 @@ export class MathExpression implements IMetric { // we can add warnings. const missingIdentifiers = allIdentifiersInExpression(this.expression).filter(i => !this.usingMetrics[i]); - const warnings: { [id: string]: string } = {}; if (!this.expression.toUpperCase().match('\\s*INSIGHT_RULE_METRIC|SELECT|SEARCH|METRICS\\s.*') && missingIdentifiers.length > 0) { warnings['CloudWatch:Math:UnknownIdentifier'] = `Math expression '${this.expression}' references unknown identifiers: ${missingIdentifiers.join(', ')}. Please add them to the 'usingMetrics' map.`; } @@ -908,23 +914,33 @@ function ifUndefined(x: T | undefined, def: T | undefined): T | undefined { /** * Change periods of all metrics in the map */ -function changeAllPeriods(metrics: Record, period: cdk.Duration): Record { - const ret: Record = {}; - for (const [id, metric] of Object.entries(metrics)) { - ret[id] = changePeriod(metric, period); +function changeAllPeriods(metrics: Record, period: cdk.Duration): { record: Record; overridden: boolean } { + const retRecord: Record = {}; + let retOverridden = false; + for (const [id, m] of Object.entries(metrics)) { + const { metric, overridden } = changePeriod(m, period); + retRecord[id] = metric; + if (overridden) { + retOverridden = true; + } } - return ret; + return { record: retRecord, overridden: retOverridden }; } /** - * Return a new metric object which is the same type as the input object, but with the period changed + * Return a new metric object which is the same type as the input object but with the period changed, + * and a flag to indicate whether the period has been overwritten. * * Relies on the fact that implementations of `IMetric` are also supposed to have * an implementation of `with` that accepts an argument called `period`. See `IModifiableMetric`. */ -function changePeriod(metric: IMetric, period: cdk.Duration): IMetric { +function changePeriod(metric: IMetric, period: cdk.Duration): { metric: IMetric; overridden: boolean} { if (isModifiableMetric(metric)) { - return metric.with({ period }); + const overridden = + isMetricWithPeriod(metric) && // always true, as the period property is set with a default value even if it is not specified + metric.period.toSeconds() !== cdk.Duration.minutes(5).toSeconds() && // exclude the default value of a metric, assuming the user has not specified it + metric.period.toSeconds() !== period.toSeconds(); + return { metric: metric.with({ period }), overridden }; } throw new Error(`Metric object should also implement 'with': ${metric}`); @@ -956,6 +972,14 @@ function isModifiableMetric(m: any): m is IModifiableMetric { return typeof m === 'object' && m !== null && !!m.with; } +interface IMetricWithPeriod { + period: cdk.Duration; +} + +function isMetricWithPeriod(m: any): m is IMetricWithPeriod { + return typeof m === 'object' && m !== null && !!m.period; +} + // Polyfill for string.matchAll(regexp) function matchAll(x: string, re: RegExp): RegExpMatchArray[] { const ret = new Array(); diff --git a/packages/aws-cdk-lib/aws-cloudwatch/test/metric-math.test.ts b/packages/aws-cdk-lib/aws-cloudwatch/test/metric-math.test.ts index 3b73f4193ddde..843f87379ae4f 100644 --- a/packages/aws-cdk-lib/aws-cloudwatch/test/metric-math.test.ts +++ b/packages/aws-cdk-lib/aws-cloudwatch/test/metric-math.test.ts @@ -124,6 +124,90 @@ describe('Metric Math', () => { }); }); + test('warn if a period is specified in usingMetrics and not equal to the value of the period for MathExpression', () => { + const m = new MathExpression({ + expression: 'm1', + usingMetrics: { + m1: new Metric({ namespace: 'Test', metricName: 'ACount', period: Duration.minutes(4) }), + }, + period: Duration.minutes(3), + }); + + expect(m.warningsV2).toMatchObject({ + 'CloudWatch:Math:MetricsPeriodsOverridden': 'Periods of metrics in \'usingMetrics\' for Math expression \'m1\' have been overridden to 180 seconds.', + }); + }); + + test('warn if periods are specified in usingMetrics and one is not equal to the value of the period for MathExpression', () => { + const m = new MathExpression({ + expression: 'm1 + m2', + usingMetrics: { + m1: new Metric({ namespace: 'Test', metricName: 'ACount', period: Duration.minutes(4) }), + m2: new Metric({ namespace: 'Test', metricName: 'BCount', period: Duration.minutes(3) }), + }, + period: Duration.minutes(3), + }); + + expect(m.warningsV2).toMatchObject({ + 'CloudWatch:Math:MetricsPeriodsOverridden': 'Periods of metrics in \'usingMetrics\' for Math expression \'m1 + m2\' have been overridden to 180 seconds.', + }); + }); + + test('warn if a period is specified in usingMetrics and not equal to the default value of the period for MathExpression', () => { + const m = new MathExpression({ + expression: 'm1', + usingMetrics: { + m1: new Metric({ namespace: 'Test', metricName: 'ACount', period: Duration.minutes(4) }), + }, + }); + + expect(m.warningsV2).toMatchObject({ + 'CloudWatch:Math:MetricsPeriodsOverridden': 'Periods of metrics in \'usingMetrics\' for Math expression \'m1\' have been overridden to 300 seconds.', + }); + }); + + test('can raise multiple warnings', () => { + const m = new MathExpression({ + expression: 'e1 + m1', + usingMetrics: { + e1: new MathExpression({ + expression: 'n1 + n2', + }), + m1: new Metric({ namespace: 'Test', metricName: 'ACount', period: Duration.minutes(4) }), + }, + period: Duration.minutes(3), + }); + + expect(m.warningsV2).toMatchObject({ + 'CloudWatch:Math:MetricsPeriodsOverridden': 'Periods of metrics in \'usingMetrics\' for Math expression \'e1 + m1\' have been overridden to 180 seconds.', + 'CloudWatch:Math:UnknownIdentifier': expect.stringContaining("'n1 + n2' references unknown identifiers"), + }); + }); + + test('don\'t warn if a period is not specified in usingMetrics', () => { + const m = new MathExpression({ + expression: 'm1', + usingMetrics: { + m1: new Metric({ namespace: 'Test', metricName: 'ACount' }), + }, + period: Duration.minutes(3), + }); + + expect(m.warningsV2).toBeUndefined(); + }); + + test('don\'t warn if a period is specified in usingMetrics but equal to the value of the period for MathExpression', () => { + const m = new MathExpression({ + expression: 'm1', + usingMetrics: { + m1: new Metric({ namespace: 'Test', metricName: 'ACount', period: Duration.minutes(3) }), + }, + period: Duration.minutes(3), + }); + + expect(m.warningsV2).toBeUndefined(); + }); + describe('in graphs', () => { test('MathExpressions can be added to a graph', () => { // GIVEN