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

feat: Add support for formula based evaluation of metrics values #4821

Merged
merged 37 commits into from
Sep 22, 2023

Conversation

gauron99
Copy link
Contributor

@gauron99 gauron99 commented Jul 25, 2023

So I have created (this big PR) but after some comments from others decided to separate it into 2 smaller PRs. So I extracted the functionality of formula and kept it here.

WHATS NEW

This change introduces a new feature I call "formula". The definition lies in SO (see below).

// scaledObject.yaml definition
spec:
  advanced:
    scalingModifiers:
      target: "2"
      formula: "(mapi_trig + kw_trig)/2"
...
  triggers:
  - type: kubernetes-workload
    name: kw_trig
    metadata:
      podSelector: 'pod=workload-test'
      value: '1'
  - type: metrics-api
    name: mapi_trig
    metadata:
      targetValue: "2"
      url: "https://mockbin.org/bin/336a8d99-9e09-4f1f-979d-851a6d1b1423"
      valueLocation: "tasks"
  • When formula is defined, it creates a new composite-scaler-metric that is passed on to HPA (hpa.go). HPA will make a request for this composite-metric ONLY instead of all external metrics. (resource triggers are untouched). This way all external metrics are collected at the same time and passed on to the formula itself (scale_handler.go). Formula is a string that specifies some mathematical/conditional statements and returns 1 number which is the final metric. Internally, it uses https://github.com/antonmedv/expr. (see language definition)
  • All external triggers must be named when using the formula because the trigger name is how its referenced in the formula itself. Otherwise some parsing would be required, to check whether specific trigger is used in formula and add only those that are.
  • The code above demonstrates working formula. -> takes mapi_trig and kw_trig, sums them up and divides by 2 aka creating an average of the 2.
  • If fallback exists, the formula is NOT applied, its skipped.

Additional info

Formula lies within the scalingModifiers struct which is used for additional changes (the second part of the big PR mentioned at the top would go here as well). The struct could also look like this:

advanced:
 modifiers:
  target: "2"
  formula: "xxx"
  external:
  - name: grpc_one
    url: localhost
    timeout: 10

