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

Feature/local-rate-limiter #1703

Open
wants to merge 31 commits into
base: main
Choose a base branch
from

Conversation

gkatzioura
Copy link

@gkatzioura gkatzioura commented May 3, 2020

Is your feature request related to a problem? Please describe.

This feature adds a Local Rate Limiter using the bucket4j library. Devs who do not want to use the Redis limiter and avoid the IO might find this rate limiter useful.

Describe the solution you'd like
The local rate limiter gives a simple ratelimiter that operates the same way the Redis one does. It can be useful on scenarios of singular rate-limiter or a hot stand-by.

Describe alternatives you've considered
The alternatives considered is to avoid the bucket4j and implement the token bucket algorithm.

Additional context
This rate limiter will work is the Bucket4j library is included and if there is not Redis and Redis Rate Limiter related configuration present.

@spencergibb
Copy link
Member

I'd rather not introduce another dependency. Gateway already depends on resilence4j, would you be willing to rework and use that library since we already manage it? https://github.com/resilience4j/resilience4j#ratelimiter

@gkatzioura
Copy link
Author

gkatzioura commented May 4, 2020

I'd rather not introduce another dependency. Gateway already depends on resilence4j, would you be willing to rework and use that library since we already manage it? https://github.com/resilience4j/resilience4j#ratelimiter

Hi @spencergibb I am more than happy to use the one you mentioned!
I will do this on the same pull request.

@spencergibb
Copy link
Member

perfect

@gkatzioura
Copy link
Author

gkatzioura commented May 10, 2020

Hi @spencergibb ! The rate limiter has been switched to the resilience4j rate limiter. Currently I am working on the testing.
Some notes on the resilience4j rate limiter.
It acts like a fixed window. For example in a 60 secs period if all the tokens are consumed the refill is happening after the 60 seconds have passed.
In the case of bucket4j the token refill is spread through that period.
Also there is no burst capability (I might proceed on a pull request on the resilience4j project and add it).
Due to the above I reckoned it will help the users to give them the choice on specifying the window of the requests using the
configuration variable refreshPeriod

@codecov
Copy link

codecov bot commented May 11, 2020

Codecov Report

Merging #1703 into master will increase coverage by 3.96%.
The diff coverage is 66.95%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1703      +/-   ##
============================================
+ Coverage     74.21%   78.17%   +3.96%     
+ Complexity     1262     1240      -22     
============================================
  Files           166      158       -8     
  Lines          5413     5032     -381     
  Branches        420      401      -19     
============================================
- Hits           4017     3934      -83     
+ Misses         1178      890     -288     
+ Partials        218      208      -10     
Impacted Files Coverage Δ Complexity Δ
...cloud/gateway/config/GatewayAutoConfiguration.java 82.45% <ø> (-0.31%) 69.00 <0.00> (ø)
.../GatewayNoLoadBalancerClientAutoConfiguration.java 76.92% <ø> (ø) 2.00 <0.00> (ø)
...ayReactiveLoadBalancerClientAutoConfiguration.java 100.00% <ø> (+50.00%) 2.00 <0.00> (+1.00)
...ork/cloud/gateway/config/HttpClientProperties.java 58.94% <ø> (+1.66%) 17.00 <0.00> (ø)
...scovery/DiscoveryClientRouteDefinitionLocator.java 80.00% <ø> (-1.40%) 18.00 <0.00> (-2.00)
...overy/GatewayDiscoveryClientAutoConfiguration.java 100.00% <ø> (ø) 4.00 <0.00> (ø)
...ud/gateway/filter/AdaptCachedBodyGlobalFilter.java 94.44% <ø> (ø) 8.00 <0.00> (ø)
...ork/cloud/gateway/filter/GatewayMetricsFilter.java 95.65% <ø> (+7.65%) 11.00 <0.00> (ø)
...teway/filter/ReactiveLoadBalancerClientFilter.java 94.73% <ø> (+2.63%) 14.00 <0.00> (+1.00)
...loud/gateway/filter/WeightCalculatorWebFilter.java 89.52% <ø> (+4.17%) 29.00 <0.00> (-1.00) ⬆️
... and 30 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dea0da5...cc52002. Read the comment docs.

@gkatzioura
Copy link
Author

The related ticket is #88

@spencergibb spencergibb force-pushed the master branch 2 times, most recently from 73d934e to 5bd5791 Compare July 28, 2020 18:26
@korektur
Copy link
Contributor

korektur commented Apr 14, 2021

Hi,

Is this going to be merged or does it needs a rework? I think it can be quite a useful feature.

@gkatzioura
Copy link
Author

gkatzioura commented Apr 14, 2021

@korektur I am available if people want to reboot it.
Also I have implemented a Refill Rate limiter for Ressilience4j In case a more smooth Rate Limiting is needed (I actually had this in mind when implementing).
@spencergibb what do you think?

Throwable exception = exchange
.getAttribute(CIRCUITBREAKER_EXECUTION_EXCEPTION_ATTR);
ServerWebExchange filteredExchange;
if (exception == null) {

Choose a reason for hiding this comment

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

filteredExchange = exception == null ? exchange : addFallbackHeaders(config, exchange, exception);

@mafias
Copy link

mafias commented Sep 2, 2022

Hey,

+1 for this one, it's a pretty useful feature without setting separated component like redis.

@spencergibb is that PR not merged only due to codecov check? Not mentioning current conflicts ofc.

@gkatzioura would you still have a will to finish this PR?

@gkatzioura
Copy link
Author

Hi @mafias
I am available for this. If I see engagement from @spencergibb and team can resume.

@homihq
Copy link

homihq commented Dec 5, 2022

+1
This will be a super useful feature addition. Not all scenarios require distributed rate limiting with Redis. Just protecting individual nodes is good enough most of the time.

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.

None yet

7 participants