Skip to content

[RateLimiter] Added the docs for the new component #14370

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

Merged
merged 1 commit into from
Oct 9, 2020

Conversation

javiereguiluz
Copy link
Member

I'd like to merge this soon, so we can later iterate the docs with the changes done in RateLimiter during the stabilization phase before Symfony 5.2 release. Thanks!

Some questions for @wouterj:

  • Is the strategy config option mandatory? If yes, could we assign a default value to it to lower the learning curve?
  • I'm a bit lost with the "burst" behavior and the "limit" options. I don't fully understand them.
  • What's the purpose of the top level interval option? Is the same as the rate.interval option?

@wouterj
Copy link
Member

wouterj commented Oct 7, 2020

Basically there are 2 "Sets" of configuration:

framework:
  rate_limiter:
    some_window_limited_limiter:
      strategy: fixed_window
      interval: '1 min'
      limit: 5

    some_token_bucket_limiter:
      strategy: token_bucket
      limit: 5
      rate: { interval: '1 min', amount: 1 }

In short explanation:

A fixed window can be represented by "time windows". They start at the first hit and then allow a maximum number of hits (limit) in a window from that start time (interval). After that interval, the number of hits is reset and you can start registering hits again. Etc.

A token bucket can be represented as a real bucket. It's initially filled with limit tokens. Each hit takes a token out of this bucket. When the bucket is empty, hits are rejected. The bucket is filled using rate as fill rate (e.g. 1 token every minute). The bucket's maximum fill capacity is limit.


Just like the docs, the config also needs some iteration I think. I now used the password hashing method of registration, but I'm not too sure. Maybe this is more clean and easier to understand?

framework:
  rate_limiter:
    some_window_limited_limiter:
      fixed_window:
        interval: 1m
        limit: 5

    some_token_bucket_limiter:
      token_bucket:
        limit: 5
        rate: { interval: 1m, amount: 1 }

@javiereguiluz
Copy link
Member Author

Thanks for your comments! It's more clear now, so I've updated the docs.

About the config ... I prefer the current format instead of the other one proposed below.

I still have a question about the interval option. Which are the possible formats/syntax of this parameter? Thanks!

@wouterj
Copy link
Member

wouterj commented Oct 8, 2020

I still have a question about the interval option. Which are the possible formats/syntax of this parameter? Thanks!

This is a bit difficult to explain, but it's everything that PHP's DateTime object supports after +. So +1 minute, +1 min, +5 hours, etc. but then without the + prefix. A formal list can be found here in the unit definition: https://www.php.net/datetime.formats.relative

unit    := (('sec' | 'second' | 'min' | 'minute' | 'hour' | 'day' | 'fortnight' | 'forthnight' | 'month' | 'year') 's'?) | 'weeks' | daytext
daytext := 'weekday', 'weekdays'

Please note that my previous comment was using a wrong syntax - I've updated it accordingly.

Copy link
Member

@wouterj wouterj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the many comments. There have been 2 major refactorings after the main PR was merged, so quite a few things have changed.

@javiereguiluz
Copy link
Member Author

Wouter, thanks for your amazing review 😍 I did all the requested changes!

Copy link
Member

@wouterj wouterj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, second round. Thanks for your quick reactions!

This doc PR has revealed quite some good improvements, I'll follow up with a PR on the code repository.

@javiereguiluz
Copy link
Member Author

Thanks again for the new review. Thanks to your help this doc is coming along nicely 👍

Copy link
Member

@wouterj wouterj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor fix left, but other than that ready to be merged imho!

wouterj added a commit to wouterj/symfony that referenced this pull request Oct 15, 2020
wouterj added a commit to wouterj/symfony that referenced this pull request Oct 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants