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.

Sign up for GitHub

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

Jump to bottom

Proposal: Add explicit unlimited rate limits #241

Open
thao-wish opened this issue Mar 31, 2021 · 7 comments
Open

Proposal: Add explicit unlimited rate limits #241

thao-wish opened this issue Mar 31, 2021 · 7 comments
Labels
help wanted

Comments

@thao-wish
Copy link
Contributor

Currently we can achieve implicit unlimited rate limits by not specifying a rate limit in the configuration. However, we'll lose the metrics (total_hits, within_limits) for those rate limits. The proposal is to add an explicit unlimited rate limit. An example config file could look like this:

domain: mongo_cps
descriptors:
  - key: database
    value: users
    rate_limit:
      unit: second
      requests_per_unit: unlimited

We can skip the trip to redis/memcached if unlimited rate limits are requested. Please let me know if the team is open to this request or discuss any alternatives. The main concern is to be able to get the metrics for the unlimited rate limits.
@mattklein123
Copy link
Member

IMO I would rather just implement #218 which I think covers this case?

@thao-wish
Copy link
Contributor Author

IMO I would rather just implement #218 which I think covers this case?

I think it's a bit different. Please correct me if I'm wrong.

Test limits are processed in the same way as real limits, so they still have to go through redis/memcached. Currently the unlimited rate limits do not go to redis/memcached, so we want the new unlimited rate limits to be the same. It's also good for performance.

@mattklein123
Copy link
Member

Yeah fair enough. I agree we can look at this independently.

@lmajercak-wish
Copy link
Contributor

Hey @mattklein123, I would like to pick this up.

I've already looked into this briefly and it seems like the biggest issue here will be deciding what to return for rate limits which are set to 'unlimited'. Specifically, we have the uint32 requests_per_unit and uint32 limit_remaining fields.

Any suggestions on what we do with these? If we omitted them, then I think there isn't a difference between a response for an unlimited ratelimit and one that does not exist, which might cause confusion. Alternatively, I guess we could return something like uint32 max value, but that does not seem ideal either.

@lmajercak-wish
Copy link
Contributor

I just realized that, if the limit is set to 0, these two fields are omitted. I guess we could do the same for unlimited ones then?

@lmajercak-wish lmajercak-wish mentioned this issue Jun 9, 2021
Merged
@lmajercak-wish
Copy link
Contributor

Opened #261

@zakhenry
Copy link
Contributor

This appears to be implemented now, can this issue be closed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
help wanted
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@zakhenry @mattklein123 @lmajercak-wish @thao-wish