Improve metric name creation to be unique using scaler index inside the scaler

[JorTurFer]

Signed-off-by: jorturfer [email protected]

This draft shows the approach of using the scaler index as a prefix to ensure all metric names are unique in the ScaledObject during the scaler constructor. All the metric names will be composed using the pattern sX-currentMetricName where x is the index.

Checklist

• Commits are signed with Developer Certificate of Origin (DCO - learn more)
• Tests have been added
• A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified)
• A PR is opened to update the documentation on (repo) (if applicable) Update metric names where the names are explicitly shown keda-docs#550
• Changelog has been updated

Fixes #2123

[JorTurFer]

Could you execute e2e test using this branch? I have tried several of them, but to be sure that they are working (and I don't have accounts to test all of them)

[zroubalik]

This is great!

[zroubalik]

We should have to go through the documentation and fix the example metric names (to include sX- prefix) on some scalers.

[JorTurFer]

We should have to go through the documentation and fix the example metric names (to include sX- prefix) on some scalers.

Ouch, I didn't think on that... I will review them but I think that the change is totally internal. I mean, if a user specify the metricName (ex, on postgresql), the external metric name will be sX-userMetricName.
We use index always.

I will check it and probably we should add a note also in docs to refer this change. From KEDA pov is not important because the hpa will be recreated, but maybe some users are quering the metric directly and this could ba a breaking change.
WDYT? Should we add a new section there?

[zroubalik]

We should have to go through the documentation and fix the example metric names (to include sX- prefix) on some scalers.

Ouch, I didn't think on that... I will review them but I think that the change is totally internal. I mean, if a user specify the metricName (ex, on postgresql), the external metric name will be sX-userMetricName. We use index always.

There are examples of the generated metric names in some scalers. For example here: https://keda.sh/docs/2.4/scalers/mongodb/ find on the page occurencies of mongodb-mongodb—test_user-test_password@xxx-27017-test-test_collection

I will check it and probably we should add a note also in docs to refer this change. From KEDA pov is not important because the hpa will be recreated, but maybe some users are quering the metric directly and this could ba a breaking change. WDYT? Should we add a new section there?

Yeah probably a section on HPA and metrics would be fine, with addition:

• with the example how to query metrics directly from k8s server: kubectl get --raw "/apis/external.metrics.k8s.io/v1beta1/namespaces/$NAMESPACE/$METRIC_NAME" | jq .
• how metrics are calculated, as described here: https://github.com/kedacore/keda/blob/main/CREATE-NEW-SCALER.md#getmetrics

[JorTurFer]

We should have to go through the documentation and fix the example metric names (to include sX- prefix) on some scalers.

Ouch, I didn't think on that... I will review them but I think that the change is totally internal. I mean, if a user specify the metricName (ex, on postgresql), the external metric name will be sX-userMetricName. We use index always.

There are examples of the generated metric names in some scalers. For example here: https://keda.sh/docs/2.4/scalers/mongodb/ find on the page occurencies of mongodb-mongodb—test_user-test_password@xxx-27017-test-test_collection

I will check it and probably we should add a note also in docs to refer this change. From KEDA pov is not important because the hpa will be recreated, but maybe some users are quering the metric directly and this could ba a breaking change. WDYT? Should we add a new section there?

Yeah probably a section on HPA and metrics would be fine, with addition:

• with the example how to query metrics directly from k8s server: kubectl get --raw "/apis/external.metrics.k8s.io/v1beta1/namespaces/$NAMESPACE/$METRIC_NAME" | jq .
• how metrics are calculated, as described here: https://github.com/kedacore/keda/blob/main/CREATE-NEW-SCALER.md#getmetrics

I will do it ASAP, after the netcoreconf last weekend, I have to earn "family points" xD.
I'l try to do at night

[zroubalik]

@JorTurFer oh boy :) no rush at all, family first.

[zroubalik]

Looking good, just nits

[zroubalik]

Looking good, the only thing I am not sure are potential consequences for External scaler, @ahmelsayed wdyt?

[JorTurFer]

Looking good, the only thing I am not sure are potential consequences for External scaler, @ahmelsayed wdyt?

Maybe we could let external scaler as it was before this change and delegate to the user to avoid collisions here 🤔

@zroubalik (IDK how you edited my comment xD):
Yeah, I meant whether there is needed some changes in external scaler docs and whatnot.