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

add ignoreNullValues for AWS CloudWatch Scaler #5635

Merged

Conversation

robpickerill
Copy link
Contributor

@robpickerill robpickerill commented Mar 28, 2024

This PR adds support for erroring the AWS Cloudwatch Scaler when the GetMetricData call returns an empty set of metrics, resulting in no scale-changing behaviour when no custom metrics are available during the Cloudwatch query window.

Checklist

Fixes #5352

Relates to:

@robpickerill robpickerill requested a review from a team as a code owner March 28, 2024 15:03
Signed-off-by: Rob Pickerill <[email protected]>
@robpickerill robpickerill force-pushed the cloudwatch-error-when-empty-metrics branch from 13c0fec to 53ea3fd Compare March 28, 2024 18:07
@robpickerill robpickerill force-pushed the cloudwatch-error-when-empty-metrics branch 3 times, most recently from 495c201 to 8e41311 Compare March 29, 2024 18:38
@robpickerill robpickerill force-pushed the cloudwatch-error-when-empty-metrics branch from 584eba6 to b0d4e84 Compare March 29, 2024 19:01
@robpickerill robpickerill changed the title add errorWhenMetricValueEmpty add errorWhenNullValues Mar 31, 2024
@robpickerill robpickerill changed the title add errorWhenNullValues add errorWhenNullValues for AWS CloudWatch Scaler Apr 1, 2024
@robpickerill
Copy link
Contributor Author

I added end to end tests here for both minMetricValue and errorWhenNullValues to cover off both scenarios. I need to tidy them up a little, which ill do over the next few days.

@robpickerill robpickerill force-pushed the cloudwatch-error-when-empty-metrics branch 2 times, most recently from 53fac56 to 0ef98c5 Compare April 4, 2024 17:26
@robpickerill robpickerill force-pushed the cloudwatch-error-when-empty-metrics branch from 0ef98c5 to 0b61f97 Compare April 4, 2024 17:28
Signed-off-by: Rob Pickerill <[email protected]>
@robpickerill robpickerill force-pushed the cloudwatch-error-when-empty-metrics branch from 68a1c71 to 0dc9b77 Compare April 4, 2024 17:44
@robpickerill robpickerill changed the title add errorWhenNullValues for AWS CloudWatch Scaler add ignoreNullValues for AWS CloudWatch Scaler Apr 11, 2024
@robpickerill
Copy link
Contributor Author

I was discussing with Jorge on slack to get the e2e tests re-ran, as the failures were upstream issues: https://kubernetes.slack.com/archives/C01JGDP8MB8/p1717129850310929?thread_ts=1715943304.582119&cid=C01JGDP8MB8

However, since then the changes for the validation have gone in, so let me fix up the conflicts here over the weekend, and then let's get e2e passing again on this branch

@zroubalik
Copy link
Member

Thanks!

Signed-off-by: robpickerill <[email protected]>
@robpickerill robpickerill force-pushed the cloudwatch-error-when-empty-metrics branch from 731d45c to 2e76a9b Compare June 8, 2024 15:37
@robpickerill
Copy link
Contributor Author

Fixed conflicts, this needs another e2e test run

@zroubalik
Copy link
Member

zroubalik commented Jun 10, 2024

/run-e2e aws
Update: You can check the progress here

Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

@robpickerill there is also a problem in static checks

@robpickerill
Copy link
Contributor Author

robpickerill commented Jun 10, 2024

@robpickerill there is also a problem in static checks

Fixed, all tests should pass now.


I see e2e didn't run due to Azure cluster scaling actions failing, would it be easier for me to put the e2e tests in a different PR and unblock this PR? The unit tests here cover the error case for the scaler, but I wanted to add e2e to prove the backward compatibility for the change.

Let me know what works best for you.

Signed-off-by: robpickerill <[email protected]>
@robpickerill robpickerill force-pushed the cloudwatch-error-when-empty-metrics branch from 48ccb91 to 43a2c3f Compare June 10, 2024 09:06
@zroubalik
Copy link
Member

zroubalik commented Jun 10, 2024

/run-e2e aws
Update: You can check the progress here

Copy link

stale bot commented Aug 9, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale All issues that are marked as stale due to inactivity label Aug 9, 2024
@JorTurFer
Copy link
Member

JorTurFer commented Aug 12, 2024

/run-e2e aws
Update: You can check the progress here

@robpickerill
Copy link
Contributor Author

Thanks Jorge, need anything else to merge this?

@stale stale bot removed the stale All issues that are marked as stale due to inactivity label Aug 12, 2024
Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

LGTM! ❤️
Please address the docs comment and we are ready

@robpickerill
Copy link
Contributor Author

Thanks Jorge, fixed up the docs, and merged main into here and docs, should be good to go now - thanks again!

@JorTurFer
Copy link
Member

JorTurFer commented Aug 20, 2024

/run-e2e aws
Update: You can check the progress here

@JorTurFer JorTurFer merged commit 0771b35 into kedacore:main Aug 20, 2024
26 checks passed
@robpickerill robpickerill deleted the cloudwatch-error-when-empty-metrics branch August 21, 2024 06:15
fira42073 pushed a commit to fira42073/keda that referenced this pull request Aug 25, 2024
* add errorWhenMetricValueEmpty

Signed-off-by: Rob Pickerill <[email protected]>

* fix golangci-lint

Signed-off-by: Rob Pickerill <[email protected]>

* improve error message for empty results

Signed-off-by: Rob Pickerill <[email protected]>

* add error when empty metric values to changelog

Signed-off-by: Rob Pickerill <[email protected]>

* rename errorWhenMetricValuesEmpty -> errorWhenNullValues

Signed-off-by: Rob Pickerill <[email protected]>

* use getParameterFromConfigV2 to read config for errorWhenNullValues

Signed-off-by: Rob Pickerill <[email protected]>

* add e2e for error state for cw, and improve e2e for min values for cw

Signed-off-by: Rob Pickerill <[email protected]>

* remove erroneous print statement

Signed-off-by: Rob Pickerill <[email protected]>

* remove unused vars

Signed-off-by: Rob Pickerill <[email protected]>

* rename errorWhenMetricValuesEmpty -> ignoreNullValues

Signed-off-by: Rob Pickerill <[email protected]>

* move towards shared package for e2e aws

Signed-off-by: Rob Pickerill <[email protected]>

* minMetricValue optionality based on ignoreNullValues

Signed-off-by: Rob Pickerill <[email protected]>

* fail fast

Co-authored-by: Jorge Turrado Ferrero <[email protected]>
Signed-off-by: Rob Pickerill <[email protected]>

* Update tests/scalers/aws/aws_cloudwatch_ignore_null_values_false/aws_cloudwatch_ignore_null_values_false_test.go

Co-authored-by: Jorge Turrado Ferrero <[email protected]>
Signed-off-by: Rob Pickerill <[email protected]>

* Apply suggestions from code review

Co-authored-by: Jorge Turrado Ferrero <[email protected]>
Signed-off-by: Rob Pickerill <[email protected]>

* fail fast

Signed-off-by: Rob Pickerill <[email protected]>

* fix broken new line

Signed-off-by: Rob Pickerill <[email protected]>

* fix broken new lines

Signed-off-by: Rob Pickerill <[email protected]>

* assert no scaling changes in e2e, and set false for required in minMetricValue

Signed-off-by: robpickerill <[email protected]>

* fix ci checks

Signed-off-by: robpickerill <[email protected]>

* Update tests/scalers/aws/aws_cloudwatch_min_metric_value/aws_cloudwatch_min_metric_value_test.go

fix invalid check

Co-authored-by: Jorge Turrado Ferrero <[email protected]>
Signed-off-by: Rob Pickerill <[email protected]>

* fix merge conflicts

Signed-off-by: robpickerill <[email protected]>

* fix e2e package names

Signed-off-by: robpickerill <[email protected]>

---------

Signed-off-by: Rob Pickerill <[email protected]>
Signed-off-by: robpickerill <[email protected]>
Co-authored-by: Jorge Turrado Ferrero <[email protected]>
Signed-off-by: Fira Curie <[email protected]>
JorTurFer added a commit to JorTurFer/keda that referenced this pull request Oct 7, 2024
* add errorWhenMetricValueEmpty

Signed-off-by: Rob Pickerill <[email protected]>

* fix golangci-lint

Signed-off-by: Rob Pickerill <[email protected]>

* improve error message for empty results

Signed-off-by: Rob Pickerill <[email protected]>

* add error when empty metric values to changelog

Signed-off-by: Rob Pickerill <[email protected]>

* rename errorWhenMetricValuesEmpty -> errorWhenNullValues

Signed-off-by: Rob Pickerill <[email protected]>

* use getParameterFromConfigV2 to read config for errorWhenNullValues

Signed-off-by: Rob Pickerill <[email protected]>

* add e2e for error state for cw, and improve e2e for min values for cw

Signed-off-by: Rob Pickerill <[email protected]>

* remove erroneous print statement

Signed-off-by: Rob Pickerill <[email protected]>

* remove unused vars

Signed-off-by: Rob Pickerill <[email protected]>

* rename errorWhenMetricValuesEmpty -> ignoreNullValues

Signed-off-by: Rob Pickerill <[email protected]>

* move towards shared package for e2e aws

Signed-off-by: Rob Pickerill <[email protected]>

* minMetricValue optionality based on ignoreNullValues

Signed-off-by: Rob Pickerill <[email protected]>

* fail fast

Co-authored-by: Jorge Turrado Ferrero <[email protected]>
Signed-off-by: Rob Pickerill <[email protected]>

* Update tests/scalers/aws/aws_cloudwatch_ignore_null_values_false/aws_cloudwatch_ignore_null_values_false_test.go

Co-authored-by: Jorge Turrado Ferrero <[email protected]>
Signed-off-by: Rob Pickerill <[email protected]>

* Apply suggestions from code review

Co-authored-by: Jorge Turrado Ferrero <[email protected]>
Signed-off-by: Rob Pickerill <[email protected]>

* fail fast

Signed-off-by: Rob Pickerill <[email protected]>

* fix broken new line

Signed-off-by: Rob Pickerill <[email protected]>

* fix broken new lines

Signed-off-by: Rob Pickerill <[email protected]>

* assert no scaling changes in e2e, and set false for required in minMetricValue

Signed-off-by: robpickerill <[email protected]>

* fix ci checks

Signed-off-by: robpickerill <[email protected]>

* Update tests/scalers/aws/aws_cloudwatch_min_metric_value/aws_cloudwatch_min_metric_value_test.go

fix invalid check

Co-authored-by: Jorge Turrado Ferrero <[email protected]>
Signed-off-by: Rob Pickerill <[email protected]>

* fix merge conflicts

Signed-off-by: robpickerill <[email protected]>

* fix e2e package names

Signed-off-by: robpickerill <[email protected]>

---------

Signed-off-by: Rob Pickerill <[email protected]>
Signed-off-by: robpickerill <[email protected]>
Co-authored-by: Jorge Turrado Ferrero <[email protected]>
Signed-off-by: Jorge Turrado <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AWS Cloudwatch Scaler, do nothing on empty metric data received
3 participants