From 4e715e98e6bf6b8c5cc6bfea875ce47c2bc1bf06 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Fri, 7 Apr 2023 11:54:25 +0200 Subject: [PATCH] fix(cloudwatch): `p100` statistic is no longer recognized Since #23095 (which was released in 2.66.0), the `p100` statistic is no longer rendered into Alarms. Fixes #24976. --- .../aws-cdk-lib/aws-cloudwatch/lib/alarm.ts | 6 +- .../aws-cloudwatch/lib/private/statistic.ts | 137 +++++++----------- .../aws-cloudwatch/test/alarm.test.ts | 25 +++- .../aws-cloudwatch/test/metrics.test.ts | 5 +- 4 files changed, 81 insertions(+), 92 deletions(-) diff --git a/packages/aws-cdk-lib/aws-cloudwatch/lib/alarm.ts b/packages/aws-cdk-lib/aws-cloudwatch/lib/alarm.ts index edbc0248d6a2..e27cd509cd3d 100644 --- a/packages/aws-cdk-lib/aws-cloudwatch/lib/alarm.ts +++ b/packages/aws-cdk-lib/aws-cloudwatch/lib/alarm.ts @@ -425,7 +425,11 @@ function renderIfExtendedStatistic(statistic?: string): string | undefined { if (parsed.type === 'single' || parsed.type === 'pair') { return normalizeStatistic(parsed); } - return undefined; + + // We can't not render anything here. Just put whatever we got as input into + // the ExtendedStatistic and hope it's correct. Either that, or we throw + // an error. + return parsed.statistic; } function mathExprHasSubmetrics(expr: MetricExpressionConfig) { diff --git a/packages/aws-cdk-lib/aws-cloudwatch/lib/private/statistic.ts b/packages/aws-cdk-lib/aws-cloudwatch/lib/private/statistic.ts index db3bc69e45ff..395f347e97eb 100644 --- a/packages/aws-cdk-lib/aws-cloudwatch/lib/private/statistic.ts +++ b/packages/aws-cdk-lib/aws-cloudwatch/lib/private/statistic.ts @@ -60,30 +60,34 @@ function parseSingleStatistic(statistic: string, prefix: string): Omit 100) { + return undefined; + } + return { + type: 'single', + rawStatistic: statistic, + statPrefix: prefixLower, + value, + }; } +/** + * Parse a statistic that looks like `tm( LOWER : UPPER )`. + */ function parsePairStatistic(statistic: string, prefix: string): Omit | undefined { - const prefixUpper = prefix.toUpperCase(); - - // Allow `tm(10%:90%)` lowercase - statistic = statistic.toUpperCase(); - - if (!statistic.startsWith(prefixUpper)) { + const r = new RegExp(`^${prefix}\\(([^)]+)\\)$`, 'i').exec(statistic); + if (!r) { return undefined; } @@ -91,87 +95,44 @@ function parsePairStatistic(statistic: string, prefix: string): Omit { + x = x.trim(); + if (!x) { + return [undefined, false]; + } + const value = parseFloat(x.replace(/%$/, '')); + const percent = x.endsWith('%'); + if (isNaN(value) || value < 0 || (percent && value > 100)) { + return ['fail', false]; + } + return [value, percent]; + }; - // TM(:99.999%) - // /TM\(:(\d{1,2}(?:\.\d+)?)%\)/ - // Note: this can be represented as a single stat! TM(:90%) = tm90 - r = new RegExp(`^${prefixUpper}\\(\\:(\\d{1,2}(?:\\.\\d+)?)%\\)$`).exec(statistic); - if (r) { - return { - ...common, - canBeSingleStat: true, - asSingleStatStr: `${prefix.toLowerCase()}${r[1]}`, - lower: undefined, - upper: parseFloat(r[1]), - isPercent: true, - }; + const [lower, lhsPercent] = parseNumberAndPercent(lhs); + const [upper, rhsPercent] = parseNumberAndPercent(rhs); + if (lower === 'fail' || upper === 'fail' || (lower === undefined && upper === undefined)) { + return undefined; } - // TM(99.999:99.999) - // /TM\((\d{1,2}(?:\.\d+)?):(\d{1,2}(?:\.\d+)?)\)/ - r = new RegExp(`^${prefixUpper}\\((\\d+(?:\\.\\d+)?)\\:(\\d+(?:\\.\\d+)?)\\)$`).exec(statistic); - if (r) { - return { - ...common, - lower: parseFloat(r[1]), - upper: parseFloat(r[2]), - isPercent: false, - }; + if (lower !== undefined && upper !== undefined && lhsPercent !== rhsPercent) { + // If one value is a percentage, the other one must be too + return undefined; } - // TM(99.999%:99.999%) - // /TM\((\d{1,2}(?:\.\d+)?)%:(\d{1,2}(?:\.\d+)?)%\)/ - r = new RegExp(`^${prefixUpper}\\((\\d{1,2}(?:\\.\\d+)?)%\\:(\\d{1,2}(?:\\.\\d+)?)%\\)$`).exec(statistic); - if (r) { - return { - ...common, - lower: parseFloat(r[1]), - upper: parseFloat(r[2]), - isPercent: true, - }; - } + const isPercent = lhsPercent || rhsPercent; + const canBeSingleStat = lower === undefined && isPercent; + const asSingleStatStr = canBeSingleStat ? `${prefix.toLowerCase()}${upper}` : undefined; - return undefined; + return { ...common, lower, upper, isPercent, canBeSingleStat, asSingleStatStr }; } export function singleStatisticToString(parsed: SingleStatistic): string { diff --git a/packages/aws-cdk-lib/aws-cloudwatch/test/alarm.test.ts b/packages/aws-cdk-lib/aws-cloudwatch/test/alarm.test.ts index ca88d9fbf398..27ecb48e5287 100644 --- a/packages/aws-cdk-lib/aws-cloudwatch/test/alarm.test.ts +++ b/packages/aws-cdk-lib/aws-cloudwatch/test/alarm.test.ts @@ -1,6 +1,6 @@ +import { Construct } from 'constructs'; import { Match, Template, Annotations } from '../../assertions'; import { Duration, Stack } from '../../core'; -import { Construct } from 'constructs'; import { Alarm, IAlarm, IAlarmAction, Metric, MathExpression, IMetric, Stats } from '../lib'; const testMetric = new Metric({ @@ -359,6 +359,29 @@ describe('Alarm', () => { const template = Annotations.fromStack(stack); template.hasWarning('/MyStack/MyAlarm', Match.stringLikeRegexp("Math expression 'oops' references unknown identifiers")); }); + + test('check alarm for p100 statistic', () => { + const stack = new Stack(undefined, 'MyStack'); + new Alarm(stack, 'MyAlarm', { + metric: new Metric({ + dimensionsMap: { + Boop: 'boop', + }, + metricName: 'MyMetric', + namespace: 'MyNamespace', + period: Duration.minutes(1), + statistic: Stats.p(100), + }), + evaluationPeriods: 1, + threshold: 1, + }); + + // THEN + const template = Template.fromStack(stack); + template.hasResourceProperties('AWS::CloudWatch::Alarm', { + bier: 'bier', + }); + }); }); class TestAlarmAction implements IAlarmAction { diff --git a/packages/aws-cdk-lib/aws-cloudwatch/test/metrics.test.ts b/packages/aws-cdk-lib/aws-cloudwatch/test/metrics.test.ts index 17e07609043b..374364f3727a 100644 --- a/packages/aws-cdk-lib/aws-cloudwatch/test/metrics.test.ts +++ b/packages/aws-cdk-lib/aws-cloudwatch/test/metrics.test.ts @@ -288,6 +288,7 @@ describe('Metrics', () => { checkParsingSingle('p99', 'p', 'percentile', 99); checkParsingSingle('P99', 'p', 'percentile', 99); checkParsingSingle('p99.99', 'p', 'percentile', 99.99); + checkParsingSingle('p100', 'p', 'percentile', 100); checkParsingSingle('tm99', 'tm', 'trimmedMean', 99); checkParsingSingle('wm99', 'wm', 'winsorizedMean', 99); checkParsingSingle('tc99', 'tc', 'trimmedCount', 99); @@ -337,8 +338,8 @@ describe('Metrics', () => { expect(parseStatistic('TM(10%:1500)').type).toEqual('generic'); expect(parseStatistic('TM(10)').type).toEqual('generic'); expect(parseStatistic('TM()').type).toEqual('generic'); - expect(parseStatistic('TM(0.:)').type).toEqual('generic'); - expect(parseStatistic('TM(:0.)').type).toEqual('generic'); + expect(parseStatistic('TM(0.:)').type).toEqual('pair'); + expect(parseStatistic('TM(:0.)').type).toEqual('pair'); expect(parseStatistic('()').type).toEqual('generic'); expect(parseStatistic('(:)').type).toEqual('generic'); expect(parseStatistic('TM(:)').type).toEqual('generic');