-
Notifications
You must be signed in to change notification settings - Fork 0
Add ratelimiter framework for grpc server interceptor #73
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #73 +/- ##
============================================
+ Coverage 60.77% 60.93% +0.16%
- Complexity 193 206 +13
============================================
Files 36 41 +5
Lines 752 814 +62
Branches 45 50 +5
============================================
+ Hits 457 496 +39
- Misses 265 287 +22
- Partials 30 31 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@@ -0,0 +1,10 @@ | |||
package org.hypertrace.ratelimiter.grpcutils; | |||
|
|||
public interface RateLimiter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's nothing in this interface tied to gRPC (which is great), but then we go ahead an implement such that it can only be used as a grpc interceptor. Suggest splitting this into a stand alone rate limit package (not in this repo) and then we can provide a utility here to build a gRPC interceptor given a rate limiter instance.
That way we can use it in non grpc use cases too.
// Prevent instantiation | ||
} | ||
|
||
public static Bucket4jRateLimiterFactory bucket4j() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally not public. The point of the abstraction of a provider is so the caller is not concerned with the impl.
@Builder | ||
public class RateLimiterConfiguration { | ||
boolean enabled; | ||
String method; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this? It's not clear from the name
Map<String, String> matchAttributes; | ||
|
||
// Extract attributes from gRPC request | ||
BiFunction<RequestContext, Object, Map<String, String>> attributeExtractor; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following on the earlier comment, we could decouple this from grpc by dropping the attribute parts (because in the general case the caller is only passing something to the rate limiter if it wants to rate limit it) and taking the token cost from the call too.
Then, for the interceptor wrapper you'd have something like
BiPredicate<RequestContext, T> rateLimitMatcher; // Should we rate limit? Alternatively could be combined with permit calculator as a 0 response
BiFunction<RequestContext, T, Object> rateLimitKeyCalculator; // What should they key be? We don't care about the type unless we have a need for human readable keys in which case swap to string
BiFunction<RequestContext, T, Integer> permitCalculator; // Same as before, just renamed/typed
@Builder | ||
public static class RateLimit { | ||
int tokens; | ||
int refreshPeriodSeconds; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - use duration unless the underlying lib only has second granularity.
import java.util.stream.Collectors; | ||
import org.hypertrace.core.grpcutils.context.RequestContext; | ||
|
||
public class RateLimiterInterceptor implements ServerInterceptor { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name should indicate server vs client as we likely want both eventually.
public void onMessage(ReqT message) { | ||
RequestContext requestContext = RequestContext.fromMetadata(headers); | ||
for (RateLimiterConfiguration config : rateLimitConfigs) { | ||
if (!config.getMethod().equals(method)) continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing braces here + below. Even for single statement if
clauses our code style is to include braces.
public void onMessage(ReqT message) { | ||
RequestContext requestContext = RequestContext.fromMetadata(headers); | ||
for (RateLimiterConfiguration config : rateLimitConfigs) { | ||
if (!config.getMethod().equals(method)) continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this also answers my earlier question about what method
is and it's the name being used for string comparison. Then, we use that to do an unchecked cast (well technically we make the rate limit config author do the unchecked cast by typing it as Object
) of the message. Instead, if you accept the method descriptor itself in the config rather than just it's name you can use that directly and get type safety as it will specify your type parameters for the other functions.
} | ||
|
||
private boolean matches(Map<String, String> match, Map<String, String> actual) { | ||
return match.entrySet().stream() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we adjust the config as described in an earlier comment this would go away in favor of an explicit predicate.
import org.hypertrace.ratelimiter.grpcutils.RateLimiterConfiguration; | ||
import org.hypertrace.ratelimiter.grpcutils.RateLimiterFactory; | ||
|
||
public class Bucket4jRateLimiterFactory implements RateLimiterFactory { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't be public, must be a singleton
Description
Add grpc rate limiter utils which contains grpc server interceptor helps in rate limiting grpc methods based on configuration.
Example configuration should be defined like