feat(cloudwatch): support the creation of CloudWatch anomaly detection alarm #31232

scorbiere · 2024-08-27T23:49:02Z

Issue #10540

Reason for this change

This change add the possibility to create an anomaly detection alarm based on a Metric or MathExpression object.

Description of changes

Creation of a helper function in Metric or MathExpression classes which will wrap the actual metric into an anomaly detection metric expression before creating the CloudWatch alarm.

This PR also include a refactor of the CreateAlarmOptions class in order to reflect the incompatibilities between standard alarm and anomaly detection ones.

Description of how you validated changes

Executed the new integration test (packages/@aws-cdk-testing/framework-integ/test/aws-cloudwatch/test/integ.anomaly-detection-alarm.ts) on a test account.

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

packages/aws-cdk-lib/aws-cloudwatch/README.md

packages/aws-cdk-lib/aws-cloudwatch/lib/alarm.ts

packages/aws-cdk-lib/aws-cloudwatch/lib/metric.ts

xazhao · 2024-08-29T00:34:08Z

packages/aws-cdk-lib/aws-cloudwatch/lib/alarm.ts

+    if (isAnomalyDetection) {
+      // Ensure returnData is set to true for both metrics in anomaly detection
+      const cfnMetrics = alarm.metrics as any[];
+      if (cfnMetrics && cfnMetrics.length >= 2) {
+        cfnMetrics[0].returnData = true;
+        cfnMetrics[1].returnData = true;
+      }
+    }


Where is cfnMetrics used?

Only in this block. To make sure the returnData property is set to true. TBH, the documentation is not super clear about which one should have the property set to true.

Can you maybe provide the documentation in the comment? This code seems weird and providing the doc in the comment for future reference would help.

packages/aws-cdk-lib/rosetta/aws_cloudwatch/default.ts-fixture

aws-cdk-automation · 2024-08-29T19:39:23Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 8179b2d
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

packages/aws-cdk-lib/aws-cloudwatch/README.md

xazhao · 2024-08-29T20:57:51Z

packages/aws-cdk-lib/aws-cloudwatch/README.md

