Activation threshold (activationValue) for Rabbitmq #2831

adborroto · 2022-03-27T11:06:26Z

Signed-off-by: Alejandro Dominguez adborroto90@gmail.com

Adding activation threshold (activationValue) for Rabbitmq. The current activation when scaling from 0 to 1 happens if there is at least 1 message messageCount > 0. If a value for activationValue then the activation occurs if messageCount > activationValue.
The default value for activationValue is 0

Documentation: PR

Checklist

Commits are signed with Developer Certificate of Origin (DCO - learn more)
Tests have been added
A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified)
A PR is opened to update the documentation on (repo) (if applicable)
Changelog has been updated and is aligned with our changelog requirements

Fixes #

Signed-off-by: Alejandro Dominguez <adborroto90@gmail.com>

JorTurFer

Thanks for your contribution!!
I have left some comments inline.
Apart from them, I'm not totally sure about minMetricValue as a key. I mean, I'd like to have something like activationThreshold, just to be clearer as possible. There are another scalers which use minMetricValue for setting the return value in case of empty response from the upstream.
IMO, we can use this change to unify this value (activationThreshold or similar) in all the scalers, deprecating the current value if the scaler already has another option
WDYT @kedacore/keda-core-contributors ?

Please, also rebase main into your branch 🙏

pkg/scalers/rabbitmq_scaler.go

tests/scalers/rabbitmq-queue-amqp.test.ts

zroubalik · 2022-03-28T08:24:18Z

Thanks for your contribution!! I have left some comments inline. Apart from them, I'm not totally sure about minMetricValue as a key. I mean, I'd like to have something like activationThreshold, just to be clearer as possible. There are another scalers which use minMetricValue for setting the return value in case of empty response from the upstream. IMO, we can use this change to unify this value (activationThreshold or similar) in all the scalers, deprecating the current value if the scaler already has another option WDYT @kedacore/keda-core-contributors ?

+1 exactly my thoughts. We should come up with a proper name first, maybe activation* prefix to the current field that holds the value/queuelength/threshold in each scaler?

@kedacore/keda-core-contributors @adborroto Let's discuss this here: #2800

Signed-off-by: Alejandro Dominguez <adborroto90@gmail.com>

JorTurFer

LGTM
only one inline comment

pkg/scalers/rabbitmq_scaler.go

JorTurFer · 2022-03-30T10:30:19Z

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

Signed-off-by: Alejandro Dominguez <adborroto90@gmail.com>

JorTurFer · 2022-03-30T14:57:38Z

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

JorTurFer · 2022-03-30T18:42:55Z

@adborroto , looks like the added e2e test if failing, could you take a look?

adborroto · 2022-03-30T20:11:05Z

@adborroto , looks like the added e2e test if failing, could you take a look?

Should be fixed now. Since the job to publish messages was already created no more messages were publish.
I added sh.exec("kubectl delete jobs/rabbitmq-publish-${queueName} --namespace ${namespace}") this should solve the issue.

Signed-off-by: Alejandro Dominguez <adborroto90@gmail.com>

JorTurFer · 2022-03-31T15:59:07Z

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

Signed-off-by: Alejandro Dominguez <adborroto90@gmail.com>

JorTurFer · 2022-03-31T19:00:49Z

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

JorTurFer

LGTM
Thanks!

zroubalik

Generaly looking good, I wonder, shouldn't we check that activationValue >= value ? Would it make sense to allow activationValue < value? 🤔

Signed-off-by: Alejandro Dominguez <adborroto90@gmail.com>

…adborroto/keda into feature/minMetricValue_rabbitmq

adborroto · 2022-04-08T14:56:09Z

Generaly looking good, I wonder, shouldn't we check that activationValue >= value ? Would it make sense to allow activationValue < value? 🤔

Good catch! I added the verification.

JorTurFer · 2022-04-08T18:30:37Z

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

zroubalik

