From 59e96a36f559d51467b00e92102bd9450f38a139 Mon Sep 17 00:00:00 2001 From: "Kenta Goto (k.goto)" <24818752+go-to-k@users.noreply.github.com> Date: Wed, 11 Dec 2024 09:29:15 +0900 Subject: [PATCH] fix(cloudwatch): `period` of each metric in `usingMetrics` for `MathExpression` is ignored (#30986) ### Issue # (if applicable) Closes #. ### Reason for this change The `usingMetrics` property (`Record`) in `MathExpressionProps` has Metric objects with a `period`. On the other hand, in the `MathExpression` construct, the `period` of each metric in the `usingMetrics` is ignored and instead overridden by the `period` of the `MathExpression`. Even if the `period` of the `MathExpression` is not specified, it is overridden by its default value. https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-cloudwatch/lib/metric.ts#L606-L608 However the statement is not written in the JSDoc. https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-cloudwatch/lib/metric.ts#L566 ```ts new MathExpression({ expression: "m1+m2", label: "AlbErrors", usingMetrics: { m1: metrics.custom("HTTPCode_ELB_500_Count", { period: Duration.minutes(1), // <- ignored and overridden by default value `Duration.minutes(5)` of `period` in the `MathExpressionOptions` statistic: "Sum", label: "HTTPCode_ELB_500_Count", }), m2: metrics.custom("HTTPCode_ELB_502_Count", { period: Duration.minutes(1), // <- ignored and overridden by default value `Duration.minutes(5)` of `period` in the `MathExpressionOptions` statistic: "Sum", label: "HTTPCode_ELB_502_Count", }), }, }), ``` ### Description of changes The current documentation could be confusing to users, so add this description. Also added warnings in the situation. ### Description of how you validated changes ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- .../aws-cdk-lib/aws-cloudwatch/lib/metric.ts | 73 +++++++++++++--- .../aws-cloudwatch/test/metric-math.test.ts | 84 +++++++++++++++++++ .../rosetta/aws_cloudwatch/default.ts-fixture | 1 + 3 files changed, 148 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 8b6c05bc45122..428598e9602b5 100644 --- a/packages/aws-cdk-lib/aws-cloudwatch/lib/metric.ts +++ b/packages/aws-cdk-lib/aws-cloudwatch/lib/metric.ts @@ -226,6 +226,35 @@ 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 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. */ readonly usingMetrics?: Record; @@ -605,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.`); @@ -624,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.`; } @@ -879,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}`); @@ -927,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 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';