Add support for the "Value" metric type in all scalers #2309

amirschw · 2021-11-21T08:40:04Z

Add support for ValueMetricType in all scalers except cpu/memory by introducing a new optional metricType field to the ScaleTrigger spec. The field will default to AverageValueMetricType to maintain backward compatibility.

Notes:

The Utilization metric type is not supported for external metrics, only Value or AverageValue.
The CPU/memory scalers will continue to support the Utilization and AverageValue metric types.
The fallback capability will continue to be supported for the AverageValue metric type only.

Checklist

Commits are signed with Developer Certificate of Origin (DCO - learn more)
Tests have been added
A PR is opened to update the documentation on (repo) (if applicable): Document the support for different metric types keda-docs#657
Changelog has been updated

Fixes #2030.

Signed-off-by: amirschw 24677563+amirschw@users.noreply.github.com

Signed-off-by: amirschw <24677563+amirschw@users.noreply.github.com>

JorTurFer · 2021-11-24T23:38:26Z

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

JorTurFer · 2021-11-25T08:36:43Z

/run-e2e mongodb.test*
Update: You can check the progres here

JorTurFer · 2021-11-25T08:37:12Z

/run-e2e azure-queue-trigger-auth*

Update: You can check the progres here

JorTurFer · 2021-11-29T07:32:50Z

/run-e2e mongodb.test*
Update: You can check the progres here

pkg/scalers/external_scaler.go

JorTurFer · 2021-12-01T22:17:33Z

/run-e2e mongodb.test*
Update: You can check the progres here

JorTurFer · 2021-12-01T22:19:56Z

hi @amirschw
In general, looks good! Thanks for it ❤️
My only concert is that it's a big change, and we should try it well. Let's wait for other opinions

zroubalik · 2021-12-02T13:12:58Z

@amirschw thanks for this proposal. So you decided to set the metric type on the ScaledObject level, what if the type could be specified on the trigger level? (each ScaledObject/Job could have mutliple scalers).

amirschw · 2021-12-02T13:36:47Z

@zroubalik I actually set the metric type on the trigger/scaler level as proposed in #2030, not on the the ScaledObject level (or at least I think I did... 😄).

See here:

keda/apis/keda/v1alpha1/scaledobject_types.go

Lines 121 to 132 in 5e51a85

    
           type ScaleTriggers struct { 
        
           	Type string `json:"type"` 
        
           	// +optional 
        
           	Name     string            `json:"name,omitempty"` 
        
           	Metadata map[string]string `json:"metadata"` 
        
           	// +optional 
        
           	AuthenticationRef *ScaledObjectAuthRef `json:"authenticationRef,omitempty"` 
        
           	// +optional 
        
           	FallbackReplicas *int32 `json:"fallback,omitempty"` 
        
           	// +optional 
        
           	MetricType autoscalingv2beta2.MetricTargetType `json:"metricType,omitempty"` 
        
           }

And here:

keda/pkg/scaling/scale_handler.go

Lines 290 to 299 in 5e51a85

    
           config := &scalers.ScalerConfig{ 
        
           	Name:              withTriggers.Name, 
        
           	Namespace:         withTriggers.Namespace, 
        
           	TriggerMetadata:   trigger.Metadata, 
        
           	ResolvedEnv:       resolvedEnv, 
        
           	AuthParams:        make(map[string]string), 
        
           	GlobalHTTPTimeout: h.globalHTTPTimeout, 
        
           	ScalerIndex:       scalerIndex, 
        
           	MetricType:        trigger.MetricType, 
        
           }

zroubalik · 2021-12-02T13:57:57Z

oh man 🤦 😄 I don't know why I thought that.

I have been quite busy recently, I will try to follow up on your PR ASAP. But I think that generally it is ok.

amirschw · 2021-12-02T14:36:42Z

No worries at all :) When you do get a chance to follow up, I will be happy to get your feedback about the fourth comment in the PR description (regarding how do we make sure it works correctly for ScaledJobs).

pavlospt · 2021-12-06T14:49:23Z

@amirschw Hello, we stumbled upon a business case today and we thought about contributing this change, until we found that you had a draft PR! Since every trigger on a ScaledObject can be of different type, why should this be on the ScaledObject level and on the metadata level of the trigger? Have you thought of some specific use-case?

If the metricType is top level on ScaleTriggers for semantic reasons, shouldn't the targetMetricValue fo there as well?

amirschw · 2021-12-07T15:59:42Z

Hi @pavlospt!

Since every trigger on a ScaledObject can be of different type, why should this be on the ScaledObject level and on the metadata level of the trigger? Have you thought of some specific use-case?

Not sure I got your question. I added the new metricType field as part of each trigger in the ScaledObject (or ScaledJob). This is exactly because every trigger on a ScaledObject can be of different type.

If the metricType is top level on ScaleTriggers for semantic reasons, shouldn't the targetMetricValue fo there as well?

Good question. I think it might be too big of a change to move the target value from the trigger metadata to reside directly under the trigger, especially since every trigger currently has a different name for the target value in its metadata, based on the scaler logic (e.g., queueLength for Azure Storage Queue, threshold for Prometheus, etc.). Will be happy to hear other opinions here.

pavlospt · 2021-12-07T16:05:12Z

Hi @pavlospt!

Since every trigger on a ScaledObject can be of different type, why should this be on the ScaledObject level and on the metadata level of the trigger? Have you thought of some specific use-case?

Not sure I got your question. I added the new metricType field as part of each trigger in the ScaledObject (or ScaledJob). This is exactly because every trigger on a ScaledObject can be of different type.

Yeah please ignore this part, my main concern is the following question!

If the metricType is top level on ScaleTriggers for semantic reasons, shouldn't the targetMetricValue fo there as well?

Good question. I think it might be too big of a change to move the target value from the trigger metadata to reside directly under the trigger, especially since every trigger currently has a different name for the target value in its metadata, based on the scaler logic (e.g., queueLength for Azure Storage Queue, threshold for Prometheus, etc.). Will be happy to hear other opinions here.

I understand the concers, maybe that could be a future improvement, but just for semantic reasons the fact that a type for the value is introduced, this should mean that the value as well now represents a "core" part of a trigger. It still remains a value independently of how any scaler treats it and its type is now configurable based on v2beta2.autoscaling!

Just my 2 cents on that since we were discussing it with my colleagues today 😃

amirschw · 2021-12-07T19:26:35Z

@pavlospt I see your point and agree it's worth considering it as a possible future improvement (KEDA v3? 😃)

zroubalik

Looking good! Sorry for the delay in a review.

ad 3. Could you please describe the issue you have with Fallback?

ad 4. You are right, we might probably end up with not supporting this setting for ScaledJob 🤔 ? @TsuyoshiUshio wdyt?

amirschw · 2021-12-08T18:02:37Z

Thanks for the review @zroubalik!

Regarding fallback: the cool thing about AverageValue is that we don't need to know how many replicas are available in order to scale to the value of "fallback replicas". If, for example, the target average value = 100, and fallback replicas = 5, then the scaler will return 500 (100 * 5) for the metric value based on this formula. And then, since we're using AverageValue, the HPA controller will scale according to the algorithm to 5 replicas: ceil[currentReplicas * ( currentMetricValue / desiredMetricValue )] = ceil[currentReplicas * ( (500 / currentReplicas) / 100 )] = ceil(500 / 100) = 5.

When Value is used, I don't think there's a way to scale to X fallback replicas without knowing how many replicas are currently available. This is probably why the isFallbackEnabled function checks that the metric type is AverageValueMetricType.

zroubalik · 2021-12-08T19:13:36Z

@amirschw gotcha. We can try to pull info from the ScaleTarget in this case, to get the number of replicas. Will that help you with modification of the fallback algorithm?

keda/pkg/scaling/executor/scale_scaledobjects.go

Lines 254 to 256 in 4a1e358

    
           func (e *scaleExecutor) getScaleTargetScale(ctx context.Context, scaledObject *kedav1alpha1.ScaledObject) (*autoscalingv1.Scale, error) { 
        
           	return e.scaleClient.Scales(scaledObject.Namespace).Get(ctx, scaledObject.Status.ScaleTargetGVKR.GroupResource(), scaledObject.Spec.ScaleTargetRef.Name, metav1.GetOptions{}) 
        
           }

zroubalik · 2022-03-29T08:59:23Z

@amirschw following tests:

scalers/rabbitmq-queue-http.test.ts
scalers/rabbitmq-queue-http-regex-vhost.test.ts
scalers/rabbitmq-queue-http-regex.test.ts

failed with:

 ***.648543265835394***e+09	ERROR	controller.scaledobject	Failed to create new HPA in cluster	***"reconciler group": "keda.sh", "reconciler kind": "ScaledObject", "name": "test-scaledobject", "namespace": "rabbitmq-queue-http-regex-vhost-test", "HPA.Namespace": "rabbitmq-queue-http-regex-vhost-test", "HPA.Name": "keda-hpa-test-scaledobject", "error": "HorizontalPodAutoscaler.autoscaling \"keda-hpa-test-scaledobject\" is invalid: [spec.metrics[0].external.target.type: Required value: must specify a metric target type, spec.metrics[0].external.target.type: Invalid value: \"\": must be either Utilization, Value, or AverageValue]"***
github.com/kedacore/keda/v2/controllers/keda.(*ScaledObjectReconciler).ensureHPAForScaledObjectExists
	/workspace/controllers/keda/scaledobject_controller.go:36***
github.com/kedacore/keda/v2/controllers/keda.(*ScaledObjectReconciler).reconcileScaledObject
	/workspace/controllers/keda/scaledobject_controller.go:229
github.com/kedacore/keda/v2/controllers/keda.(*ScaledObjectReconciler).Reconcile
	/workspace/controllers/keda/scaledobject_controller.go:***80
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Reconcile
	/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.***.0/pkg/internal/controller/controller.go:***4
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler
	/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.***.0/pkg/internal/controller/controller.go:3***
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem
	/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.***.0/pkg/internal/controller/controller.go:266
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2
	/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.***.0/pkg/internal/controller/controller.go:227
***.648543265835483e+09	ERROR	controller.scaledobject	Failed to ensure HPA is correctly created for ScaledObject	***"reconciler group": "keda.sh", "reconciler kind": "ScaledObject", "name": "test-scaledobject", "namespace": "rabbitmq-queue-http-regex-vhost-test", "error": "HorizontalPodAutoscaler.autoscaling \"keda-hpa-test-scaledobject\" is invalid: [spec.metrics[0].external.target.type: Required value: must specify a metric target type, spec.metrics[0].external.target.type: Invalid value: \"\": must be either Utilization, Value, or AverageValue]"***

zroubalik · 2022-03-29T08:59:39Z

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

tests/scalers/prometheus.test.ts

JorTurFer

LGTM!
Really, thanks for the huge effort that you have done with this PR! ❤️ ❤️

JorTurFer · 2022-03-29T19:40:23Z

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

Signed-off-by: amirschw <24677563+amirschw@users.noreply.github.com>

JorTurFer · 2022-03-29T20:15:55Z

@amirschw , really, KUDOS to you. You have done a great (and huge) work with this PR 🙇
I'd like to help you with the rabbitmq problem. I have debugged it and there is one change missing, RabbitMQ Scaler has two different places where the scaler is built and you have changed only one of them.
You have changed this, but you need to update with the metricType this too

Thanks again for this awesome contribution

Only one more thing, could you rebase main to fix the changelog conflicts? That will also pull new AWS e2e test, and we can retrigger e2e test again testing those scalers also 🙏

zroubalik · 2022-03-29T20:20:37Z

RabbitMQ Scaler has to different places where the scaler is built and you have changed only one of them.
You have changed this, but you need to update with the metricType this too

Would be great if you could refactor that part of the code, so we create just on struct rabbitMQScaler{} at one place and returning it just once. So we prevent similar mistake again. Thanks!

Signed-off-by: amirschw <24677563+amirschw@users.noreply.github.com>

amirschw · 2022-03-29T20:29:19Z

@zroubalik, @JorTurFer thanks! I was just debugging the rabbitmq failure myself and pushed a change without checking your comments. Will address your latest comments shortly 😄

tests/scalers/prometheus.test.ts

Signed-off-by: amirschw <24677563+amirschw@users.noreply.github.com>

pkg/scalers/rabbitmq_scaler.go

Signed-off-by: amirschw <24677563+amirschw@users.noreply.github.com>

zroubalik · 2022-03-30T06:51:15Z

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

pkg/scalers/rabbitmq_scaler.go

zroubalik

LGTM,

/hold for @JorTurFer for the final review

thanks a lot!

zroubalik · 2022-03-30T08:09:38Z

We should open an issue to remove deprecated fields in the CPU/Memory scaler sometime in the future, once this get merged.

amirschw · 2022-03-30T08:36:18Z

