Deprecate explicitly setting metricName for all scalers

[yardenshoham]

In v2.12 this setting will be internalized and should not be set by users

A log message was added

Checklist

• I have verified that my change is according to the deprecations & breaking changes policy
• Changelog has been updated and is aligned with our changelog requirements
• A PR is opened to update the documentation on (repo)
• Commits are signed with Developer Certificate of Origin (DCO - learn more)

Docs PR kedacore/keda-docs#1074
Relates to #4220

[JorTurFer]

This is a good idea, but sadly we cannot do it at this level because there are some scalers where metricName is a valid value used in the scaler (like aws cloudwatch). We need to do it one by one

[zroubalik]

This is a good idea, but sadly we cannot do it at this level because there are some scalers where metricName is a valid value used in the scaler (like aws cloudwatch). We need to do it one by one

If aws cloudwatch is the only one, then we can probably add the exception to the code added in this PR. Or is there any other scaler where metricName is valid?

[JorTurFer]

I'm not 100% sure, I haven't had time to check it in depth yet

[zroubalik]

@yardenshoham could you please check the source code of scalers that uses metricName to verify that it is being used only for internal metrics representation?

For example:

keda/pkg/scalers/prometheus_scaler.go

Line 48 in 72defc2

metricName string

vs

keda/pkg/scalers/aws_cloudwatch_scaler.go

Line 36 in 72defc2

metricsName string

[yardenshoham]

Will do, tonight

[JorTurFer]

Also remember that this doesn't solve all the cases. I mean, this adds the warning notification to all at once (which is nice), but scalers like prometheus requires metricName, they need to be updated to make it optional too

[yardenshoham]

Yes, I didn't claim this would resolve the issue

[JorTurFer]

Yeah, I know it, I said just to share the point. Maybe we can fix them as part of this PR if there are only a few. I mean, if only 1-3 scalers require changes, we could do it in one shoot
Wdyt?

[yardenshoham]

I think it's a good idea. For prometheus, how about I normalize the query and set that as queryName?

[JorTurFer]

I think it's a good idea. For prometheus, how about I normalize the query and set that as queryName?

I think it's not necessary, adding a fixed value like 'prometheus' is more than enough as this is an internal value. Something like
If given value, warning and use it
If not given, use something like 'prometheus'

And please, add a comment in scalers where the metricName should be removed or write them somewhere to have them tracked for removing them 🙏

[yardenshoham]

So meta.metricName need not be unique?

[JorTurFer]

So meta.metricName need not be unique?

No, it isn't required since we introduced a dynamic prefix. Each metric has sX- as a prefix where X is the scaler index, so even if you use the same name for all the cases, the metric name will be unique in the scaled object because it's based on the scaler index

[yardenshoham]

Two scalers stand out:

• aws-cloudwatch
• huawei-cloudeye

How should I handle them?

[JorTurFer]

How should I handle them?

As they are just 2, maybe we can add them in the if to ignore them for the warning. WDYT?

[yardenshoham]

Sounds good, done

[JorTurFer]

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

[JorTurFer]

LGTM! Thanks for the improvement ❤️
Could you update docs as well?

[JorTurFer]

PTAL @zroubalik

[yardenshoham]

Could you update docs as well?

Sure, I'll get started as soon as this PR lands

[yardenshoham]

Should I keep updating this PR?

[zroubalik]

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

[zroubalik]

LGTM, great job!

@yardenshoham could you please open a docs PR. We require to have approved docs PR before merging code PR. Thanks a lot

[zroubalik]

Should I keep updating this PR?

It is fine as it now, thanks! :)

[yardenshoham]

Great! Docs PR will be ready tomorrow today

[yardenshoham]

Docs PR kedacore/keda-docs#1074

[zroubalik]

Docs PR kedacore/keda-docs#1074

awesome, could you please track tasks in the issue #4220 ? In the issue describtion there's TODO list

[yardenshoham]

Apart from creating the discussion, what are the other action items? Not sure what needs to be done in "update documentation to specify deprecation notice"

[zroubalik]

Apart from creating the discussion, what are the other action items? Not sure what needs to be done in "update documentation to specify deprecation notice"

exactly this PR you did: kedacore/keda-docs#1074 :)

Just link it in the tasks in the issue and mark this item as done :)

[yardenshoham]

I commented #4220 (comment)

[zroubalik]

I commented #4220 (comment)

I mean the todo list, but I've just realized you probably can't modify that, right?

[yardenshoham]

Correct

[yardenshoham]

Is there any more work to get this PR merged? It has 2 approvals

[zroubalik]

Thanks a lot for the contribution @yardenshoham !