Refactor scalers parsing to unify them

[JorTurFer]

Proposal

Currently, each scaler parses the configuration using its own way, some of them use helpers for reading envs, other don't, some use one helper, others use their own helpers... There is a lot of fragmentation in scalers code and we should unify it

Use-Case

No response

Is this a feature you are interested in implementing yourself?

Maybe

Anything else?

No response

[dttung2905]

I take a look at some use cases inside parse${SCALER_NAME}Metadata method and its true that the way we parse the config varies from scaler to scaler. I also noticed we have a helper method getParameterFromConfig in azure_log_analytics_scaler.go that does exactly this thing i.e. get the string value from either AuthParams or TriggerMetadata and throw out error if we can't find anything. https://github.com/kedacore/keda/blob/faf8c9a426e6b2163dd40bc3e6b3711cfe6fda70/pkg/scalers/azure_log_analytics_scaler.go#L246-L255

My proposal is to move this helper function to something like pkg/scalers/helper.go ( not sure about the exact path though but it should be near to scalers. After we get the string value, whatever transformation can be done after that. What do you think about this ?

[zroubalik]

There are also additional things that needs to be considered.

• It should be able to distinguish between optional and mandatory fields
• Some settings have dependencies - eg. if setting1 = foo then setting2 must be set etc. I don't have the specific example here, but I this is needed for some settings a setting around tls/auth

[dttung2905]

Some settings have dependencies - eg. if setting1 = foo then setting2 must be set etc. I don't have the specific example here, but I this is needed for some settings a setting around tls/auth

So for this case we are just going to throw out an error right? Assuming its throwing out an error, I'm thinking of modifying the function getParameterFromConfig to add in an extra arg isOptional of type bool. For case where dependencies are not met, this field should be set as isOptional=false

// getParameterFromConfig gets the parameter from the configs, if checkAuthParams is true
// then AuthParams is also check for the parameter
 func getParameterFromConfig(config *ScalerConfig, parameter string, checkAuthParams bool, isOptional bool) (string, error) {
    if val, ok := config.AuthParams[parameter]; checkAuthParams && ok && val != "" {
        return val, nil
    } else if val, ok := config.TriggerMetadata[parameter]; ok && val != "" {
        return val, nil
    } else if val, ok := config.TriggerMetadata[fmt.Sprintf("%sFromEnv", parameter)]; ok && val != "" {
        return config.ResolvedEnv[config.TriggerMetadata[fmt.Sprintf("%sFromEnv", parameter)]], nil
    }
        // if its an optional field, we can throw out something here, just put empty string as an arbitrary value for now
    if isOptional {
        return "", nil
    }
    return "", fmt.Errorf("error parsing metadata. Details: %s was not found in metadata. Check your ScaledObject configuration", parameter)

wdyt?

[JorTurFer]

I think that this can be useful and I agree with the approach of having a single function like getParameterFromConfig with some config parameters, but I'd be more explicit, something like:

• useMetadata
• useAuthentication
• useResolvedEnv
• isOptional
• default

There are some parameters that we must read only from secure sources 😄 If possible, I'd use generics here to return directly the expected value with the correct type, instead of having to parse outside, so maybe we have to add another parameter for the type

[zroubalik]

Our discussion on Slack: @wozniakjan : just fyi, I started looking into this PR (and also the PRs that merged earlier to get more familiar with the context), I think it's generally on track with the previous development, so I guess the keda maintainers can approve and merge at their convenience. But I have some additional ideas for potential improvements. I personally find the invocations a little hard to read

certGiven, err := getParameterFromConfigV2(config, "cert", false, true, false, true, "", reflect.TypeOf(""))

I will give it a try to see if I can make it a little more readable, I'm thinking something along the lines of

certGiven, err := getParameterFromConfigV2(config, "cert", USE_AUTHENTICATION | OPTIONAL, "")

I'm thinking we can leverage generics to sugar coat the reflections a bit, I guess we it will be hard to get rid of the reflections entirely but maybe we can at least hide them. And then the false, true, false, true - this might actually be a good candidate for masked binary flags, imho those would improve the code readability

@zroubalik: Yeah, that’s true. Maybe we can try to investigate a way with options, where we have sane defaults and specific stuff (auth, …) is enabled by options:

func getParameterFromConfigV2(config *ScalerConfig, parameter string, options ...opt.ConfigOption) {}

...
param, err := getParameterFromConfigV2(config, "cert", opt.UseAuth(), opt.Optional(), opt.TypeString())

This is just a draft ^, there might be better names and usage, but I think it is also viable option to think about

[wozniakjan]

I like the idea of ConfigOptions but I think it's not a blocker for https://github.com/kedacore/keda/pull/5319, we can merge that PR and then as a followup tackle just the options separately.

And I took a stab at replacing reflections from the current implementation of getParameterFromConfigV2 in favour of generics in https://github.com/kedacore/keda/pull/5388 (mainly just https://github.com/kedacore/keda/pull/5388/commits/f03313c69ce29d56066d8eb896d55acb2c67e46e, other commits are from https://github.com/kedacore/keda/pull/5319 so I have an easier life when rebasing once https://github.com/kedacore/keda/pull/5319 merges). I kind of like the result, also enjoy the bonus side effect of adding a type constraint to catch bugs from calling getParameterFromConfigV2 with types not supported by convertToType during compile time.

[dttung2905]

Thanks @wozniakjan for giving an attempt on this. Coincidently, I also drafted a go at this as well, using the functional option pattern. https://github.com/kedacore/keda/pull/5391 . Please help to take a look too :muscle:

[stale[bot]]

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.

[stale[bot]]

This issue has been automatically closed due to inactivity.

[wozniakjan]

@JorTurFer, @dttung2905 I spent some time playing around with a slightly different approach to scalers config parsing and the desired refactor. My efforts are in https://github.com/kedacore/keda/pull/5676. I wanted to achieve a similar experience as json.Unmarshal() and I'd be curious to hear some early input from you. The https://github.com/kedacore/keda/pull/5676 contains only core logic and single scaler refactor, I chose prometheus scaler by a lucky draw, no higher reasoning.

In high-level terms, this is what I managed to get with an example of some fine-tunning, and I think it might be promising:

type MyConfigXYZ struct {
  Flag    bool           `keda:"name=flag,    parsingOrder=triggerMetadata"`
  Addr    string         `keda:"name=addr,    parsingOrder=triggerMetadata;resolvedEnv, optional"`
  Headers map[string]int `keda:"name=headers, parsingOrder=triggerMetadata,             optional, default=d"`
}

sc := &scalerconfig.ScalerConfig{ ... }
conf := MyConfigXYZ{}
if err := sc.TypedConfig(conf); err != nil {
   ...
}

[wozniakjan]

given #5676 merged, I will continue with refactor of the individual scalers

$ for f in pkg/scalers/*; do basename "$f"; done | grep '_scaler.go' | sort | uniq | wc -l
59

It looks like there are 59 configs overall, prometheus already taken care of. @dttung2905 given you already worked on kafka, would you like to try that one?

I will try first five alphabetically this week

$ for f in pkg/scalers/*; do basename "$f"; done | grep '_scaler.go' | sort | uniq | head -n 5
activemq_scaler.go
apache_kafka_scaler.go
arangodb_scaler.go
artemis_scaler.go
aws_cloudwatch_scaler.go

[dttung2905]

@wozniakjan I have created a separate issue to track this. I can start on this kafka later today as well