We should open an issue to remove deprecated fields in the CPU/Memory scaler sometime in the future, once this get merged.

Good idea. I opened #2844 for this.

JorTurFer · 2022-03-30T11:29:19Z

LGTM,

/hold for @JorTurFer for the final review

thanks a lot!

LGTM too!
Thanks for the awesome improvement

amirschw requested review from ahmelsayed and zroubalik as code owners November 21, 2021 08:40

Add an optional metric type field to the scale trigger spec

5e51a85

Signed-off-by: amirschw <24677563+amirschw@users.noreply.github.com>

amirschw force-pushed the metric-type branch from 6786b78 to 5e51a85 Compare November 21, 2021 08:40

amirschw mentioned this pull request Nov 21, 2021

Add support for the "Value" metric type in addition to "AverageValue" #2030

Closed

tomkerkhove added this to the v2.6.0 milestone Nov 23, 2021

tomkerkhove marked this pull request as draft November 23, 2021 16:16

tomkerkhove assigned ahmelsayed, zroubalik and JorTurFer Nov 23, 2021

JorTurFer reviewed Dec 1, 2021

View reviewed changes

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

arschles approved these changes Dec 3, 2021

View reviewed changes

zroubalik reviewed Dec 8, 2021

View reviewed changes

zroubalik assigned TsuyoshiUshio Dec 8, 2021

zroubalik reviewed Mar 29, 2022

View reviewed changes

tests/scalers/prometheus.test.ts Outdated Show resolved Hide resolved

JorTurFer approved these changes Mar 29, 2022

View reviewed changes

omit default metric type from prometheus e2e test

5c0624f

Signed-off-by: amirschw <24677563+amirschw@users.noreply.github.com>

add missing metricType in rabbitmq scaler

f9ae0d7

Signed-off-by: amirschw <24677563+amirschw@users.noreply.github.com>

zroubalik reviewed Mar 29, 2022

View reviewed changes

tests/scalers/prometheus.test.ts Outdated Show resolved Hide resolved

amirschw added 3 commits March 29, 2022 23:32

Merge branch 'main' into metric-type

88aa974

Signed-off-by: amirschw <24677563+amirschw@users.noreply.github.com>

create and return rabbitMQScaler just once

49fb1f3

Signed-off-by: amirschw <24677563+amirschw@users.noreply.github.com>

improve e2e tests to also check the hpa metric type

7f0f21b

Signed-off-by: amirschw <24677563+amirschw@users.noreply.github.com>

amirschw commented Mar 29, 2022

View reviewed changes

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

fix bug in rabbitmq scaler

5c8a0ab

Signed-off-by: amirschw <24677563+amirschw@users.noreply.github.com>

zroubalik reviewed Mar 30, 2022

View reviewed changes

pkg/scalers/rabbitmq_scaler.go Show resolved Hide resolved

zroubalik approved these changes Mar 30, 2022

View reviewed changes

amirschw mentioned this pull request Mar 30, 2022

Remove deprecated "type" field from the CPU, Memory, Datadog scalers metadata #2844

Open

zroubalik merged commit d5483cc into kedacore:main Mar 30, 2022

amirschw deleted the metric-type branch March 30, 2022 17:50

amirschw mentioned this pull request Mar 30, 2022

Add a deprecation notice for the 'type' field #2848

Merged

2 tasks

zroubalik mentioned this pull request Apr 19, 2022

changed metric type to value instead average #2909

Closed

5 tasks

amirschw mentioned this pull request Jun 9, 2022

Document ScaledObject support for Value metric type in FAQ kedacore/keda-docs#777

Merged

1 task

amirschw mentioned this pull request Aug 24, 2022

cron scaler metricType shouldn't be configurable #3591

Closed

KevinCarterDev mentioned this pull request May 16, 2023

Allow setting metricType on ScaledObject porter-dev/porter-charts#642

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for the "Value" metric type in all scalers #2309

Add support for the "Value" metric type in all scalers #2309

amirschw commented Nov 21, 2021 •

edited

Loading

JorTurFer commented Nov 24, 2021 •

edited by github-actions bot

Loading

JorTurFer commented Nov 25, 2021 •

edited by github-actions bot

Loading

JorTurFer commented Nov 25, 2021 •

edited by github-actions bot

Loading

JorTurFer commented Nov 29, 2021 •

edited by github-actions bot

Loading

JorTurFer commented Dec 1, 2021 •

