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

Add support for scaling by Google Pub/Sub subscription publish rate #5070

Closed
mbrancato opened this issue Oct 11, 2023 · 11 comments · Fixed by #5246
Closed

Add support for scaling by Google Pub/Sub subscription publish rate #5070

mbrancato opened this issue Oct 11, 2023 · 11 comments · Fixed by #5246
Assignees
Labels
good first issue Good for newcomers help wanted Looking for support from community scaler

Comments

@mbrancato
Copy link

Proposal

Scaling by unacknowledged messages and oldest message are good, reactive scaling metrics. If a queue is behind, both of these are good for responding to how much is currently in the queue. They are, however, reactive and likely can lead to constant scaling up and down to keep up with consistent message volumes.

It would be nice if there was a trigger based on the rate of incoming messages (pubsub.googleapis.com/topic/message_sizes) to keep up with current rate, and use a reactive metric like unacknowledged messages to scale up when things get behind.

Scaler Source

Google Pub/Sub incoming messages

Scaling Mechanics

Scale based on metric

Authentication Source

GCP

Anything else?

No response

@JorTurFer
Copy link
Member

Hello!
It's a nice idea, but I think we can just add it as another flavor of the current scaler. Does GCP expose the publishing rate metric?
BTW, are you willing to contribute with the feature?

@JorTurFer JorTurFer added help wanted Looking for support from community good first issue Good for newcomers and removed needs-discussion labels Oct 15, 2023
@kevinmingtarja
Copy link
Contributor

kevinmingtarja commented Nov 10, 2023

Hey, I'm interested in taking on this feature!

@kevinmingtarja
Copy link
Contributor

I see that there's already pkg/scalers/gcp_pubsub_scaler.go for Google Pub/Sub, so I'm guessing this should be a good place to start, along with other scalers in this package.

But please let me know if you have any additional pointer, thank you!

@zroubalik
Copy link
Member

Thanks @kevinmingtarja, yeah, that is a good starting point. Here are docs for contributing a new scaler, you might find something valuable there.

@kevinmingtarja
Copy link
Contributor

Got it, thanks for the pointers! I'll get started soon.

@kevinmingtarja
Copy link
Contributor

Hi @zroubalik @JorTurFer, I did a bit of reading and have a question regarding how we should approach this.

Right now for the GCP Pub/Sub scaler, the level of granularity is subscriptions, so as part of the metadata users supply subscriptionName and mode (which can be any metrics that starts with subscription/) as inputs.

But the proposal here is to use the metric topic/message_sizes, which is a different level of granularity than subscription/, and thus not supported right now. The relationship between the two is that a topic can have multiple subscriptions, but a given subscription belongs to a single topic.

With this in mind, I think there are two options at least:

  1. Extend the existing scaler. Perhaps we can have a new input topicName, and make one of subscriptionName | topicName required. Based on which of the two has a non nil value, we're able to figure out whether the mode will be a subscription or a topic metric, so we can prefix the mode accordingly when looking up the metric via the GCP API.
  2. Create a new scaler, so we have one scaler working at the level of subscriptions (the existing one), and a new one for topics.

I'm kind of leaning towards option (1), since the API change is very limited and non-breaking, so by default it's backwards compatible as well. But I wanted to get a second opinion before starting to code. Thank you!

@JorTurFer
Copy link
Member

I'm kind of leaning towards option (1), since the API change is very limited and non-breaking, so by default it's backwards compatible as well. But I wanted to get a second opinion before starting to code. Thank you!

I also think that the option 1 is the best if it's not a breaking change

@kevinmingtarja
Copy link
Contributor

Got it, thanks for the input Jorge

@zroubalik
Copy link
Member

I agree with that, thanks for the investigation.

@kevinmingtarja
Copy link
Contributor

kevinmingtarja commented Nov 25, 2023

After starting to code, I think I realized the current code assumes that the metric/mode is a single value, and doesn't handle the case if it's a distribution [1], like for topic/message_sizes.

The main difference I can see is that for distribution, we need to specify an aggregation like sum, mean, p99, etc (example below) in order to get a single value. So I'll address this as well in my changes.

Screen Shot 2023-11-26 at 03 54 49

References:
[1] GCP value types: https://cloud.google.com/monitoring/api/v3/kinds-and-types

@kevinmingtarja
Copy link
Contributor

Hi @mbrancato, just a heads up, once the PR for this gets in on the next release, you will be able to do what you proposed with the following params with the GCP PubSub scaler!

apiVersion: keda.sh/v1alpha1
kind: ScaledObject
...
spec:
  ...
  triggers:
  - type: gcp-pubsub
    metadata:
      mode: "MessageSizes"
      aggregation: "count"
      topicName: "mytopic"
      ...

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 scaler
Projects
Status: Ready To Ship
Development

Successfully merging a pull request may close this issue.

4 participants