metricName shouldn't be required and should be deprecated for all the scalers

[JorTurFer]

Proposal

Currently, some scalers require a metricName which is used only for KEDA internal things (it's not sent to the prom server).
This requirement isn't necessary since we introduced mechanisms to ensure the unique metrics names inside the ScaledObject and we should make it optional and also deprecate it in favour of self-generated internal name

Scalers:

• aws-cloudwatch - metric name is needed for this one
• azure-log-analytics
• azure-monitor
• azure-storage-blob
• cassandra
• couchdb - just lefover in the code, doesn't have to be included in the docs and deprecations
• graphite
• huawei-cloudeye - metric name is needed for this one
• influxdb
• mongodb
• mssql
• postgresql
• prometheus
• rabbitmq-queue

TODO list

• code change to print warning message with deprecation notice Deprecate explicitly setting metricName for all scalers #4235
• update documentation to specify deprecation notice - our deprecation policy - https://github.com/kedacore/governance/blob/main/DEPRECATIONS.md Deprecate explicitly setting metricName for all scalers keda-docs#1074
• create a new issue to actually remove this code once deprecation period is over (mark it this way: https://github.com/kedacore/keda/issues?q=is%3Aissue+is%3Aopen+sort%3Aupdated-desc+label%3Abreaking-change) - Remove deprecated field metricName from Scalers. #4240
• update deprecation notice in the Changelog Deprecate explicitly setting metricName for all scalers #4235
• create discussion for the deprecation like these: https://github.com/kedacore/keda/discussions/categories/deprecations

[JorTurFer]

@kedacore/keda-core-contributors , do you agree with this?

[zroubalik]

Agree, this should be done for all scalers, not just Prometheus.

[zroubalik]

@JorTurFer could you please identify those scalers and create a separate issues for them? And give them good first issues/help wanted labels? As these could be ideal issues for newcomers.

[JorTurFer]

Yeah, I will do a round checking all the scalers.
I rename this issue with a generic title to track the new issues I'll create

[JorTurFer]

I have found 4 scalers where this happens. I have created an issue for each one and update this issue to track them

[dttung2905]

@JorTurFer I could help with 1 or 2. If you can do 1 first as an example, I could look at it and see what are needed to be changed 🙏

[JorTurFer]

@JorTurFer I could help with 1 or 2. If you can do 1 first as an example, I could look at it and see what are needed to be changed 🙏

Sure, I will do one of them this week (I have a trip today and I'll be outside until Wednesday night)

[zroubalik]

What should be done:

• code change to print warning message with deprecation notice
• update documentation to specify deprecation notice
• create a new issue to actually remove this code once deprecation period is over (mark it this way: https://github.com/kedacore/keda/issues?q=is%3Aissue+is%3Aopen+sort%3Aupdated-desc+label%3Abreaking-change)
• update deprecation notice in the Changelog
• create discussion for the deprecation like these: https://github.com/kedacore/keda/discussions/categories/deprecations

anything else @kedacore/keda-core-contributors ?

[JorTurFer]

That's the process!
I'll keep all the issues for newcomers :)

[yardenshoham]

Some more scalers to add to the task list (based on searching metricName in v2.10 docs):

• aws-cloudwatch
• azure-log-analytics
• azure-monitor
• azure-storage-blob
• huawei-cloudeye
• influxdb
• mongodb
• mssql
• postgresql
• rabbitmq-queue

[tomkerkhove]

@OsamaNabih More on that in our deprecation policy - https://github.com/kedacore/governance/blob/main/DEPRECATIONS.md

[JorTurFer]

Some more scalers to add to the task list (based on searching metricName in v2.10 docs):

• aws-cloudwatch
• azure-log-analytics
• azure-monitor
• azure-storage-blob
• huawei-cloudeye
• influxdb
• mongodb
• mssql
• postgresql
• rabbitmq-queue

Be careful, in that list there are some scalers which really need metricName, for exampleaws-cloudwatch scaler needs the value for the upstream, we need to check if they really need the value or it's only used for the internal metric name

[yardenshoham]

@JorTurFer

#4235 will complete the tasks "code change to print warning message with deprecation notice" and "update deprecation notice in the Changelog".

kedacore/keda-docs#1074 will complete "update documentation to specify deprecation notice...".

[yardenshoham]

I was hoping to complete this today by opening a discussion but I don't have permissions to open a deprecation category discussion

[JorTurFer]

Should we create the discussion now or maybe as part of the release @kedacore/keda-core-contributors ? I guess that we should open it during the release, because maybe the new way of doing the things isn't release yet (not in this specific case)

[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.

[yardenshoham]

I think this is done

[dttung2905]

Hello @JorTurFer, just want to double check whether we still need to deprecate azure-monitor scaler? I see this scaler in the list you mention at the start of this PR but so far I don't see the FIXME tag in azure-monitor code like

keda/pkg/scalers/azure_monitor_scaler.go

Line 223 in ee81112

Name: GenerateMetricNameWithIndex(s.metadata.scalerIndex, kedautil.NormalizeString(fmt.Sprintf("azure-monitor-%s", s.metadata.azureMonitorInfo.Name))),

However, I don't see any default option to use internally if metricName is not specified in trigger metadata though. Is this field metricName still needed ?

keda/pkg/scalers/azure_monitor_scaler.go

Lines 120 to 124 in ee81112

	if val, ok := config.TriggerMetadata[azureMonitorMetricName]; ok && val != "" {
	meta.azureMonitorInfo.Name = val
	} else {
	return nil, fmt.Errorf("no metricName given")
	}

If it is still needed, I can make changes together in this PR too #4806

[JorTurFer]

Nice catch @dttung2905
I have checked the code, and azure_monitor scaler uses metricName for the request, so it's correctly added in the scaler and we don't have to remove it