edited by github-actions bot

Loading

JorTurFer commented Dec 1, 2021

zroubalik commented Dec 2, 2021

amirschw commented Dec 2, 2021

zroubalik commented Dec 2, 2021 •

edited

Loading

amirschw commented Dec 2, 2021

pavlospt commented Dec 6, 2021 •

edited

Loading

amirschw commented Dec 7, 2021

pavlospt commented Dec 7, 2021

amirschw commented Dec 7, 2021

zroubalik left a comment

amirschw commented Dec 8, 2021 •

edited

Loading

zroubalik commented Dec 8, 2021

zroubalik commented Mar 29, 2022 •

edited

Loading

zroubalik commented Mar 29, 2022 •

edited by github-actions bot

Loading

JorTurFer left a comment

JorTurFer commented Mar 29, 2022 •

edited by github-actions bot

Loading

JorTurFer commented Mar 29, 2022 •

edited by zroubalik

Loading

zroubalik commented Mar 29, 2022 •

edited

Loading

amirschw commented Mar 29, 2022

zroubalik commented Mar 30, 2022 •

edited by github-actions bot

Loading

zroubalik left a comment •

edited

Loading

zroubalik commented Mar 30, 2022 •

edited

Loading

amirschw commented Mar 30, 2022

JorTurFer commented Mar 30, 2022

Add support for the "Value" metric type in all scalers #2309

Add support for the "Value" metric type in all scalers #2309

Conversation

amirschw commented Nov 21, 2021 • edited Loading

Checklist

JorTurFer commented Nov 24, 2021 • edited by github-actions bot Loading

JorTurFer commented Nov 25, 2021 • edited by github-actions bot Loading

JorTurFer commented Nov 25, 2021 • edited by github-actions bot Loading

JorTurFer commented Nov 29, 2021 • edited by github-actions bot Loading

JorTurFer commented Dec 1, 2021 • edited by github-actions bot Loading

JorTurFer commented Dec 1, 2021

zroubalik commented Dec 2, 2021

amirschw commented Dec 2, 2021

zroubalik commented Dec 2, 2021 • edited Loading

amirschw commented Dec 2, 2021

pavlospt commented Dec 6, 2021 • edited Loading

amirschw commented Dec 7, 2021

pavlospt commented Dec 7, 2021

amirschw commented Dec 7, 2021

zroubalik left a comment

Choose a reason for hiding this comment

amirschw commented Dec 8, 2021 • edited Loading

zroubalik commented Dec 8, 2021

zroubalik commented Mar 29, 2022 • edited Loading

zroubalik commented Mar 29, 2022 • edited by github-actions bot Loading

JorTurFer left a comment

Choose a reason for hiding this comment

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

JorTurFer commented Mar 29, 2022 • edited by zroubalik Loading

zroubalik commented Mar 29, 2022 • edited Loading

amirschw commented Mar 29, 2022

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

zroubalik left a comment • edited Loading

Choose a reason for hiding this comment

zroubalik commented Mar 30, 2022 • edited Loading

amirschw commented Mar 30, 2022

JorTurFer commented Mar 30, 2022

amirschw commented Nov 21, 2021 •

edited

Loading

JorTurFer commented Nov 24, 2021 •

edited by github-actions bot

Loading

JorTurFer commented Nov 25, 2021 •

edited by github-actions bot

Loading

JorTurFer commented Nov 25, 2021 •

edited by github-actions bot

Loading

JorTurFer commented Nov 29, 2021 •

edited by github-actions bot

Loading

JorTurFer commented Dec 1, 2021 •

edited by github-actions bot

Loading

zroubalik commented Dec 2, 2021 •

edited

Loading

pavlospt commented Dec 6, 2021 •

edited

Loading

amirschw commented Dec 8, 2021 •

edited

Loading

zroubalik commented Mar 29, 2022 •

edited

Loading

zroubalik commented Mar 29, 2022 •

edited by github-actions bot

Loading

JorTurFer commented Mar 29, 2022 •

edited by github-actions bot

Loading

JorTurFer commented Mar 29, 2022 •

edited by zroubalik

Loading

zroubalik commented Mar 29, 2022 •

edited

Loading

zroubalik commented Mar 30, 2022 •

edited by github-actions bot

Loading

zroubalik left a comment •

edited

Loading

zroubalik commented Mar 30, 2022 •

edited

Loading