Skip to content

Commit c7d8004

Browse files
fix(cloudwatch): metric period in AnomalyDetectionAlarm is not being respected (#35319)
Fixes issue where anomaly detection band alarm's period defaulted to 300 seconds regardless of the metric's period. AnomalyDetectionAlarmProps didn't have a period property because it was deprecated in the parent interface and removed by jsii during compilation, causing the internal MathExpression to default to 300 seconds. I am an Amazon employee (see commit email). ### Issue Should fix #34614 (the issue was closed as duplicate but I'm not sure [the issue it was linked to](#32221) is actually the same issue) ### Reason for this change Although I passed in the duration of 1 day as the `period` for the alarm metric, the actual period for the evaluation of the anomaly detection band is overridden to 5 minutes (300 seconds). i.e, `cdk synth` shows this warning: > [Warning at /TestCdkStack/TestAnomalyAlarm] Periods of metrics in 'usingMetrics' for Math expression 'ANOMALY_DETECTION_BAND(m0, 2)' have been overridden to 300 seconds. [ack: CloudWatch:Math:MetricsPeriodsOverridden] And, on deployment, I can see on the CloudWatch alarm dashboard that the period is 5 minutes. Example alarm: ```typescript import * as cdk from 'aws-cdk-lib'; import { Construct } from 'constructs'; import { AnomalyDetectionAlarm, ComparisonOperator, Metric, Stats } from 'aws-cdk-lib/aws-cloudwatch'; export class TestCdkStack extends cdk.Stack { constructor(scope: Construct, id: string, props?: cdk.StackProps) { super(scope, id, props); new AnomalyDetectionAlarm(this, 'TestAnomalyAlarm', { alarmName: 'TestAnomalyDetectionAlarm', metric: new Metric({ namespace: 'TestNamespace', metricName: 'TestMetric', statistic: Stats.SUM, period: cdk.Duration.days(1), // This will get overriden }), // period: cdk.Duration.days(1), I can't add period here since AnomalyDetectionAlarmProps doesn't have period prop stdDevs: 2, comparisonOperator: ComparisonOperator.GREATER_THAN_UPPER_THRESHOLD, evaluationPeriods: 1, }); } } ``` This happens because `AnomalyDetectionAlarm` creates an internal `MathExpression` for the anomaly detection band, and since `AnomalyDetectionAlarmProps` doesn't have a period property, no period gets passed to that math expression. The math expression then defaults to 300 seconds which overrides the period I've set on the metric used within it (math expression always [overrides](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_cloudwatch.MathExpressionProps.html#period) the periods of the metrics passed into it) I believe this is a bug in aws-cdk-lib where the metric's period isn't being respected by the Anomaly Detection Alarm. If I try to include the `period` property in `AnomalyDetectionAlarmProps`, I get the error: `Object literal may only specify known properties, and 'period' does not exist in type 'AnomalyDetectionAlarmProps'` despite `AnomalyDetectionAlarmProps` extending `CreateAlarmOptionsBase`, which has the `period` property. This is because the `period` property has been marked as [deprecated](https://github.com/aws/aws-cdk/blob/6966c03b1a7aece0846f5a91bbeb825cd7491689/packages/aws-cdk-lib/aws-cloudwatch/lib/private/alarm-options.ts#L16-L18) so [`jsii` removed that property](https://github.com/aws/aws-cdk/blob/86638f6daca6ead382d0b9c1cf65bb04f70d4b3d/packages/aws-cdk-lib/package.json#L32) during compilation, which resulted in `AnomalyDetectionAlarmProps` not receiving this property from the parent interface. ### Possible Fixes 1. Add a `period` property to `AnomalyDetectionAlarmProps` 2. Have the AnomalyDetectionAlarm use the period set in the `metric` passed into it This PR implements approach 2 (see [discussion](#35319 (comment))) ### Describe any new or updated permissions being added N/A ### Description of how you validated changes Added two new unit tests Added an integration test #### Steps I took for testing (listing it here to catch if I missed anything/did something wrong) - I made a `testCDK` package with the example alarm I've provided above. - Made the change to `aws-cdk-lib` and ran `npx lerna run build --scope=aws-cdk-lib` - Ran `../aws/link-all.sh` in `testCDK` directory. - Also had to follow the second option mentioned [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md#import-errors) since I faced the same issue. - Ran `npx cdk synth` to make sure the generated file is correct. - Also deployed to my aws account with `npx cdk deploy` and verified that the dashboard is displaying the correct duration. - Ran `yarn test` in `aws-cdk/packages/aws-cdk-lib` - Ran `npx lerna run build --scope=@aws-cdk-testing/framework-integ` followed by `yarn integ --update-on-failed` ### 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*
1 parent 46fb2dc commit c7d8004

File tree

13 files changed

+960
-227
lines changed

13 files changed

+960
-227
lines changed

packages/@aws-cdk-testing/framework-integ/test/aws-cloudwatch/test/integ.anomaly-detection-alarm.js.snapshot/AnomalyDetectionAlarmIntegTestDefaultTestDeployAssert033A4433.assets.json

Lines changed: 8 additions & 8 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

packages/@aws-cdk-testing/framework-integ/test/aws-cloudwatch/test/integ.anomaly-detection-alarm.js.snapshot/AnomalyDetectionAlarmIntegTestDefaultTestDeployAssert033A4433.template.json

Lines changed: 53 additions & 136 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

packages/@aws-cdk-testing/framework-integ/test/aws-cloudwatch/test/integ.anomaly-detection-alarm.js.snapshot/AnomalyDetectionAlarmTestStack.assets.json

Lines changed: 4 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

packages/@aws-cdk-testing/framework-integ/test/aws-cloudwatch/test/integ.anomaly-detection-alarm.js.snapshot/AnomalyDetectionAlarmTestStack.template.json

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,34 @@
8585
],
8686
"ThresholdMetricId": "expr_1"
8787
}
88+
},
89+
"CustomPeriodAnomalyAlarmD85345B9": {
90+
"Type": "AWS::CloudWatch::Alarm",
91+
"Properties": {
92+
"ComparisonOperator": "LessThanLowerOrGreaterThanUpperThreshold",
93+
"EvaluationPeriods": 1,
94+
"Metrics": [
95+
{
96+
"Expression": "ANOMALY_DETECTION_BAND(m0, 2)",
97+
"Id": "expr_1",
98+
"Label": "Anomaly Detection Band",
99+
"ReturnData": true
100+
},
101+
{
102+
"Id": "m0",
103+
"MetricStat": {
104+
"Metric": {
105+
"MetricName": "CPUUtilization",
106+
"Namespace": "AWS/EC2"
107+
},
108+
"Period": 86400,
109+
"Stat": "Average"
110+
},
111+
"ReturnData": true
112+
}
113+
],
114+
"ThresholdMetricId": "expr_1"
115+
}
88116
}
89117
},
90118
"Outputs": {
@@ -111,6 +139,14 @@
111139
"Export": {
112140
"Name": "AnomalyDetectionAlarmTestStack:ExportsOutputRefDescriptiveAnomalyAlarm8B14203E100E41DF"
113141
}
142+
},
143+
"ExportsOutputRefCustomPeriodAnomalyAlarmD85345B903D51401": {
144+
"Value": {
145+
"Ref": "CustomPeriodAnomalyAlarmD85345B9"
146+
},
147+
"Export": {
148+
"Name": "AnomalyDetectionAlarmTestStack:ExportsOutputRefCustomPeriodAnomalyAlarmD85345B903D51401"
149+
}
114150
}
115151
},
116152
"Parameters": {

0 commit comments

Comments
 (0)