LGTM, thanks for this contribution!

The only thing that's missing is finishing the docs and I think that we are good to merge then.

adborroto · 2022-05-11T16:06:18Z

Hi all, any idea to unlock this one??

zroubalik · 2022-05-11T18:41:55Z

@adborroto I think that we should have the general documentation on activation first.

JorTurFer · 2022-06-27T08:33:30Z

hey!
The documentation about the behavior is merged.
https://keda.sh/docs/2.8/concepts/scaling-deployments/#activating-and-scaling-thresholds

zroubalik · 2022-07-11T10:10:03Z

@adborroto could you please rebase this PR? I think that we are good to merge this one, WDYT @JorTurFer ?

JorTurFer · 2022-07-11T12:44:24Z

@adborroto could you please rebase this PR? I think that we are good to merge this one, WDYT @JorTurFer ?

Agree

JorTurFer · 2022-07-29T09:02:16Z

one update about this, I have been talking with @adborroto and he will give a try this weekend, if we cannot do it, I will tackle it

# Conflicts: # pkg/scalers/rabbitmq_scaler.go # pkg/scalers/rabbitmq_scaler_test.go # tests/scalers/rabbitmq-helpers.ts # tests/scalers/rabbitmq-queue-amqp.test.ts

…adborroto/keda into feature/minMetricValue_rabbitmq

Signed-off-by: Alejandro Dominguez <adborroto90@gmail.com>

JorTurFer

Looking good, only some nits inline
Thanks! ❤️

CHANGELOG.md

pkg/scalers/rabbitmq_scaler.go

JorTurFer · 2022-07-30T19:04:30Z

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

Signed-off-by: Alejandro Dominguez <adborroto90@gmail.com>

JorTurFer · 2022-07-30T19:40:48Z

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

tests/scalers_go/rabbitmq/rabbitmq_queue_amqp/rabbitmq_queue_amqp_test.go

…mqp_test.go Signed-off-by: jorturfer <jorge_turrado@hotmail.es>

Signed-off-by: jorturfer <jorge_turrado@hotmail.es>

JorTurFer · 2022-08-03T10:21:43Z

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

Signed-off-by: jorturfer <jorge_turrado@hotmail.es>

Activation threshold (minMetricValue) for Rabbitmq

077dd0c

Signed-off-by: Alejandro Dominguez <adborroto90@gmail.com>

adborroto requested a review from a team as a code owner March 27, 2022 11:06

update CHANGELOG

b23269a

Signed-off-by: Alejandro Dominguez <adborroto90@gmail.com>

adborroto mentioned this pull request Mar 27, 2022

ActivationValue for RabbitMQ scaler kedacore/keda-docs#733

Merged

1 task

JorTurFer requested changes Mar 28, 2022

View reviewed changes

pkg/scalers/rabbitmq_scaler.go Outdated Show resolved Hide resolved

tests/scalers/rabbitmq-queue-amqp.test.ts Outdated Show resolved Hide resolved

adborroto added 3 commits March 30, 2022 10:38

Change metric name to activationValue

bab43ec

Signed-off-by: Alejandro Dominguez <adborroto90@gmail.com>

Refactor unnecessary casting

cf3cef8

Signed-off-by: Alejandro Dominguez <adborroto90@gmail.com>

Resolve conflicts

ef248de

Signed-off-by: Alejandro Dominguez <adborroto90@gmail.com>

adborroto changed the title ~~Activation threshold (minMetricValue) for Rabbitmq~~ Activation threshold (activationValue) for Rabbitmq Mar 30, 2022

JorTurFer reviewed Mar 30, 2022

View reviewed changes

pkg/scalers/rabbitmq_scaler.go Outdated Show resolved Hide resolved

Fix typo

2c4142a

Signed-off-by: Alejandro Dominguez <adborroto90@gmail.com>

Fix integration test

ff0425f

