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

[Proposal] Use unique name for metricName for each scaler #733

Closed
zroubalik opened this issue Apr 8, 2020 · 14 comments
Closed

[Proposal] Use unique name for metricName for each scaler #733

zroubalik opened this issue Apr 8, 2020 · 14 comments
Assignees
Labels
feature All issues for new features that have been committed to
Milestone

Comments

@zroubalik
Copy link
Member

zroubalik commented Apr 8, 2020

Currently metric name is hard coded to a specific value (see the example on Kafka scaler), this applies to most of the scalers. Metric name is essential for Metric server and HPA as it is used to distiguish the correct metrics used for scaling.

If we define two or more scalers of the same kind in one ScaledObject, scaling won't be correct as all the scalers will share the same metric name, eg. for Kafka it would be lagThreshold. This could be even problem when using specific combination fo different scalers, as they are using same metric name, eg. Kafka and Stan and probably some others as well.

Using multiple scalers of different kind (or same kind but with different metrics) in one ScaledObject is working: #702

If we want to support multiple scalers, we will need to do modification in every scaler. Each scaler would have to use unique metric name, it's up for discussion what approach would be the best, some of the possibilities are:

  • generate unique string
  • add unique preffix/suffix to the metric name
  • generate unique string/prefix/suffix only if there are mutliple scalers in ScaledObject (to keep the same metric name for the scalers that we have in v1)
  • enforce users to set name in trigger metadata in case they will specify more than one scaler in ScaledObject

Use-Case

There could be multiple scalers of the same kind in ScaledObject

Discussion

  • Do we want this feature for v2?
  • What is the best way to define metric name?

//EDIT added note about Kafka + Stan scalers

@zroubalik zroubalik added needs-discussion feature-request All issues for new features that have not been committed to labels Apr 8, 2020
@zroubalik zroubalik added this to the v2.0 milestone Apr 8, 2020
@tomkerkhove
Copy link
Member

Let's enforce this as of v2.0; think this is a good point of improvement!

add unique preffix/suffix to the metric name

I would go for this as it's consistent across scalers & scenario which we can use in our logging as well

@AmithGanesh
Copy link

I think having an optional parameter to provide a unique name . if it's not found then we can generate the unique name based on the parameters for the scalar.

@tomkerkhove
Copy link
Member

That's a good suggestion imo!

@zroubalik
Copy link
Member Author

Yeah, but we should bear in mind that the generator must be idempotent, provide the same "unique" value for the same trigger.

@AmithGanesh
Copy link

yes. i think we can generate it based on the parameters provided. for example : kafka-scalar-> "kafka-topic-group"

@zroubalik
Copy link
Member Author

@AmithGanesh are you volunteering to implement this? 😄

@AmithGanesh
Copy link

yes. i would like to take this up

@zroubalik
Copy link
Member Author

@AmithGanesh what is the status of this please?

@AmithGanesh
Copy link

Almost done. will submit a Pull Request by the end of the week.

@zroubalik
Copy link
Member Author

@AmithGanesh that's great! You might want to share your progress on a meeting this Thursday :)

@zroubalik
Copy link
Member Author

@AmithGanesh any update on this?

@AmithGanesh
Copy link

opened a PR for this. #866

@tomkerkhove tomkerkhove added feature All issues for new features that have been committed to and removed feature-request All issues for new features that have not been committed to needs-discussion labels Jun 1, 2020
@zroubalik zroubalik assigned samuelmacko and unassigned AmithGanesh Jul 8, 2020
@zroubalik
Copy link
Member Author

@samuelmacko agreed to finish this

@zroubalik
Copy link
Member Author

Solved in #966

SpiritZhou pushed a commit to SpiritZhou/keda that referenced this issue Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature All issues for new features that have been committed to
Projects
None yet
Development

No branches or pull requests

4 participants