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

feat: add an optional burstable rate limiter #1924

Merged
merged 4 commits into from
Jan 27, 2025
Merged

Conversation

cstockton
Copy link
Contributor

The existing rate limiter was moved to a separate package and renamed to IntervalLimiter. Added BurstLimiter which is a wrapper around the "golang.org/x/time/rate" package.

The conf.Rate type now has a private typ field that indicates if it is a "interval" or "burst" rate limiter. If the config value is in the form of "<burst>/<rate>" we set it to "burst", otherwise "interval". The conf.Rate.GetRateType() method is then called from the ratelimit.New function to determine the underlying type of ratelimit.Limiter it returns.

Then changed api.NewLimiterOptions to call ratelimit.New instead of creating a specific type of rate limiter.

The existing rate limiter was moved to a separate package and
renamed to IntervalLimiter. Added BurstLimiter which is a wrapper
around the "golang.org/x/time/rate" package.

The conf.Rate type now has a private `typ` field that indicates
if it is a "interval" or "burst" rate limiter. If the config value
is in the form of "<burst>/<rate>" we set it to "burst", otherwise
"interval". The conf.Rate.GetRateType() method is then called from
the ratelimit.New package to determine the underlying type of
`ratelimit.Limiter` returned from `ratelimit.New`.

Finally we changed `api.NewLimiterOptions` to call `ratelimit.New`
instead of creating a specific type of rate limiter.
@cstockton cstockton requested a review from a team as a code owner January 22, 2025 22:08
@coveralls
Copy link

coveralls commented Jan 22, 2025

Pull Request Test Coverage Report for Build 12995163660

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 65 of 65 (100.0%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 65.58%

Totals Coverage Status
Change from base Build 12917117821: 0.1%
Covered Lines: 9860
Relevant Lines: 15035

💛 - Coveralls

@cstockton cstockton added the enhancement New feature or request label Jan 23, 2025
internal/conf/rate.go Show resolved Hide resolved
internal/ratelimit/burst_test.go Outdated Show resolved Hide resolved
internal/ratelimit/interval.go Show resolved Hide resolved
Chris Stockton added 2 commits January 27, 2025 08:47
We have a special case check for rate limits of 0 in some areas
of the code. Eventually I would like to remove those so I'm
going to set the rate limit of 0 as a valid rate limit which
allows no events.
@cstockton cstockton merged commit 1f06f58 into master Jan 27, 2025
3 checks passed
@cstockton cstockton deleted the cs/rate-limit-refactor branch January 27, 2025 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants