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

Way to create different time units? #190

Open
vincentjames501 opened this issue Nov 13, 2020 · 17 comments
Open

Way to create different time units? #190

vincentjames501 opened this issue Nov 13, 2020 · 17 comments

Comments

@vincentjames501
Copy link

We'd like to implement rate limiting such that certain time-periods have time have different limits (i.e. bursting). For example:

Scenario Time-period (sec) Max allowed operations
Send to Conversation 1 5
Send to Conversation 30 60
Send to Conversation 300 100

Right now the configuration is pretty simple with:

rate_limit:
  unit: <second, minute, hour, day>
  requests_per_unit: <uint>

The problem is that I can't adjust the unit to be something like 30 seconds or 5 minutes (or at least I'm not clear how one would set that).

What would folks think about adding some unit multiplier such as:

rate_limit:
  unit_multiplier: <uint | default=1>
  unit: <second, minute, hour, day>
  requests_per_unit: <uint>

This would allow someone to easily express 30 seconds or 5 minutes?

@vincentjames501
Copy link
Author

It also appears I can only get one rate limit to match and be processed. Is there a way to have multiple rate limits be applied to a single route?

                      routes:
                        - match:
                            prefix: /api/
                            headers: 
                              name: :method
                              exact_match: GET
                          route:
                            cluster: api
                            rate_limits:
                              - actions:
                                  - request_headers:
                                      header_name: x-user-id
                                      descriptor_key: default_get_limit_sm
                                  - request_headers:
                                      header_name: x-user-id
                                      descriptor_key: default_get_limit_med
                                  - request_headers:
                                      header_name: x-user-id
                                      descriptor_key: default_get_limit_lg
---
domain: rl
descriptors:
  - key: default_get_limit_sm
    rate_limit:
      unit: second
      requests_per_unit: 30
  - key: default_get_limit_md
    rate_limit:
      unit: minute
      requests_per_unit: 120
  - key: default_get_limit_lg
    rate_limit:
      unit: hour
      requests_per_unit: 1800

It could be neat if instead of rate_limit there was rate_limits which would be an array of rate_limit objects?

@vincentjames501
Copy link
Author

I figured out the multiple rate limits for a given route. I'll post that here incase anyone else runs into the same thing.

                    routes:
                        - match:
                            prefix: /api/
                            headers: 
                              name: :method
                              exact_match: GET
                          route:
                            cluster: api
                            rate_limits:
                              - actions:
                                - generic_key: 
                                    descriptor_value: default_get_sm
                                - request_headers:
                                    header_name: x-user-id
                                    descriptor_key: user_id
                              - actions:
                                  - generic_key: 
                                      descriptor_value: default_get_md
                                  - request_headers:
                                      header_name: x-user-id
                                      descriptor_key: user_id
                              - actions:
                                  - generic_key: 
                                      descriptor_value: default_get_lg
                                  - request_headers:
                                      header_name: x-user-id
                                      descriptor_key: user_id
---
domain: rl
descriptors:
  - key: generic_key
    value: default_get_sm
    descriptors:
      - key: user_id
        rate_limit:
          unit: second
          requests_per_unit: 30
  - key: generic_key
    value: default_get_md
    descriptors:
      - key: user_id
        rate_limit:
          unit: minute
          requests_per_unit: 120
  - key: generic_key
    value: default_get_lg
    descriptors:
      - key: user_id
        rate_limit:
          unit: hour
          requests_per_unit: 1800

@stale
Copy link

stale bot commented Dec 19, 2020

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or other activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Dec 19, 2020
@cesutherland
Copy link

cesutherland commented Jan 13, 2021

We would also benefit from "bursting" where, eg. 30r/3s would support a burst of 30 requests with a sustained traffic of 10 requests/s.

I don't believe there's any option for that today.

@stale stale bot removed the stale label Jan 13, 2021
@github-actions
Copy link

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label Feb 13, 2021
@vincentjames501
Copy link
Author

Can this be tagged with help wanted at least?

@github-actions github-actions bot removed the stale label Feb 13, 2021
@mattklein123
Copy link
Member

Sorry what is the actual request? I think we support multiple limits today. Can bursting be expressed as multiple concurrent rate limits?

@vincentjames501
Copy link
Author

@mattklein123 Yes, this service can represent multiple rate limits for bursting today. The issue is our company has different time windows they want to enforce limits on that don't fit into second, minute, hour, and day units. For example, you can't really make these limits:

X requests over 30 seconds
X requests over 5 minutes
X requests over 30 minutes
X requests over 12 hours
X requests over 7 days

The proposal I made earlier was some sort of unit multiplier such as:

rate_limit:
  unit_multiplier: <uint | default=1>
  unit: <second, minute, hour, day>
  requests_per_unit: <uint>

which would allow you to represent any time period. Does that make sense?

@mattklein123
Copy link
Member

which would allow you to represent any time period. Does that make sense?

Yup OK makes sense. I will mark it help wanted.

@MischaWeerwag
Copy link

I would also like to set multiple units for a single action so:

            descriptors:
              - key: remote_address
                rate_limit:
                  unit: second
                  requests_per_unit: 3
                rate_limit:
                  unit: day
                  requests_per_unit: 100

That is currently (afaik) not possible without duplicating the complete action with a generic key.

@basimons
Copy link

This seems like a very nice feature as just converting your bucket to a 1 second or 1 minute bucket can be undesirable. Since with larger time units you can catch bursts more easily, but the user has to wait longer before reaching the new bucket.

rate_limit:
  unit_multiplier: <uint | default=1>
  unit: <second, minute, hour, day>
  requests_per_unit: <uint>

In my case something like a 10 second bucket would be perfect.

@uzzz
Copy link

uzzz commented May 25, 2023

Hi.

I can work on this improvement if you don't mind

@quyenhoang96
Copy link

I think this feature is extremely important. You can develop it and create PR. I can fork that PR to test for you. Because we really need it right now. Tks you

@eduardosanzb
Copy link

@uzzz @quyenhoang96 did you got any progress on this feature? how could I support you to get this done?

@eduardosanzb
Copy link

I would also like to set multiple units for a single action so:

            descriptors:
              - key: remote_address
                rate_limit:
                  unit: second
                  requests_per_unit: 3
                rate_limit:
                  unit: day
                  requests_per_unit: 100

That is currently (afaik) not possible without duplicating the complete action with a generic key.

@MischaWeerwag how did you achieved this? do you have an example?

@harpunius
Copy link

We would also like to see something like this, has there been any work on this?

@vamsibandarupalli
Copy link

Any update on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants