From af0c17605d9de8f6c7eec432bd870532f19ff88e Mon Sep 17 00:00:00 2001 From: go-to-k <24818752+go-to-k@users.noreply.github.com> Date: Wed, 31 Jul 2024 17:56:59 +0900 Subject: [PATCH 1/4] docs(cloudwatch): add description of ignoring period of each metrics in math expression --- packages/aws-cdk-lib/aws-cloudwatch/lib/metric.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/aws-cdk-lib/aws-cloudwatch/lib/metric.ts b/packages/aws-cdk-lib/aws-cloudwatch/lib/metric.ts index 8b6c05bc45122..65cf36d20c9aa 100644 --- a/packages/aws-cdk-lib/aws-cloudwatch/lib/metric.ts +++ b/packages/aws-cdk-lib/aws-cloudwatch/lib/metric.ts @@ -226,6 +226,9 @@ export interface MathExpressionProps extends MathExpressionOptions { * The key is the identifier that represents the given metric in the * expression, and the value is the actual Metric object. * + * The `period` of each Metric object is ignored and instead overridden + * by the `period` of this MathExpression object. + * * @default - Empty map. */ readonly usingMetrics?: Record; From 91f64bb9634a3dc733773ce81e56a46b109189db Mon Sep 17 00:00:00 2001 From: go-to-k <24818752+go-to-k@users.noreply.github.com> Date: Wed, 31 Jul 2024 18:32:30 +0900 Subject: [PATCH 2/4] change statements --- packages/aws-cdk-lib/aws-cloudwatch/lib/metric.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/aws-cdk-lib/aws-cloudwatch/lib/metric.ts b/packages/aws-cdk-lib/aws-cloudwatch/lib/metric.ts index 65cf36d20c9aa..444d1073220af 100644 --- a/packages/aws-cdk-lib/aws-cloudwatch/lib/metric.ts +++ b/packages/aws-cdk-lib/aws-cloudwatch/lib/metric.ts @@ -226,8 +226,9 @@ export interface MathExpressionProps extends MathExpressionOptions { * The key is the identifier that represents the given metric in the * expression, and the value is the actual Metric object. * - * The `period` of each Metric object is ignored and instead overridden - * by the `period` of this MathExpression object. + * The `period` of each Metric object is ignored and instead overridden by + * the `period` of this math expression object. Even if the `period` of the + * math expression is not specified, it is overridden by its default value. * * @default - Empty map. */ From b8b2ae083413f0a3a4bd7b0675e32c92ee1ccef8 Mon Sep 17 00:00:00 2001 From: go-to-k <24818752+go-to-k@users.noreply.github.com> Date: Thu, 5 Sep 2024 16:52:33 +0900 Subject: [PATCH 3/4] modify comments and add sample code fix for rosetta --- .../aws-cdk-lib/aws-cloudwatch/lib/metric.ts | 31 +++++++++++++++++-- .../rosetta/aws_cloudwatch/default.ts-fixture | 1 + 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/packages/aws-cdk-lib/aws-cloudwatch/lib/metric.ts b/packages/aws-cdk-lib/aws-cloudwatch/lib/metric.ts index 444d1073220af..e0594987cbd42 100644 --- a/packages/aws-cdk-lib/aws-cloudwatch/lib/metric.ts +++ b/packages/aws-cdk-lib/aws-cloudwatch/lib/metric.ts @@ -226,9 +226,34 @@ export interface MathExpressionProps extends MathExpressionOptions { * The key is the identifier that represents the given metric in the * expression, and the value is the actual Metric object. * - * The `period` of each Metric object is ignored and instead overridden by - * the `period` of this math expression object. Even if the `period` of the - * math expression is not specified, it is overridden by its default value. + * The `period` of each metric in `usingMetrics` is ignored and instead overridden + * by the `period` specified for the `MathExpression` construct. Even if no `period` + * is specified for the `MathExpression`, it will be overridden by the default + * value (`Duration.minutes(5)`). + * + * Example: + * + * ```ts + * declare const metrics: elbv2.IApplicationLoadBalancerMetrics; + * new cloudwatch.MathExpression({ + * expression: 'm1+m2', + * label: 'AlbErrors', + * usingMetrics: { + * m1: metrics.custom('HTTPCode_ELB_500_Count', { + * period: Duration.minutes(1), // <- This period will be ignored + * statistic: 'Sum', + * label: 'HTTPCode_ELB_500_Count', + * }), + * m2: metrics.custom('HTTPCode_ELB_502_Count', { + * period: Duration.minutes(1), // <- This period will be ignored + * statistic: 'Sum', + * label: 'HTTPCode_ELB_502_Count', + * }), + * }, + * period: Duration.minutes(3), // <- This overrides the period of each metric in `usingMetrics` + * // (Even if not specified, it is overridden by the default value) + * }); + * ``` * * @default - Empty map. */ diff --git a/packages/aws-cdk-lib/rosetta/aws_cloudwatch/default.ts-fixture b/packages/aws-cdk-lib/rosetta/aws_cloudwatch/default.ts-fixture index 8783a1272fe84..399a0d1eee7c3 100644 --- a/packages/aws-cdk-lib/rosetta/aws_cloudwatch/default.ts-fixture +++ b/packages/aws-cdk-lib/rosetta/aws_cloudwatch/default.ts-fixture @@ -2,6 +2,7 @@ import { Construct } from 'constructs'; import { Stack, Duration } from 'aws-cdk-lib'; import * as cloudwatch from 'aws-cdk-lib/aws-cloudwatch'; +import * as elbv2 from 'aws-cdk-lib/aws-elasticloadbalancingv2'; import * as route53 from 'aws-cdk-lib/aws-route53'; import * as sns from 'aws-cdk-lib/aws-sns'; import * as lambda from 'aws-cdk-lib/aws-lambda'; 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 4/4] 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