Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

metricName shouldn't be required and should be deprecated for Prometheus Scaler #4230

Closed
JorTurFer opened this issue Feb 12, 2023 · 4 comments
Labels
good first issue Good for newcomers help wanted Looking for support from community

Comments

@JorTurFer
Copy link
Member

JorTurFer commented Feb 12, 2023

Proposal

Currently, Prometheus Scaler requires 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 favor of self-generated internal name

https://github.com/kedacore/keda/blob/main/pkg/scalers/prometheus_scaler.go#L128-L132

@JorTurFer JorTurFer added help wanted Looking for support from community good first issue Good for newcomers needs-discussion feature-request All issues for new features that have not been committed to and removed needs-discussion feature-request All issues for new features that have not been committed to labels Feb 12, 2023
@OsamaNabih
Copy link

OsamaNabih commented Feb 15, 2023

Hey there,
I'm new here and think this might be a good issue to start with.
Could you elaborate more on the self-generation of the internal name? Is there code that does already, or will it be implemented in the PR? Does it follow a specific format?

Here's my progress so far, as noted by the comments at:
#4227
#4220
#3596 (comment)

--- a/pkg/scalers/prometheus_scaler.go
+++ b/pkg/scalers/prometheus_scaler.go
@@ -82,7 +82,7 @@ func NewPrometheusScaler(config *ScalerConfig) (Scaler, error) {  

        logger := InitializeLogger(config, "prometheus_scaler")

-       meta, err := parsePrometheusMetadata(config)
+       meta, err := parsePrometheusMetadata(config, logger)
        if err != nil {
                return nil, fmt.Errorf("error parsing prometheus metadata: %w", err)
        }
@@ -110,7 +110,7 @@ func NewPrometheusScaler(config *ScalerConfig) (Scaler, error) {
        }, nil
 }

-func parsePrometheusMetadata(config *ScalerConfig) (meta *prometheusMetadata, err error) {
+func parsePrometheusMetadata(config *ScalerConfig, logger logr.Logger) (meta *prometheusMetadata, err error) {
        meta = &prometheusMetadata{}

        if val, ok := config.TriggerMetadata[promServerAddress]; ok && val != "" {
@@ -148,6 +148,7 @@ func parsePrometheusMetadata(config *ScalerConfig) (meta *prometheusMetadata, er

        if val, ok := config.TriggerMetadata[promMetricName]; ok && val != "" {
                meta.metricName = val
+               logger.Info("WARNING: the field 'metricName' has been deprecated and will be removed in v2.12")
        }

        if val, ok := config.TriggerMetadata[promNamespace]; ok && val != "" {```

@JorTurFer
Copy link
Member Author

JorTurFer commented Feb 16, 2023

Hi,
I'm not totally sure, but I think that there is another PR in progress that includes this
#4235

@OsamaNabih
Copy link

I see.

@zroubalik
Copy link
Member

will be handled by #4220

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Looking for support from community
Projects
Archived in project
Development

No branches or pull requests

3 participants