Same metric name with multiple scaled object defined

[cdlliuy]

Report

We had multiple scaling metrics with azure-data-explorer-scaler when the metric extracted from the same azure-data-explorer-db, but from different query.

i.e I created a trigger using metricName "metric-A" to run query "query-A", then "metric-B" with run with "query-B"

  triggers:
  - authenticationRef:
      name: kusto-trigger-auth
    metadata:
      databaseName: azure-data-explorer-dbName
      endpoint:  azure-data-explorer-dbUrl
      metricName:  metric-A
      query: |
        <azure-data-explorer-query-A>
      threshold: "10"
    type: azure-data-explorer
  - authenticationRef:
      name: kusto-trigger-auth
    metadata:
      databaseName: azure-data-explorer-dbName
      endpoint:  azure-data-explorer-dbUrl
      metricName:  metric-B
      query: |
        <azure-data-explorer-query-B>
      threshold: "10"
    type: azure-data-explorer

With current implementation, both of the scaled object has the same metric name:

  externalMetricNames:
  - s0-azure-data-explorer-dbName
  - s1-azure-data-explorer-dbName

It is hard to remember the exactly query purpose with the externalMetricNames, which bring troubles with monitoring.

Expected Behavior

I would like to suggest to add the orignal metadata.metricName in the final externalMetricNames, i.e. s0-metricName-A-azure-data-explorer-dbName in above example

Actual Behavior

  externalMetricNames:
  - s0-azure-data-explorer-dbName
  - s1-azure-data-explorer-dbName

Steps to Reproduce the Problem

See above description

Logs from KEDA operator

example

KEDA Version

No response

Kubernetes Version

No response

Platform

No response

Scaler Details

azure-data-explorer-scaler

Anything else?

I will raise a PR to address this issue

[JorTurFer]

I think that we shouldn't do this. The metric name is an internal KEDA thing that it's used to avoid conflicts. Previously we gave the option to "customize it" to avoid conflicts with duplicated metric names, but that it's already solved, and every metric is unique inside the SO. Some scalers have that option because they had it before the changes, and we are removing the support to it.

@kedacore/keda-core-contributors ?

[tomkerkhove]

Is the composition of the internal metric name specific per scaler or in general? Because from the example above, it looks like it's scaler specific (except for the s{n} previx)

[cdlliuy]

I think this is a scaler specific issue.
The Azure-data-explorer scaler used "s{n} + databasename" as metricName when creating the metric object, which does make troubles for monitoring purpose.

i.e. when monitoring the metric from Prometheus, the "metric" name is too similar and hard to explain its exact meaning to others.

keda_metrics_adapter_scaler_metrics_value{container="keda-operator-metrics-apiserver", endpoint="metrics", exported_namespace="default", instance="172.18.7.104:9022", job="monitoring/prometheus-podmonitor", metric="s1-azure-data-explorer-dbname", namespace="keda", pod="keda-operator-metrics-apiserver-66dcb4ddfc-lq7g8", scaledObject="app-scaledobject", scaler="azureDataExplorerScaler", scalerIndex="1"} = 89

keda_metrics_adapter_scaler_metrics_value{container="keda-operator-metrics-apiserver", endpoint="metrics", exported_namespace="default", instance="172.18.7.104:9022", job="monitoring/prometheus-podmonitor", metric="s2-azure-data-explorer-dbname", namespace="keda", pod="keda-operator-metrics-apiserver-66dcb4ddfc-lq7g8", scaledObject="app-scaledobject", scaler="azureDataExplorerScaler", scalerIndex="2"} = 171

When the user has defined a meaningful metric name, I think the scaler need to at least pick up the origin trigger.metadata.metricName when creating their own one.

That is what I am trying to do in PR #3589 .

If there is any better solution to solve the monitoring pain point, I am glad to adopt as well :-)

[JorTurFer]

I'm not an expert on that scaler, but based on docs, users don't have any parameter to provide metric name, I guess that's because the scaler doesn't need that information for working. We try to avoid supporting extra metric name parameters because as I said, we used it to avoid conflicts that now don't happen any more.

Is the ScaledObject name not enough for monitoring proposes? I mean, with the SO name you could get all the info

[cdlliuy]

@JorTurFer , scaledobject name is not enough.

