AWS SQS Scaler: Add Visible + NotVisible messages for scaling considerations

[TyBrown]

This change affects the AWS SQS Scaler and how it considers when to scale down a ScaledObject. Before this change, the scaler only considered AproximateNumberOfMessages, which only includes pending queue messages.

After this change, the scaler will add ApproximateNumberOfMessages and ApproximateNumberOfMessagesNotVisible, the latter of which is all in-flight messages.

Checklist

• Commits are signed with Developer Certificate of Origin (DCO - learn more)
• [N/A] Tests have been added
• [N/A] A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified)
• A PR is opened to update the documentation on (repo) (if applicable)
• Changelog has been updated

Fixes #1663

[zroubalik]

Looking good, thanks for the fix!

Could you please update the Changelog as well? (Improvements section)

I wonder whether we should add a note to the docs about this behavior: https://keda.sh/docs/2.1/scalers/aws-sqs/#trigger-specification

WDYT @tomkerkhove ?

[TyBrown]

@zroubalik I've updated the CHANGELOG as requested. Happy to update docs as well if y'all want, just let me know.

[tomkerkhove]

Looking good, thanks for the fix!

Could you please update the Changelog as well? (Improvements section)

I wonder whether we should add a note to the docs about this behavior: keda.sh/docs/2.1/scalers/aws-sqs/#trigger-specification

WDYT @tomkerkhove ?

Would be good to just doc it indeed!

[TyBrown]

kedacore/keda-docs#398 is opened for Doc updates.

[zroubalik]

LGTM, thanks!

[sdanzan]

This should be documented as a breaking change. Counting the in flight messages as part of the queue length changes the behaviour and makes it less intuitive to select a proper scale up threshold in case of systems with a sustained throughput with occasional bursts. If my goal is to maintain a close to zero queue length at all time, previously it would be just a matter of selecting a random non zero high enough threshold to trigger a scale up. This threshold could be pretty much independent of the number of pods I need in a normal situation (and thus could be chosen equal across multiple similar systems), this is no longer the case with this change.

[yaronya]

This should have been configurable IMHO.
For my use case, it might take some time for my pod to delete the message from the queue but in the meantime KEDA spawns another redundant pod because it also treats in-flight messages now.

I can work around it by changing the polling rate but that increases the delay for this whole batch, which isn't cool.

[pjaak]

I agree with the above. We have some messages which can take like 30mins + to process so therefore Keda adds unnecessary pods.

[LiamStorkey]

I agree with the above. This now causes resource redundancy with pods being told to "stay up" while there is no work for them to do. Can we add in a configurable flag for this ?

[zroubalik]

OK, I haven't been aware of such usecase. Anybody willing to contribute the configuration option?

[pjaak]

Happy to take a look with @LiamStorkey!

[pimbeenes]

@pjaak Did you ever get to implementing this ?