From b59aed01c2a7a6ddcac1cd6530f0603707594a9c Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Thu, 30 Apr 2020 18:45:23 +0200 Subject: [PATCH] fix(cloudwatch): Alarm can't use `MathExpression` without submetrics `MathExpression`s without submetrics (like for example, `INSIGHT_RULE_METRIC`) will end up without a `period`, which is not allowed. Add a `period` field to the schema (it's not in the upstream schema yet), and render it out when submetrics are missing. Fixes #7155. --- packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts | 7 +++++- .../aws-cloudwatch/lib/metric-types.ts | 5 +++++ .../@aws-cdk/aws-cloudwatch/lib/metric.ts | 1 + packages/@aws-cdk/aws-cloudwatch/package.json | 3 ++- .../aws-cloudwatch/test/test.metric-math.ts | 22 +++++++++++++++++++ ...ch_Alarm_MetricDataQuery_Period_patch.json | 19 ++++++++++++++++ 6 files changed, 55 insertions(+), 2 deletions(-) create mode 100644 packages/@aws-cdk/cfnspec/spec-source/500_CloudWatch_Alarm_MetricDataQuery_Period_patch.json diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts b/packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts index 6301cfe93684f..4145922fcfdf7 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts @@ -3,7 +3,7 @@ import { IAlarmAction } from './alarm-action'; import { CfnAlarm, CfnAlarmProps } from './cloudwatch.generated'; import { HorizontalAnnotation } from './graph'; import { CreateAlarmOptions } from './metric'; -import { IMetric, MetricStatConfig } from './metric-types'; +import { IMetric, MetricExpressionConfig, MetricStatConfig } from './metric-types'; import { dispatchMetric, metricPeriod } from './private/metric-util'; import { dropUndefined } from './private/object'; import { MetricSet } from './private/rendering'; @@ -326,6 +326,7 @@ export class Alarm extends Resource implements IAlarm { expression: expr.expression, id: entry.id || uniqueMetricId(), label: conf.renderingProperties?.label, + period: mathExprHasSubmetrics(expr) ? undefined : expr.period, returnData: entry.tag ? undefined : false, // Tag stores "primary" attribute, default is "true" }; }, @@ -388,4 +389,8 @@ function renderIfExtendedStatistic(statistic?: string): string | undefined { return undefined; } +function mathExprHasSubmetrics(expr: MetricExpressionConfig) { + return Object.keys(expr.usingMetrics).length > 0; +} + type Writeable = { -readonly [P in keyof T]: T[P] }; diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts index 112e68b77be06..eaa48446672f9 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts @@ -315,6 +315,11 @@ export interface MetricExpressionConfig { * Metrics used in the math expression */ readonly usingMetrics: Record; + + /** + * How many seconds to aggregate over + */ + readonly period: number; } /** diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts index 040049be75adf..e554edf80cc8d 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts @@ -485,6 +485,7 @@ export class MathExpression implements IMetric { public toMetricConfig(): MetricConfig { return { mathExpression: { + period: this.period.toSeconds(), expression: this.expression, usingMetrics: this.usingMetrics, }, diff --git a/packages/@aws-cdk/aws-cloudwatch/package.json b/packages/@aws-cdk/aws-cloudwatch/package.json index e6a12ff672652..0e19dd9274dd3 100644 --- a/packages/@aws-cdk/aws-cloudwatch/package.json +++ b/packages/@aws-cdk/aws-cloudwatch/package.json @@ -98,7 +98,8 @@ "props-default-doc:@aws-cdk/aws-cloudwatch.MetricGraphConfig.unit", "props-default-doc:@aws-cdk/aws-cloudwatch.MetricRenderingProperties.color", "props-default-doc:@aws-cdk/aws-cloudwatch.MetricRenderingProperties.label", - "props-default-doc:@aws-cdk/aws-cloudwatch.MetricRenderingProperties.stat" + "props-default-doc:@aws-cdk/aws-cloudwatch.MetricRenderingProperties.stat", + "duration-prop-type:@aws-cdk/aws-cloudwatch.MetricExpressionConfig.period" ] }, "engines": { diff --git a/packages/@aws-cdk/aws-cloudwatch/test/test.metric-math.ts b/packages/@aws-cdk/aws-cloudwatch/test/test.metric-math.ts index e646c7d960001..64c2a378df5dc 100644 --- a/packages/@aws-cdk/aws-cloudwatch/test/test.metric-math.ts +++ b/packages/@aws-cdk/aws-cloudwatch/test/test.metric-math.ts @@ -507,6 +507,28 @@ export = { test.done(); }, + 'MathExpression without inner metrics emits its own period'(test: Test) { + // WHEN + new Alarm(stack, 'Alarm', { + threshold: 1, evaluationPeriods: 1, + metric: new MathExpression({ + expression: 'INSIGHT_RULE_METRIC("SomeId", UniqueContributors)', + usingMetrics: {}, + }), + }); + + // THEN + alarmMetricsAre([ + { + Expression: 'INSIGHT_RULE_METRIC("SomeId", UniqueContributors)', + Id: 'expr_1', + Period: 300, + }, + ]); + + test.done(); + }, + 'annotation for a mathexpression alarm is calculated based upon constituent metrics'(test: Test) { // GIVEN const alarm = new Alarm(stack, 'Alarm', { diff --git a/packages/@aws-cdk/cfnspec/spec-source/500_CloudWatch_Alarm_MetricDataQuery_Period_patch.json b/packages/@aws-cdk/cfnspec/spec-source/500_CloudWatch_Alarm_MetricDataQuery_Period_patch.json new file mode 100644 index 0000000000000..692f6dd4e869d --- /dev/null +++ b/packages/@aws-cdk/cfnspec/spec-source/500_CloudWatch_Alarm_MetricDataQuery_Period_patch.json @@ -0,0 +1,19 @@ +{ + "PropertyTypes": { + "AWS::CloudWatch::Alarm.MetricDataQuery": { + "patch": { + "description": "Add Period to AWS::CloudWatch::Alarm.MetricDataQuery (#7155)", + "operations": [ + { + "op": "add", + "path": "/Properties/Period", + "value": { + "PrimitiveType": "Integer", + "UpdateType": "Mutable" + } + } + ] + } + } + } +} \ No newline at end of file