Per KEDA docs, it is recommended to define multiple triggers for the same scaling target,

KEDA allows you to use multiple triggers as part of the same ScaledObject or ScaledJob.

By doing this, your autoscaling becomes better:

All your autoscaling rules are in one place
You will not have multiple ScaledObject’s or ScaledJob’s interfering with each other
KEDA will start scaling as soon as when one of the triggers meets the criteria. Horizontal Pod Autoscaler (HPA) will calculate metrics for every scaler and use the highest desired replica count to scale the workload to.

That means, if you have a deployment A, and you hope to auto scale A under different conditions , you need to define a single scaledobject for A with multiple triggers. In this case, adding "scaledobject" name can't help at all to distinguish the different metric for triggers.

In my example in original post, I defined 2 triggers based on the different query of "azure-data-explorer". , even adding "scaledobject name", the metric name will still be "s0-azure-data-explorer-dbName-scaledobjectName" and "s1-azure-data-explorer-dbName-scaledobjectName". These names are not meaningful enough.

See metric example below, how can I tell the exactly meaning of the each metric?

keda_metrics_adapter_scaler_metrics_value{ metric="s1-azure-data-explorer-dbname", scaledObject="app-scaledobject",  scalerIndex="1"} = 89

keda_metrics_adapter_scaler_metrics_value{metric="s2-azure-data-explorer-dbname", scaledObject="app-scaledobject", scalerIndex="2"} = 171

So, I hope the customized trigger.metadata.metricName can be honored.

The metricName is common in other scalers.
i.e. prometheus scaler, which used the trigger.metadata.metricName metricName directly as the final metricName.
https://github.com/kedacore/keda/blob/main/pkg/scalers/prometheus_scaler.go#L118-L119

[zroubalik]

I also vote for not including this parametr in the scaler metadata. We are actively trying to get rid of these, to avoid confusion.

[cdlliuy]

@zroubalik , then how to resolve the monitoring pain point? See my example:

When creating the trigger, I set 2 different trigger.metadata.metricName , one is "inProgressTaskNumber", another is "pendingTaskNumber", but from the monitoring metric, all the meaningful info is erased. How can I explain to others that "s1" is the "inProgreesNumber" and "s2" is the "Pending"?

I think metricName should not be treated to avoid duplication only. It does give people a place to add meaning description to the trigger.

If metricName is not supported anymore, could we discuss to add a "metricDescription" field, and add the "metricDescription" into the metric "keda_metrics_adapter_scaler_metrics_value" ?

[tomkerkhove]

Isn't the best solution that end-users can specify a suffix that we use after s{n} which has to be unique for the SO but is not in the metadata? I definitely get why end-users would want to have control over this

[cdlliuy]

@tomkerkhove , yes, suffix can work, but it should be unique for each trigger, not for "SO" (considering multiple triggers for one SO).

[zroubalik]

I completely understand the UX problem. Though I don't think this is the best solution.

What I would suggest is to use this (at the moment unused) field:

keda/apis/keda/v1alpha1/scaledobject_types.go

Line 126 in 40da2e2

Name string `json:"name,omitempty"`

Basically to give a custom name to each trigger, that will be completely unrelated to the internal metric names. We can expose this property in the prom metrics. We will need to use such field anyway if we would like to have this functionality: #2440 (which is unrelated to this issue, but depends on the same).

Another benefit is, that we could implement this for all scalers at once.

WDYT?

[tomkerkhove]

Sounds good to me

[zroubalik]

That^ change should come hand in hand with a slow deprecation of metricName field for scalers that have it at the moment. Probably starting by making this property optional.

[cdlliuy]

@zroubalik , if my understanding is correct, I can start to define trigger.name directly in current KEDA version. But to show the "name" in grafana, additional code changes to enable "name" as a label is required. Correct?

[zroubalik]

Correct.

[JorTurFer]

I think atm that field it's not documented but I'm willing to open a PR adding the field to the exported metrics and also documenting.
could this solve the issue?

[cdlliuy]

@JorTurFer , I didn't notice your comments ....
Instead, I created a PR to fix the issue #3650 per the suggestion in above #3588 (comment) by @zroubalik

can you help to review?

[JorTurFer]

sure, I'll do it