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

Introduce activationThreshold/minMetricValue for all scalers #2800

Closed
6 of 7 tasks
zroubalik opened this issue Mar 22, 2022 · 14 comments
Closed
6 of 7 tasks

Introduce activationThreshold/minMetricValue for all scalers #2800

zroubalik opened this issue Mar 22, 2022 · 14 comments
Labels
feature-request All issues for new features that have not been committed to needs-discussion stale-bot-ignore All issues that should not be automatically closed by our stale bot

Comments

@zroubalik
Copy link
Member

zroubalik commented Mar 22, 2022

Proposal

Current activation (ie. scaling 0<->1) is happening if there's any activity on the target scaler, ie. the condition for comparison is metricFromScaler > 0. The threshold property on individual scalers is not respected, it is just applied to 1<->N scaling.
There are 2 scalers that supports flexible activation configuration, AWS Cloudwatch and Huawei Cloudeye (via minMetricValue property), where the default value is 0. The condition for comparison in this case is: metricFromScaler > minMetricValue.

We might want to introduce the same capability for all scalers.

  • introduce activation threshold for all scalers
  • properly document activation (the current state and the new option)

Pending to implement:

Will not implement this:

Use-Case

No response

Anything else?

No response

@zroubalik zroubalik added needs-discussion feature-request All issues for new features that have not been committed to labels Mar 22, 2022
@JorTurFer
Copy link
Member

@adborroto , @xoanmm
Maybe anyone of you is interested on this 😉

@adborroto
Copy link
Contributor

About the name of this new property. Will be a generic name or specific by sealer?
IMO, a specific name makes more sense and would be self-explanatory sometimes.

@zroubalik
Copy link
Member Author

Agree, I'd do something that I have suggested on the PR: activation* prefix to the current field that holds the value/queuelength/threshold in each scaler.

But I am definitely open to another options.

@JorTurFer
Copy link
Member

JorTurFer commented Mar 28, 2022

What do you think if we use this change to unify those parameters? I mean, we can rename all of them into threshold/activationThreshold and keep the consistency accros all the scalers.
Of course, the current case must be maintained but we can deprecate it. IMO having more options makes the things more complex, and at the end of the day, the "unit" of the threshold depends on the scaler, but the goal is always the same

@ybialik
Copy link

ybialik commented Apr 11, 2022

We would also like this feature for kafka scaler.

@JorTurFer
Copy link
Member

Related with this issue, during the last standup we decided to continue with it for all scalers, and we also agree with using activation* as a prefix for every scaler where * is the current metric name because it has enough context for each scaler users
image

@zroubalik
Copy link
Member Author

@ybialik as @JorTurFer mentioned, we plan to have this for all scalers, so you can contribute this for Kafka :)

@ybialik
Copy link

ybialik commented Apr 11, 2022

@zroubalik I am only a user, I can only contribute my feedback :)

@stale
Copy link

stale bot commented Jun 10, 2022

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 Jun 10, 2022
@JorTurFer JorTurFer added the stale-bot-ignore All issues that should not be automatically closed by our stale bot label Jun 10, 2022
@stale
Copy link

stale bot commented Jun 17, 2022

This issue has been automatically closed due to inactivity.

@v-shenoy
Copy link
Contributor

v-shenoy commented Aug 8, 2022

Is every scaler done now?

@zroubalik
Copy link
Member Author

Redis Streams is missing because we are not sure about the functionality. If you happen to have Redis Streams knowledge, or know anybody, open a PR for that please!

@zroubalik
Copy link
Member Author

@v-shenoy here is the Redis Streams related discussion: #3415 (comment)

@JorTurFer
Copy link
Member

I think this is ready. I close it for the moment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request All issues for new features that have not been committed to needs-discussion stale-bot-ignore All issues that should not be automatically closed by our stale bot
Projects
Archived in project
Development

No branches or pull requests

5 participants