Skip to content

Commit

Permalink
add warnings
Browse files Browse the repository at this point in the history
  • Loading branch information
@go-to-k
go-to-k committed Sep 5, 2024
1 parent b8b2ae0 commit 712e937
Show file tree
Hide file tree
Showing 2 changed files with 118 additions and 10 deletions.
44 changes: 34 additions & 10 deletions packages/aws-cdk-lib/aws-cloudwatch/lib/metric.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.`);
Expand All @@ -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.`;
}
Expand Down Expand Up @@ -908,23 +914,33 @@ function ifUndefined<T>(x: T | undefined, def: T | undefined): T | undefined {
/**
* Change periods of all metrics in the map
*/
function changeAllPeriods(metrics: Record<string, IMetric>, period: cdk.Duration): Record<string, IMetric> {
const ret: Record<string, IMetric> = {};
for (const [id, metric] of Object.entries(metrics)) {
ret[id] = changePeriod(metric, period);
function changeAllPeriods(metrics: Record<string, IMetric>, period: cdk.Duration): { record: Record<string, IMetric>; overridden: boolean } {
const retRecord: Record<string, IMetric> = {};
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}`);
Expand Down Expand Up @@ -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<RegExpMatchArray>();
Expand Down
84 changes: 84 additions & 0 deletions packages/aws-cdk-lib/aws-cloudwatch/test/metric-math.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 712e937

Please sign in to comment.