Signed-off-by: Alejandro Dominguez <adborroto90@gmail.com>

adborroto requested a review from JorTurFer March 31, 2022 13:30

Fix line trim

fee0f3d

Signed-off-by: Alejandro Dominguez <adborroto90@gmail.com>

JorTurFer approved these changes Mar 31, 2022

View reviewed changes

zroubalik reviewed Apr 1, 2022

View reviewed changes

adborroto added 2 commits April 8, 2022 16:53

Fix line trim

a59818c

Signed-off-by: Alejandro Dominguez <adborroto90@gmail.com>

Merge branch 'feature/minMetricValue_rabbitmq' of https://github.com/…

1ed47dc

…adborroto/keda into feature/minMetricValue_rabbitmq

adborroto requested a review from zroubalik April 8, 2022 14:55

zroubalik approved these changes Apr 11, 2022

View reviewed changes

v-shenoy mentioned this pull request Jul 27, 2022

Migrate e2e tests for RabbitMQ to Go. #3428

Merged

2 tasks

adborroto added 4 commits July 30, 2022 11:33

Merge branch 'main' into feature/minMetricValue_rabbitmq

10b51d5

# Conflicts: # pkg/scalers/rabbitmq_scaler.go # pkg/scalers/rabbitmq_scaler_test.go # tests/scalers/rabbitmq-helpers.ts # tests/scalers/rabbitmq-queue-amqp.test.ts

Merge branch 'feature/minMetricValue_rabbitmq' of https://github.com/…

f121e03

…adborroto/keda into feature/minMetricValue_rabbitmq

tesst: remove ts files

2191c3d

Signed-off-by: Alejandro Dominguez <adborroto90@gmail.com>

style: fix format

a94dc54

Signed-off-by: Alejandro Dominguez <adborroto90@gmail.com>

JorTurFer reviewed Jul 30, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

pkg/scalers/rabbitmq_scaler.go Outdated Show resolved Hide resolved

pkg/scalers/rabbitmq_scaler.go Outdated Show resolved Hide resolved

pr: fix comments

0f2d65a

Signed-off-by: Alejandro Dominguez <adborroto90@gmail.com>

JorTurFer reviewed Jul 30, 2022

View reviewed changes

tests/scalers_go/rabbitmq/rabbitmq_queue_amqp/rabbitmq_queue_amqp_test.go Outdated Show resolved Hide resolved

Update tests/scalers_go/rabbitmq/rabbitmq_queue_amqp/rabbitmq_queue_a…

285728b

…mqp_test.go Signed-off-by: jorturfer <jorge_turrado@hotmail.es>

JorTurFer force-pushed the feature/minMetricValue_rabbitmq branch from 270b7de to 285728b Compare August 3, 2022 10:11

JorTurFer approved these changes Aug 3, 2022

View reviewed changes

remove unused import

f948b42

Signed-off-by: jorturfer <jorge_turrado@hotmail.es>

This was referenced Aug 3, 2022

feat: Introduce activationThreshold/minMetricValue for Redis Scalers kedacore/keda-docs#832

Merged

Introduce activationThreshold/minMetricValue for all scalers #2800

Closed

remove unecessary test case

af78509

Signed-off-by: jorturfer <jorge_turrado@hotmail.es>

JorTurFer added the skip-e2e label Aug 3, 2022

JorTurFer merged commit 98e937e into kedacore:main Aug 3, 2022

laithalissa mentioned this pull request Jun 5, 2023

Optionally use avg_rate from rabbitmq metrics and support setting parameters #4507

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Activation threshold (activationValue) for Rabbitmq #2831

Activation threshold (activationValue) for Rabbitmq #2831

adborroto commented Mar 27, 2022 •

edited by JorTurFer

Loading

JorTurFer left a comment •

edited

Loading

zroubalik commented Mar 28, 2022 •

edited

Loading

JorTurFer left a comment

JorTurFer commented Mar 30, 2022 •

