Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: make anomaly detection alarm work on math expression #426

Merged
merged 1 commit into from
Sep 14, 2023
Merged

fix: make anomaly detection alarm work on math expression #426

merged 1 commit into from
Sep 14, 2023

Conversation

Laxenade
Copy link
Contributor

@Laxenade Laxenade commented Sep 14, 2023

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

Copy link
Member

@echeung-amzn echeung-amzn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@mergify mergify bot merged commit b07f361 into cdklabs:main Sep 14, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants