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.

Sign up for GitHub

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

Jump to bottom

Added alternative expression field in AWS CloudWatch scaler #2997

Merged
merged 4 commits into from
May 4, 2022
Merged

Added alternative expression field in AWS CloudWatch scaler #2997

merged 4 commits into from
May 4, 2022

Conversation

dekelev
Copy link
Contributor

@dekelev dekelev commented May 4, 2022 •
edited by JorTurFer
Loading

Added alternative expression field in AWS CloudWatch scaler.
Here's an example ScaledObject config:

awsRegion: us-east-1
expression: >-
  SELECT MIN(MessageCount) FROM "AWS/AmazonMQ" WHERE Broker =
  'production' and Queue = 'worker'
metricCollectionTime: '60'
metricName: WorkerMessageCount
metricStatPeriod: '60'
minMetricValue: '0'
namespace: AWS/AmazonMQ
targetMetricValue: '100'

In addition, AWS CloudWatch scaler was changed to use int64 instead of float64 for metric type, since HPA doesn't work with float numbers, integers only.

Checklist

In addition to PR #2911
Fixes #2998

@dekelev
Added alternative expression field in AWS CloudWatch scaler
0e25370 
Signed-off-by: Dekel Barzilay <dekelev@gmail.com>
@dekelev dekelev requested a review from a team as a code owner May 4, 2022 11:19
@dekelev dekelev mentioned this pull request May 4, 2022
Closed
5 tasks
@JorTurFer
Copy link
Member

JorTurFer commented May 4, 2022 •
edited by github-actions bot
Loading

/run-e2e aws-cloudwatch*
Update: You can check the progres here

@dekelev dekelev mentioned this pull request May 4, 2022
Merged
@dekelev
Copy link
Contributor Author

dekelev commented May 4, 2022 •
edited
Loading

@JorTurFer Docs changes PR in v2.7:
kedacore/keda-docs#755

https://deploy-preview-755--keda.netlify.app/docs/2.7/scalers/aws-cloudwatch/

@dekelev
Copy link
Contributor Author

dekelev commented May 4, 2022 •
edited
Loading

/run-e2e aws-cloudwatch* Update: You can check the progres here

@JorTurFer Looks like it's failing on something unrelated to this PR, right? Maybe it's not working because the PR is out of sync with the main branch? It was forked about 2 weeks ago.

JorTurFer
JorTurFer reviewed May 4, 2022
View reviewed changes
Copy link
Member

@JorTurFer JorTurFer left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM!
Thanks for the contribution! ❤️
Only 2 things thing, could you update the changelog with the changes?
I have created an issue for tracking this,
so the line that you have to add is something like this:

  • AWS CloudWatch: Add support to use expressions(#2998)

(Under the ### Improvements section)

Please, rebase also the latest changes into your branch, there are some changes needed for running e2e tests

@dekelev
Copy link
Contributor Author

dekelev commented May 4, 2022

LGTM! Thanks for the contribution! ❤️ Only 2 things thing, could you update the changelog with the changes? I have created an issue for tracking this, so the line that you have to add is something like this:

  • AWS CloudWatch: Add support to use expressions(#2998)

(Under the ### Improvements section)

Please, rebase also the latest changes into your branch, there are some changes needed for running e2e tests

Done

JorTurFer
JorTurFer reviewed May 4, 2022
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@JorTurFer
Copy link
Member

JorTurFer commented May 4, 2022 •
edited by github-actions bot
Loading

/run-e2e aws-cloudwatch*
Update: You can check the progres here

@dekelev
Updated CHANGELOG with AWS CloudWatch expression improvement
a69f032 
Signed-off-by: Dekel Barzilay <dekelev@gmail.com>
zroubalik
zroubalik reviewed May 4, 2022
View reviewed changes
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.

Generally looking good! Great job. Just a comment on metric name.

pkg/scalers/aws_cloudwatch_scaler.go Show resolved Hide resolved
zroubalik
zroubalik approved these changes May 4, 2022
View reviewed changes
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.

LGTM

@zroubalik
Copy link
Member

zroubalik commented May 4, 2022 •
edited by github-actions bot
Loading

/run-e2e aws-cloudwatch*
Update: You can check the progres here

JorTurFer
JorTurFer approved these changes May 4, 2022 •
edited
Loading
View reviewed changes
JorTurFer
JorTurFer approved these changes May 4, 2022
View reviewed changes
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!
❤️ ❤️ ❤️ ❤️

@zroubalik zroubalik merged commit 1d6aedb into kedacore:main May 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zroubalik zroubalik zroubalik approved these changes

@JorTurFer JorTurFer JorTurFer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support expressions in AWS CloudWatch Scaler
3 participants
@dekelev @JorTurFer @zroubalik