or it could be separated entirely. The problem is where to put the target because it is mandatory for formula and optional for external grpc servers. (if grpc server is given but target not, it will modify each external metric on its own by users definition of Calculate method - more in external metric calculation PR - will open this after struct is decided

Checklist

Fixes #2440
Fixes #4823

Relates to #4583

@gauron99 gauron99 requested a review from a team as a code owner July 25, 2023 16:10
@gauron99 gauron99 marked this pull request as draft July 25, 2023 16:10
@github-actions
Copy link

Thank you for your contribution! 🙏 We will review your PR as soon as possible.

🏖️ Over the summer, the response time will be longer than usual due to maintainers taking time off so please bear with us.

While you are waiting, make sure to:

Learn more about:

@gauron99
Copy link
Contributor Author

/assign zroubalik

@matan-legit
Copy link

will this support boolean operators as well?

@gauron99 gauron99 force-pushed the formula_only_from_4583 branch 4 times, most recently from 4ef3dc4 to 3686015 Compare July 31, 2023 10:00
Copy link
Contributor Author

@gauron99 gauron99 left a comment

Choose a reason for hiding this comment

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

some self-review of what i thought were important points

apis/keda/v1alpha1/scaledobject_webhook.go Outdated Show resolved Hide resolved
apis/keda/v1alpha1/scaledobject_types.go Outdated Show resolved Hide resolved
@gauron99
Copy link
Contributor Author

gauron99 commented Aug 2, 2023

will this support boolean operators as well?

you can see what you can with the package in here: https://expr.medv.io/docs/Language-Definition
you can use anything that returns a single value, which will be passed to hpa for scaling

@gauron99 gauron99 marked this pull request as ready for review August 2, 2023 15:40
@gauron99
Copy link
Contributor Author

gauron99 commented Aug 2, 2023

I rechecked everything, hopefully i didnt miss anything. I started a self-review with the structure name.

Once this PR is merged I will match the second new feature of the big parent PR of the structure name etc. and open it for a review

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.

First of all, I 💘 this feature! Thanks a ton for implementing it!

I know that it's not something direct, but if we'd have a way to query CPU/Memory metrics to include them in the formula would be nice. I think that using multiple scalers, including the CPU/Memory is a common pattern. Primary scaling based on events, but having CPU/Memory as fallback.

Regarding with the formula calculation, maybe we can move it to it's own file/package to group all the related stuff. Maybe its own package for all the modifiers is worth as there is too much logic only for it this formula calculation and it'll increase in the future

CHANGELOG.md Outdated Show resolved Hide resolved
apis/keda/v1alpha1/scaledobject_types.go Outdated Show resolved Hide resolved
apis/keda/v1alpha1/scaledobject_webhook.go Outdated Show resolved Hide resolved
apis/keda/v1alpha1/scaledobject_webhook.go Outdated Show resolved Hide resolved
controllers/keda/hpa.go Outdated Show resolved Hide resolved
controllers/keda/hpa.go Outdated Show resolved Hide resolved
pkg/scaling/scale_handler.go Show resolved Hide resolved
pkg/scaling/scale_handler.go Show resolved Hide resolved
pkg/scaling/scale_handler.go Outdated Show resolved Hide resolved
@tomkerkhove
Copy link
Member

Is the doc PR up-to-date with the end-user experience @gauron99?

@gauron99
Copy link
Contributor Author

gauron99 commented Sep 4, 2023

Is the doc PR up-to-date with the end-user experience @gauron99?

its up to date with what i had in mind when i created the PR. Im expecting to change some names of structures etc based on some conversation above if thats what you mean. I have this @tomkerkhove which currently matches the code in this PR

@tomkerkhove
Copy link
Member

Thanks, added some doc comments: kedacore/keda-docs#1198 (review)

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.

Great stuff! I think we should mark this feature as an EXPERIMENATAL. This will allow us to change/modify the behavior after the release without the need for backward compatibility.

pkg/scaling/scale_handler.go Show resolved Hide resolved
apis/keda/v1alpha1/scaledobject_types.go Outdated Show resolved Hide resolved
apis/keda/v1alpha1/scaledobject_webhook.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
pkg/scaling/scale_handler.go Outdated Show resolved Hide resolved
pkg/scaling/scale_handler.go Show resolved Hide resolved
pkg/scaling/scale_handler.go Outdated Show resolved Hide resolved
pkg/scaling/scale_handler.go Outdated Show resolved Hide resolved
@gauron99
Copy link
Contributor Author

gauron99 commented Sep 11, 2023

@zroubalik @JorTurFer @tomkerkhove
Regarding the new package/file where to put the feature. I just want to start this comment on this so we can unify the decision.

Currently there are formula help functions in pkg/scaling/scale_handler.go that help to implement the feature and there are validating functions in apis/keda/v1alpha1/scaledobject_webhook.go that are used in the webhook itself and while creating hpa object in controllers/keda/hpa.go.

Im thinking of few possibilities:

  1. completely separate package which can implement each feature separately (even if we want to add more in the future) called something like scaling_modifiers with files like formula.go with validation either in the file or separate like formula_validation.go. It just cannot lie within the scaling package (pkg/scaling/) because it imports kedav1alpha1 therefore the validation in webhook would cause a cyclic dep.

  2. A file for the implementation called something like scaling_modifiers.go where all these new features could be but the validation would be separate in apis/keda/v1alpha1 in a file something like scaling_modifiers.go (to match the name maybe? or just add _validation.go at the end.

the question is if its ok for the implementation to be in the pkg/scaling package and validation be part of kedav1alpha1 and/or if its worth creating a whole new package for each/ both and where?

…dation

Signed-off-by: gauron99 <fridrich.david19@gmail.com>
Signed-off-by: gauron99 <fridrich.david19@gmail.com>
Signed-off-by: gauron99 <fridrich.david19@gmail.com>
Signed-off-by: gauron99 <fridrich.david19@gmail.com>
Signed-off-by: gauron99 <fridrich.david19@gmail.com>
Signed-off-by: gauron99 <fridrich.david19@gmail.com>
Signed-off-by: gauron99 <fridrich.david19@gmail.com>
Signed-off-by: gauron99 <fridrich.david19@gmail.com>
Signed-off-by: gauron99 <fridrich.david19@gmail.com>
Signed-off-by: gauron99 <fridrich.david19@gmail.com>
Signed-off-by: gauron99 <fridrich.david19@gmail.com>
Signed-off-by: gauron99 <fridrich.david19@gmail.com>
Signed-off-by: gauron99 <fridrich.david19@gmail.com>
Signed-off-by: gauron99 <fridrich.david19@gmail.com>
Signed-off-by: gauron99 <fridrich.david19@gmail.com>
Signed-off-by: gauron99 <fridrich.david19@gmail.com>
Signed-off-by: gauron99 <fridrich.david19@gmail.com>
Signed-off-by: gauron99 <fridrich.david19@gmail.com>
Signed-off-by: gauron99 <fridrich.david19@gmail.com>
Signed-off-by: gauron99 <fridrich.david19@gmail.com>
Signed-off-by: gauron99 <fridrich.david19@gmail.com>
@gauron99
Copy link
Contributor Author

@zroubalik @JorTurFer PTAL 🙏 Ive also created #4992

@zroubalik
Copy link
Member

zroubalik commented Sep 22, 2023

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

@JorTurFer
Copy link
Member

LGTM!
Thanks for this incredible feature

@zroubalik zroubalik enabled auto-merge (squash) September 22, 2023 15:47
@zroubalik zroubalik merged commit 2fb370b into kedacore:main Sep 22, 2023
18 checks passed
toniiiik pushed a commit to toniiiik/keda that referenced this pull request Jan 15, 2024
…acore#4821)

Signed-off-by: gauron99 <fridrich.david19@gmail.com>
Signed-off-by: anton.lysina <alysina@gmail.com>
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.

Add Trigger to stop scaling Support custom formula for multiple metrics
5 participants