Skip to content

Commit

Permalink
fix(cloudwatch): p100 statistic is no longer recognized
Browse files Browse the repository at this point in the history 
Since #23095 (which was released in 2.66.0), the `p100` statistic
is no longer rendered into Alarms.

Fixes #24976.
  • Loading branch information
@rix0rrr
rix0rrr committed Apr 7, 2023
1 parent 2bef1b0 commit 4e715e9
Show file tree
Hide file tree
Showing 4 changed files with 81 additions and 92 deletions.
6 changes: 5 additions & 1 deletion packages/aws-cdk-lib/aws-cloudwatch/lib/alarm.ts
View file
Expand Up @@ -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) {
Expand Down
137 changes: 49 additions & 88 deletions packages/aws-cdk-lib/aws-cloudwatch/lib/private/statistic.ts
View file
Expand Up @@ -60,118 +60,79 @@ function parseSingleStatistic(statistic: string, prefix: string): Omit<SingleSta
return undefined;
}

let r: RegExpExecArray | null = null;
// A decimal positive number regex (1, 1.2, 99.999, etc)
const reDecimal = '\\d+(?:\\.\\d+)?';

// p99.99
// /^p(\d{1,2}(?:\.\d+)?)$/
r = new RegExp(`^${prefixLower}(\\d{1,2}(?:\\.\\d+)?)$`).exec(statistic);
if (r) {
return {
type: 'single',
rawStatistic: statistic,
statPrefix: prefixLower,
value: parseFloat(r[1]),
};
// /^p(\d+(?:\.\d+)?)$/
const r = new RegExp(`^${prefixLower}(${reDecimal})$`).exec(statistic);
if (!r) {
return undefined;
}

return undefined;
const value = parseFloat(r[1]);
if (value < 0 || value > 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<PairStatistic, 'statName'> | 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;
}

const common: Omit<PairStatistic, 'statName' | 'isPercent'> = {
type: 'pair',
canBeSingleStat: false,
rawStatistic: statistic,
statPrefix: prefixUpper,
statPrefix: prefix.toUpperCase(),
};

let r: RegExpExecArray | null = null;

// TM(99.999:)
// /TM\((\d{1,2}(?:\.\d+)?):\)/
r = new RegExp(`^${prefixUpper}\\((\\d+(?:\\.\\d+)?)\\:\\)$`).exec(statistic);
if (r) {
return {
...common,
lower: parseFloat(r[1]),
upper: undefined,
isPercent: false,
};
}

// TM(99.999%:)
// /TM\((\d{1,2}(?:\.\d+)?)%:\)/
r = new RegExp(`^${prefixUpper}\\((\\d{1,2}(?:\\.\\d+)?)%\\:\\)$`).exec(statistic);
if (r) {
return {
...common,
lower: parseFloat(r[1]),
upper: undefined,
isPercent: true,
};
const [lhs, rhs] = r[1].split(':');
if (rhs === undefined) {
// Doesn't have 2 parts
return undefined;
}

// TM(:99.999)
// /TM\(:(\d{1,2}(?:\.\d+)?)\)/
r = new RegExp(`^${prefixUpper}\\(\\:(\\d+(?:\\.\\d+)?)\\)$`).exec(statistic);
if (r) {
return {
...common,
lower: undefined,
upper: parseFloat(r[1]),
isPercent: false,
};
}
const parseNumberAndPercent = (x: string): [number | undefined | 'fail', boolean] => {
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 {
Expand Down
25 changes: 24 additions & 1 deletion packages/aws-cdk-lib/aws-cloudwatch/test/alarm.test.ts
View file
@@ -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({
Expand Down Expand Up @@ -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 {
Expand Down
5 changes: 3 additions & 2 deletions packages/aws-cdk-lib/aws-cloudwatch/test/metrics.test.ts
View file
Expand Up @@ -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);
Expand Down Expand Up @@ -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');
Expand Down

0 comments on commit 4e715e9

Please sign in to comment.