edited by github-actions bot

Loading

JorTurFer commented Mar 30, 2022 •

edited by github-actions bot

Loading

JorTurFer commented Mar 30, 2022

adborroto commented Mar 30, 2022 •

edited

Loading

JorTurFer commented Mar 31, 2022 •

edited by github-actions bot

Loading

JorTurFer commented Mar 31, 2022 •

edited by github-actions bot

Loading

JorTurFer left a comment

zroubalik left a comment •

edited

Loading

adborroto commented Apr 8, 2022

JorTurFer commented Apr 8, 2022 •

edited by github-actions bot

Loading

zroubalik left a comment

adborroto commented May 11, 2022

zroubalik commented May 11, 2022

JorTurFer commented Jun 27, 2022

zroubalik commented Jul 11, 2022

JorTurFer commented Jul 11, 2022

JorTurFer commented Jul 29, 2022

JorTurFer left a comment

JorTurFer commented Jul 30, 2022 •

edited by github-actions bot

Loading

JorTurFer commented Jul 30, 2022 •

edited by github-actions bot

Loading

JorTurFer commented Aug 3, 2022 •

edited by github-actions bot

Loading

Activation threshold (activationValue) for Rabbitmq #2831

Activation threshold (activationValue) for Rabbitmq #2831

Conversation

adborroto commented Mar 27, 2022 • edited by JorTurFer Loading

Checklist

JorTurFer left a comment • edited Loading

Choose a reason for hiding this comment

zroubalik commented Mar 28, 2022 • edited Loading

JorTurFer left a comment

Choose a reason for hiding this comment

JorTurFer commented Mar 30, 2022 • edited by github-actions bot Loading

JorTurFer commented Mar 30, 2022 • edited by github-actions bot Loading

JorTurFer commented Mar 30, 2022

adborroto commented Mar 30, 2022 • edited Loading

JorTurFer commented Mar 31, 2022 • edited by github-actions bot Loading

JorTurFer commented Mar 31, 2022 • edited by github-actions bot Loading

JorTurFer left a comment

Choose a reason for hiding this comment

zroubalik left a comment • edited Loading

Choose a reason for hiding this comment

adborroto commented Apr 8, 2022

JorTurFer commented Apr 8, 2022 • edited by github-actions bot Loading

zroubalik left a comment

Choose a reason for hiding this comment

adborroto commented May 11, 2022

zroubalik commented May 11, 2022

JorTurFer commented Jun 27, 2022

zroubalik commented Jul 11, 2022

JorTurFer commented Jul 11, 2022

JorTurFer commented Jul 29, 2022

JorTurFer left a comment

Choose a reason for hiding this comment

JorTurFer commented Jul 30, 2022 • edited by github-actions bot Loading

JorTurFer commented Jul 30, 2022 • edited by github-actions bot Loading

JorTurFer commented Aug 3, 2022 • edited by github-actions bot Loading

adborroto commented Mar 27, 2022 •

edited by JorTurFer

Loading

JorTurFer left a comment •

edited

Loading

zroubalik commented Mar 28, 2022 •

edited

Loading

JorTurFer commented Mar 30, 2022 •

edited by github-actions bot

Loading

JorTurFer commented Mar 30, 2022 •

edited by github-actions bot

Loading

adborroto commented Mar 30, 2022 •

edited

Loading

JorTurFer commented Mar 31, 2022 •

edited by github-actions bot

Loading

JorTurFer commented Mar 31, 2022 •

edited by github-actions bot

Loading

zroubalik left a comment •

edited

Loading

JorTurFer commented Apr 8, 2022 •

edited by github-actions bot

Loading

JorTurFer commented Jul 30, 2022 •

edited by github-actions bot

Loading

JorTurFer commented Jul 30, 2022 •

edited by github-actions bot

Loading

JorTurFer commented Aug 3, 2022 •

edited by github-actions bot

Loading