+To create an alarm based on anomaly detection, you can use the `createAnomalyDetectionAlarm` method on a `Metric` or `MathExpression` object:
+
+```ts
+declare const queue: sqs.Queue;


nit: customers may not get why using a queue here. Can we use the example in the integration test?

const metric = new Metric({ namespace: 'AWS/EC2', metricName: 'CPUUtilization', statistic: 'Average', period: Duration.minutes(5), }); metric.createAnomalyDetectionAlarm(this, 'AnomalyAlarm', { bounds: 2, evaluationPeriods: 3, datapointsToAlarm: 2, comparisonOperator: ComparisonOperator.LESS_THAN_LOWER_OR_GREATER_THAN_UPPER_THRESHOLD, });

xazhao · 2024-08-29T21:36:41Z

packages/aws-cdk-lib/aws-cloudwatch/lib/metric.ts

+      throw new Error(`Invalid comparison operator for anomaly detection alarm: ${props.comparisonOperator}`);
+    }
+    const anomalyDetectionExpression = new MathExpression({
+      expression: `ANOMALY_DETECTION_BAND(m0, ${props.bounds ?? 1})`,


since bounds is a required property, do we need null check here?

Right, this is useless now that the property is required.

Or should we have a default value for it?

xazhao · 2024-08-29T21:53:58Z

packages/aws-cdk-lib/aws-cloudwatch/lib/alarm.ts

@@ -190,7 +213,8 @@ export class Alarm extends AlarmBase {

      // Evaluation
      comparisonOperator,
-      threshold: props.threshold,
+      threshold: isAnomalyDetection ? undefined : props.threshold,


What will happen if it's anomaly detection and threshold value is passed in? Does it ignore the value or throw an error?

If the comparison operator is for an anomaly detection alarm, there will be an error when CloudFormation will try to execute the template. Because providing a threshold for anomaly detection is not valid.

Can we do this validation on CDK side to preemptively throw?

xazhao · 2024-08-29T23:25:51Z

General question: There's a CfnAnomalyDetector L1 construct which is not used here. Could you help me understand the diff between CfnAnomalyDetector and what we are achieving in this PR?

packages/aws-cdk-lib/aws-cloudwatch/lib/metric.ts

GavinZZ · 2024-08-30T23:19:22Z

packages/aws-cdk-lib/aws-cloudwatch/lib/alarm.ts

@@ -190,7 +213,8 @@ export class Alarm extends AlarmBase {

      // Evaluation
      comparisonOperator,
-      threshold: props.threshold,
+      threshold: isAnomalyDetection ? undefined : props.threshold,


Can we do this validation on CDK side to preemptively throw?

GavinZZ · 2024-08-30T23:19:57Z

packages/aws-cdk-lib/aws-cloudwatch/lib/alarm.ts

@@ -206,12 +230,22 @@ export class Alarm extends AlarmBase {
      ...metricProps,
    });

+    if (isAnomalyDetection) {
+      // Ensure returnData is set to true for both metrics in anomaly detection
+      const cfnMetrics = alarm.metrics as any[];


can we make it stronger type instead or any?

GavinZZ · 2024-08-30T23:21:00Z

packages/aws-cdk-lib/aws-cloudwatch/lib/alarm.ts

+    if (isAnomalyDetection) {
+      // Ensure returnData is set to true for both metrics in anomaly detection
+      const cfnMetrics = alarm.metrics as any[];
+      if (cfnMetrics && cfnMetrics.length >= 2) {
+        cfnMetrics[0].returnData = true;
+        cfnMetrics[1].returnData = true;
+      }
+    }


Can you maybe provide the documentation in the comment? This code seems weird and providing the doc in the comment for future reference would help.

rix0rrr

API-wise, it seems that to create an Anomaly detection alarm, 3 things need to happen:

You must set thresholdMetricId
You must wrap metric in a MetricMath that uses ANOMALY_DETECTION_BAND
You must not set a threshold
For an operator you must pick one of the special 3 comparison operators.

That feels like a lot of work, no?

rix0rrr · 2024-09-05T13:48:17Z

packages/aws-cdk-lib/aws-cloudwatch/lib/alarm.ts

+  /**
+   * In an alarm based on an anomaly detection model, this is the ID of the ANOMALY_DETECTION_BAND function used as the threshold for the alarm.
+   *
+   * @default - No metric id.
+   */
+  readonly thresholdMetricId?: string;


We don't really expose or rely on people specifying metric IDs anywhere else.

Why don't we expect them to pass in a MetricMath object here, which must of the kind ANOMALY_DETECTION_BAND, and we render it into the metrics[] array and render its id correctly to the underlying L1 property?

(Or in fact if not given we could create the ANOMALY_DETECTION_BAND() automatically with a sensible stdDev like in createAnomalyDetectionAlarm)

rix0rrr · 2024-09-05T13:49:34Z

packages/aws-cdk-lib/aws-cloudwatch/lib/alarm.ts

@@ -190,7 +213,8 @@ export class Alarm extends AlarmBase {

      // Evaluation
      comparisonOperator,
-      threshold: props.threshold,
+      threshold: isAnomalyDetection ? undefined : props.threshold,
+      thresholdMetricId: isAnomalyDetection ? (props.thresholdMetricId || 'expr_1') : undefined,


How likely is expr_1 to be correct?

rix0rrr · 2024-09-05T13:50:29Z

packages/aws-cdk-lib/aws-cloudwatch/lib/alarm.ts

+        cfnMetrics[0].returnData = true;
+        cfnMetrics[1].returnData = true;


What's the logic here, in that we have at least 2 metrics (but could be 5 or 10), and we switch the first 2 to returnData ? How do we know these are the right ones? Which ones are you trying to target?

rix0rrr · 2024-09-05T13:51:46Z

packages/aws-cdk-lib/aws-cloudwatch/lib/metric.ts

+    return new Alarm(scope, id, {
+      ...props,
+      metric: anomalyDetectionExpression,
+      threshold: 0, // This value will be ignored for anomaly detection alarms


Shouldn't we make threshold optional then, and do a runtime validation for its presence?

rix0rrr · 2024-09-05T13:53:16Z

packages/aws-cdk-lib/aws-cloudwatch/lib/metric.ts

+   * - The minimum value should be greater than 0. A value of 0 or negative values would not make sense in the context of calculating standard deviations.
+   * - There is no strict maximum value defined, as standard deviations can theoretically extend infinitely. However, in practice, values beyond 5 or 6 standard deviations are rarely used, as they would result in an extremely wide anomaly detection band, potentially missing significant anomalies.
+   */
+  readonly bounds: number;


Why don't we call this stdDevs then?

rix0rrr · 2024-09-05T13:53:43Z

packages/aws-cdk-lib/aws-cloudwatch/lib/metric.ts

+  /**
+   * Comparison to use to check if metric is breaching
+   */
+  readonly comparisonOperator: ComparisonOperator;


I think the "within bounds" operator would make a nice default here.

rix0rrr · 2024-09-05T13:54:53Z

packages/aws-cdk-lib/aws-cloudwatch/lib/metric.ts

+ * @param metric The metric to create the alarm for
+ * @returns The newly created Alarm
+ */
+  public static createAnomalyDetectionAlarmFromMetric(scope: Construct, id: string, props: CreateAnomalyDetectionAlarmProps, metric: IMetric): Alarm {


Keep in mind that this function is intended as a convenience function -- the main interface for users is still new Alarm(), and so that API should still be ergonomic.

Don't make design decisions that make it easy to implement this function, but harder to use the new Alarm() API.

rix0rrr

See previous review

scorbiere added 3 commits August 27, 2024 16:15

Support the creation of CloudWatch anomaly detection alarm

2b9571b

Cleanup comments and documentation.

850f892

Fix a property comment.

faf2819

aws-cdk-automation requested a review from a team August 27, 2024 23:49

github-actions bot added bug This issue is a bug. effort/medium Medium work item – several days of effort p1 labels Aug 27, 2024

mergify bot added the contribution/core This is a PR that came from AWS. label Aug 27, 2024

aws-cdk-automation previously requested changes Aug 27, 2024

View reviewed changes

scorbiere changed the title ~~fix(aws-cloudwatch): support the creation of CloudWatch anomaly detection alarm~~ fix(cloudwatch): support the creation of CloudWatch anomaly detection alarm Aug 27, 2024

scorbiere added 3 commits August 28, 2024 13:34

Merge branch 'main' into issue_10540

60c3a27

Trying to fix README example.

f5545d5

Fix README example

678903d

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Aug 28, 2024

xazhao reviewed Aug 29, 2024

View reviewed changes

refactor createAnomalyDetectionAlarm

a0fe2b0

xazhao reviewed Aug 29, 2024

View reviewed changes

packages/aws-cdk-lib/rosetta/aws_cloudwatch/default.ts-fixture Show resolved Hide resolved

Refactor isAnomalyDetectionOperator method

8179b2d

xazhao reviewed Aug 29, 2024

View reviewed changes

scorbiere changed the title ~~fix(cloudwatch): support the creation of CloudWatch anomaly detection alarm~~ feat(cloudwatch): support the creation of CloudWatch anomaly detection alarm Aug 30, 2024

xazhao reviewed Aug 30, 2024

View reviewed changes

packages/aws-cdk-lib/aws-cloudwatch/lib/metric.ts Show resolved Hide resolved

GavinZZ requested changes Aug 30, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Aug 30, 2024

This was referenced Sep 1, 2024

Monthly PR metrics report #31279

Closed

Weekly PR metrics report #31284

Closed

rix0rrr reviewed Sep 5, 2024

View reviewed changes

rix0rrr requested changes Sep 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cloudwatch): support the creation of CloudWatch anomaly detection alarm #31232

feat(cloudwatch): support the creation of CloudWatch anomaly detection alarm #31232

scorbiere commented Aug 27, 2024

aws-cdk-automation left a comment

xazhao Aug 29, 2024

scorbiere Aug 29, 2024

GavinZZ Aug 30, 2024

aws-cdk-automation commented Aug 29, 2024

xazhao Aug 29, 2024

xazhao Aug 29, 2024

scorbiere Aug 30, 2024

xazhao Aug 30, 2024

xazhao Aug 29, 2024

scorbiere Aug 30, 2024

GavinZZ Aug 30, 2024

xazhao commented Aug 29, 2024

GavinZZ Aug 30, 2024

GavinZZ Aug 30, 2024

GavinZZ Aug 30, 2024

rix0rrr left a comment

rix0rrr Sep 5, 2024

rix0rrr Sep 5, 2024

rix0rrr Sep 5, 2024

rix0rrr Sep 5, 2024

rix0rrr Sep 5, 2024

rix0rrr Sep 5, 2024

rix0rrr Sep 5, 2024

rix0rrr left a comment

		cfnMetrics[0].returnData = true;
		cfnMetrics[1].returnData = true;

feat(cloudwatch): support the creation of CloudWatch anomaly detection alarm #31232

Are you sure you want to change the base?

feat(cloudwatch): support the creation of CloudWatch anomaly detection alarm #31232

Conversation

scorbiere commented Aug 27, 2024

Issue #10540

Reason for this change

Description of changes

Description of how you validated changes

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Aug 29, 2024

AWS CodeBuild CI Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xazhao commented Aug 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr left a comment

Choose a reason for hiding this comment