document new publishRate trigger

[rwkarg]

Corresponding code change: kedacore/keda#1653

[tomkerkhove]

Just a thought: Do we use this approach, or do we introduce a mode which defines how it should work?

Thoughts on this @zroubalik?

[rwkarg]

I had talked with @coderanger on Slack about this.

queueLength needs to be kept to prevent a breaking change from existing ScaledObject definitions. That leaves the following options:

1. Add a triggerType field to make queueLength really mean publish rate (that seems confusing)
2. Add a generic triggerValue field and a triggerType field (with queue length and publish rate possible values). This would give two ways to set a queue length trigger (queueLength: X or triggerValue: X, triggerType: QUEUE_LENGTH) which is not ideal but prevents breaking changes. Publish rate would have only one option for config (triggerValue: X, triggerType: PUBLISH_RATE)
3. Separate queueLength and publishRate fields that are mutually exclusive.

If this were a completely new scaler then only having only the triggerValue and triggerType fields would probably be the way to go, but we've already got existing uses with the queueLength field configured.

I'm happy to align with another direction if that would be more consistent with how other scalers are set up. The one thing I don't want to do is introduce a change that would break existing users.

[tomkerkhove]

I was just thinking of this so that it's clear what you want to use and simplify the validation of the config without relying on assumptions. I don't like assumptions as it is not always straightforward if you are new to it or not aware of how it works without reading the code/docs carefully.

  metadata:
    mode: QueueDepth
    queueLength: '20' # Optional. Queue length target for HPA. Default: 20 messages
    # Standard config

  metadata:
    mode: MessageRate
    publishRate: '100' # Optional. Publish/sec. target on the queue for HPA. Requires http host/protocol
    # Standard config

That way, you could also use these configs:

  metadata:
    mode: QueueDepth
    # Standard config

  metadata:
    mode: MessageRate
    # Standard config

This allows people to rely on the standard value, but still, define the trigger behavior.

When using both fields, this can be tricky if you are new (and maybe didn't read the docs correctly); but if you use a flag then it is explicit. The only caveat would be, though, that if mode is not provided that it defaults to queue depth.

That said, this was just my idea; I'm happy to go with the current if everyone prefers that.

mode is just an idea, can be triggerMode as well

[coderanger]

Using both fields is impossible, it's disjoint mode. And I'm not sure any default value for publishRate makes sense so I think this is overly verbose.

[rwkarg]

What if we kept queueLength but marked it obsolete until the next major version when it would be removed. Write out a log/event (that nobody will probably read...) to update to using the mode version.

This would leave three usage patterns:

queueLength (obsolete but supported until the next major version)

 metadata:
    queueLength: "10"

mode: QueueDepth (preferred over queueLength)

 metadata:
    mode: "QueueDepth"
    value: "10" # this could still default to 20 when omitted, if desired

mode: MessageRate

 metadata:
    mode: "MessageRate"
    value: "100" # required with `mode: "MessageRate"`

[rwkarg]

Or we could just remove queueLength from the documentation and only surface the mode: variants, but still accept the queueLength config forever as an undocumented back compat. config?

[tomkerkhove]

I like the deprecation suggestion!

[coderanger]

Or we could just remove queueLength from the documentation and only surface the mode: variants, but still accept the queueLength config forever as an undocumented back compat. config?

-1 on undocumented config. Folks may have a running Keda and be trying to understand it, and having undocumented legacy flags makes that much harder. But a clear "legacy, deprecated flags" section is 👍

[rwkarg]

@coderanger I've updated to include the deprecated queueLength in the docs but call it out as deprecated and how to migrate. It is still fully supported with the same behavior, but it is mutually exclusive to the mode/value config.

[tomkerkhove]

LGTM, WDYT @zroubalik @ahmelsayed ?

[tomkerkhove]

Thanks for this nice change btw, @rwkarg!

[zroubalik]

LGTM, just the small nit: #388 (comment)

[tomkerkhove]

Thanks a ton @rwkarg!