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

[amirschw]

Proposal

Except for the CPU and memory scalers, all scalers, including the external scaler, only support the averageValue metric type. The proposal is to allow for other metric types (specifically value).

Edit: Apparently, Utilization is not supported for external metrics, so the proposal is only to support the Value metric type in addition to AverageValue.

Use-Case

There are already a few related issues with some use cases:

• Support "Value" metric type for RabbitMQ scaler #1314
• Incorrect calculation of metric from Prometheus #1881
• Fallback wrapper scaler  #1872
• StatefulSet not scaled  #1940
• Support "QueueLength/Value" metric type for Kafka scaler  #1942

Our use case is the following. We currently use the Azure Service Bus scaler for scaling one of our workloads. We would like to add another trigger (with an external scaler) that will scale the deployment based on the latency of the queue workers or the average dequeue delta. For this we need the absolute metric value in milliseconds and not the average value across all pods (e.g. we want to scale out when average latency is greater than 500ms).

Anything else?

The CPU and memory scalers already support different metric types by setting a required type in the scaled object metadata:

keda/pkg/scalers/cpu_memory_scaler.go

Lines 42 to 68 in b27de0c

	if val, ok := config.TriggerMetadata["type"]; ok && val != "" {
	meta.Type = v2beta2.MetricTargetType(val)
	} else {
	return nil, fmt.Errorf("no type given")
	}
	var value string
	var ok bool
	if value, ok = config.TriggerMetadata["value"]; !ok || value == "" {
	return nil, fmt.Errorf("no value given")
	}
	switch meta.Type {
	case v2beta2.ValueMetricType:
	valueQuantity := resource.MustParse(value)
	meta.Value = &valueQuantity
	case v2beta2.AverageValueMetricType:
	averageValueQuantity := resource.MustParse(value)
	meta.AverageValue = &averageValueQuantity
	case v2beta2.UtilizationMetricType:
	valueNum, err := strconv.ParseInt(value, 10, 32)
	if err != nil {
	return nil, err
	}
	utilizationNum := int32(valueNum)
	meta.AverageUtilization = &utilizationNum
	default:
	return nil, fmt.Errorf("unsupport type")

If we want to make this more generic, another approach can be to add the metric type to the trigger spec. Something like the below for example. The default metric type can be averageValue for maintaining backward compatibility.

triggers:
  - type: external
    metricType: value
    metadata:
      scalerAddress: external-scaler-service:8080
      ... # more metadata

After we discuss and agree on the approach we will be happy to contribute this.

[zroubalik]

Thanks for raising this issue 🎉

This is generally useful feature, I have a few remarks:

• we need to be sure that mixing different types of metricType in one ScaledObject works correctly - ie. define multiple triggers with different metricType. I am not sure how HPA handles that, we should investigate, most probably it won't be a problem
• we need to be sure that this works correctly with ScaledJob, as it is not using MetricServer and HPA for scaling, related: ScaledJob: introduce MultipleScalersCalculation #2016
• this should come with a solid documentation, we should explain different metricTypes with examples and consequencies on scaling
• I concur we should make it more generic, ie. adding this to triggers spec is probably the right place

Do you think that you can join our community call and discuss this feature there? Details: https://keda.sh/community/

[amirschw]

Thank you @zroubalik for responding and for the good remarks! I will be happy to join the next community call to discuss this. Should I just hop on the call or sign myself up somewhere?

[zroubalik]

Great, no need to sign anywhere. The call is open for everyone :)

[sqerison]

Hey guys. I am also looking forward to seeing this feature released. So +1

[fbalicchia]

Hi @amirschw , I don't know if you start to implements feature. In the meantime I've written POC for my case
extending Scaler interface proving an enum, adding a utils to set default check and sanitize input.
this is little bit different approach to add a spec
https://github.com/fbalicchia/keda/commit/d20b88615610bc1158d47a463c1f796fbbfa3f88

Tell me how do you want go head, when you finish I will happy to test it
Thanks

[amirschw]

Hey @fbalicchia, thanks for sharing your POC and apologies for the delay, I just got back from a long vacation (Jewish holidays... 😄). I should hopefully have time to get back to this soon.

[amirschw]

Just got back to this issue now. I hope to have at least a draft PR by tomorrow.

[amirschw]

@zroubalik, @sqerison, @fbalicchia, here's the PR: #2309.

[stale]

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]

This issue has been automatically closed due to inactivity.

[Punkoivan]

Hello folks!
Took a look at related PR. Once you make it, I'll be happy to test it :)

Currently searching for a way to use value instead of averageValue for Prometheus trigger.
Hope I'm looking into correct issue.

Thanks a lot for your work, you folks definitely make our lives easier.

[denzhel]

Hello folks! Took a look at related PR. Once you make it, I'll be happy to test it :)

Currently searching for a way to use value instead of averageValue for Prometheus trigger. Hope I'm looking into correct issue.

Thanks a lot for your work, you folks definitely make our lives easier.

Same here, would be happy to test it