feat(cloudwatch): support anomaly detection alarms

[brandondahler]

• Allows customers to create Alarms with anomaly detection enabled.
• Supports both direct metrics and math expressions.

Closes #10540

Implementation Notes

• TreatMissingData has to be moved from alarm.ts to metric.ts as it not longer is specific to only Alarm.
• Deprecated anomaly-based comparison operators from Alarm's ComparisonOperator enum.
  • Created a new AnomalyDetectionComparisonOperator enum to contain the anomaly-based comparison operators.
• AnomalyDetectionAlarm is mostly copied from Alarm, main change required was to update how renderMetric works.
  • Removed horizontalAnnotation behavior as I don't believe there's a direct equivalent for anomaly detection at the moment.
  • The copying did duplicate some utility functions, wasn't sure if it was worth moving into the private/metric-util file or not.

---

All Submissions:

• Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

• This PR adds new unconventional dependencies following the process described here

New Features

• Have you added the new feature to an integration test?
  • Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

[gitpod-io]

[kaizencc]

Hi @brandondahler! Thanks for picking this up. The README looks excellent, and I have a few preliminary comments regarding the implementation.

[kaizencc]

question: how much of this is copied directly from Alarm.renderMetric and how much is original?

[brandondahler]

It originally was copied directly from Alarm.renderMetric.

I then:

1. Added code to assign/generate an id for the anomaly band expression
2. Added code to resolve the target metric's id, generating it earlier than originally if it isn't assigned in the case of a MathExpression
3. Updated the returns of the callbacks to include the thresholdMetricId value that was assigned/generated in the output.
4. Updated the returns of the callbacks for dispatchMetric to include the extra metric:

            {
              expression: `ANOMALY_DETECTION_BAND(${metricId}, ${threshold})`,
              id: thresholdMetricId,
              label: 'Expected',
              returnData: true,
            },
   

[brandondahler]

I'll work through those suggestions soon.

FYSA - In general most of this code was copied from Alarm, so the naming and documentation comments are likely also applicable there. I'm fine with making them for this new construct but that will make this construct diverge from how the normal Alarm construct is named / documented.

[brandondahler]

Review comments addressed, rebased on latest master branch.

Pull request has been modified.

[brandondahler]

Looks like its just under the wire on branch coverage, I'll review what branch coverage I'm missing soon.

[brandondahler]

Improved unit test coverage.

[brandondahler]

Cleaned up some nits around documentation that I found after self-review.

[pflorek]

How is the usage of the AnomalyDetectionAlarm?

const metric = new Metric(...);
const anomalyDetector = new CfnAnomalyDetector(this, "AnomalyDetector", {
      metricName: invocationsMetric.metricName,
      namespace: invocationsMetric.namespace,
      stat: "Sum",
    });
const alarm = new AnomalyDetectionAlarm(this, "Alarm", {
    metric: metric,
    ...
});

An idea, does it make sense to construct CfnAnomalyDetector inside AnomalyDetectionAlarm for the given metric?

[brandondahler]

My understanding from the documentation and testing is that CfnAnomalyDetector isn’t necessary for alarm usage.

That simplifies usage down to the same as a normal alarm.

const alarm = new AnomalyDetectionAlarm(this, "Alarm", {
    metric: metric,
    ...
});

The only significant difference is that threshold represents the number of standard deviations the band should use and the comparison operator determines whether it should use the upper, lower, or both thresholds.

[brandondahler]

An idea, does it make sense to construct CfnAnomalyDetector inside AnomalyDetectionAlarm for the given metric?

I've looked a lot closer on how the documentation says things work around anomaly detection alarms and how that interacts with CfnAnomalyDetector (i.e. PutAnomalyDetector/DeleteAnomalyDetector).

I believe that effectively how CloudWatch with an anomaly detection alarm works is that each time it tries to evaluate the upper and lower bound, if the anomaly detector doesn't already exist (or was deleted) it'll call PutAnomalyDetector with the minimal information (i.e. the appropriate metric info, no extra configuration). Because of the auto-creation the usage of CfnAnomalyDetector isn't technically required.

Since the creation isn't technically required, the question becomes "Should we create/delete it because it makes things easier for customers in some manner?"

The documentation makes reference to deleting unused anomaly detectors due to them having a recurring cost; however, the pricing page makes no mention of ongoing cost of anomaly detector resources existing (only the +2 metric cost associated with running an alarm). I believe the documentation that references that cost is just out of date. Due to the lack of extra cost, I don't think it is necessary to manage creating and deleting the anomaly detectors that are otherwise auto-created by CloudWatch.

The only other consideration I found was that explicitly creating the anomaly detector allowed customers to exclude anomalous time ranges and configure the detector for a specific timezone (for DST purposes). The ability to configure the anomaly detector is an important feature that helps ensure that the anomaly detector doesn't train on bad data; however, I don't think configuring it within the AnomalyDetectionAlarm makes sense. My reasoning is that the AnomalyDetectionAlarm isn't explicitly tied to any anomaly detector instance -- the anomaly detector instance is a singleton per-metric/statistic. If we were to create/manage the anomaly detector instance within the AnomalyDetectionAlarm, separate alarms that both target the same metric+stat (e.g. with different standard deviation thresholds) may either fight between each other on which one's settings are the effective settings used or even worse the removal of one would silently wipe out the configuration from the other.

TL;DR - I don't think that AnomalyDetectionAlarm is the right place to manage CfnAnomalyDetector instances and instead we should separately make an AnomalyDetector L2 construct in a different PR (to allow customers to configure the detector as they see fit).

[TheRealAmazonKendra]

Looks like our branch change to v2 caused a branch conflict. Can you please take a look at getting that resolved?

[brandondahler]

Copy, I'll get it fixed when I get some time -- hopefully in the next week or two.

[brandondahler]

Rebased onto latest main

Pull request has been modified.

[TheRealAmazonKendra]

Thank you for your contribution! This is just an initial passthrough, I didn't get too into the weeds at this point. Please address the inline comments and then I'll provide a more thorough review.

Also, thank you for the extensive unit testing here!

[TheRealAmazonKendra]

I don't see a reason to do this. Can you explain why you want to deprecate these when they're used more widely than just in this context?

[TheRealAmazonKendra]

I don't think this move makes sense. This is directly related to creating an alarm.

[TheRealAmazonKendra]

It looks like a lot of this is duplicating what's in the Alarm class. We should abstract the duplications out of both into a class that these can extend.

[TheRealAmazonKendra]

@Mergifyio update

[mergify]

update

✅ Branch has been successfully updated

[TheRealAmazonKendra]

If/when you rebase without a code change, please use the Mergify commands instead of merging in other ways. It will keep my review active so I'll be able to more easily see when you've made updates to PR.

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: 9f08889
• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[TheRealAmazonKendra]

Our autoclose/autowarn action doesn't seem to be picking this PR up. It would normally warn after 21 days and then close after 28. Since this has been dormant this long, I'm going to go ahead and close this. If you would like to continue working on it, we certainly welcome that. Please open a new PR addressing the last round of feed back and we'll re-review.

[Lykr]

Hi, is there any update to this issue? Do we plan to provide user a more convenient way to create anomaly detection alarms?

[IhebMarnaoui]

Do we already have some clear way on how to create anomaly detection alarms now or has this never been picked up ?