Skip to content

Commit

Permalink
fix: make anomaly detection alarm work on math expression (#426)
Browse files Browse the repository at this point in the history
Fixes #425 
Fixes #340

### Changes

Changes to the implementation of `AnomalyDetectionMathExpression`:
* Fixed an issue where `thresholdMetricId` was assigned the incorrect expression id when multiple math expressions were present in the generated CFN template.
* Modified `returnData` to be true only for the `ANOMALY_DETECTION_BAND` function and its direct dependency, rather than for all of the metrics in `Metrics`.

I haven't come across any internal or external documentation indicating that two `returnData: true` are necessary for anomaly detection. However, after doing some experiments on CFN, this appears to be true.

### Testing

I copied the fix into my own CDK package, with the fix, I was able to deploy my stacks.

---

_By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license_
  • Loading branch information
Laxenade authored Sep 14, 2023
1 parent 465e388 commit b07f361
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 5 deletions.
18 changes: 16 additions & 2 deletions lib/common/metric/AnomalyDetectionMathExpression.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,29 @@ export class AnomalyDetectionMathExpression extends MathExpression {
createAlarm(scope: Construct, id: string, props: CreateAlarmOptions): Alarm {
const alarm = super.createAlarm(scope, id, props);

// `usingMetrics` of an anomaly detection alarm can only ever have one entry.
// Should the entry be a math expression, the math expression can have its own `usingMetrics`.
const finalExpressionId = Object.keys(this.usingMetrics)[0];

// https://github.com/aws/aws-cdk/issues/10540#issuecomment-725222564
const cfnAlarm = alarm.node.defaultChild as CfnAlarm;
cfnAlarm.addPropertyDeletionOverride("Threshold");
(cfnAlarm.metrics as CfnAlarm.MetricDataQueryProperty[]).forEach(
(metric, index) => {
if (metric.expression) {
// To create an anomaly detection alarm, returned data should be set to true on two MetricDataQueryProperty(s):
// 1. The metric or math expression that is being evaluated for anomaly detection (eg. expr_1)
// 2. The actual expression of anomaly detection (eg. ANOMALY_DETECTION_BAND(expr_1, 1))
let returnData = false;

if (metric.expression?.includes("ANOMALY_DETECTION_BAND")) {
// thresholdMetricId is the ID of the ANOMALY_DETECTION_BAND function used as the threshold for the alarm.
cfnAlarm.thresholdMetricId = metric.id;
returnData = true;
} else if (metric.id === finalExpressionId) {
returnData = true;
}
cfnAlarm.addPropertyOverride(`Metrics.${index}.ReturnData`, true);

cfnAlarm.addPropertyOverride(`Metrics.${index}.ReturnData`, returnData);
}
);

Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit b07f361

Please sign in to comment.