Ratelimit interceptor #181

ceshihao · 2018-12-13T05:44:22Z

Add ratelimit interceptor, and implement a token bucket limiter using github.com/juju/ratelimit

Fix #134

andyxning · 2018-12-13T13:29:20Z

/sub

jsindy · 2019-03-18T00:10:33Z

Why are checkdocs failing?

codecov-io · 2019-03-18T02:12:42Z

Codecov Report

Merging #181 into master will increase coverage by 0.19%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #181      +/-   ##
==========================================
+ Coverage   73.09%   73.28%   +0.19%     
==========================================
  Files          36       37       +1     
  Lines        1349     1359      +10     
==========================================
+ Hits          986      996      +10     
  Misses        314      314              
  Partials       49       49

Impacted Files	Coverage Δ
ratelimit/ratelimit.go	`100% <100%> (ø)`

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 cfaf568...821b156. Read the comment docs.

ceshihao · 2019-03-18T02:14:27Z

I rebase the changes on latest master.
CI pass :)

makkalot · 2019-03-27T10:54:49Z

Any plans to merge this ?

EngHabu · 2019-05-20T18:16:22Z

ping :) @domgreen ?

domgreen

This implementation feels very focused on the specific implementation of the github.com/juju/ratelimit token package.
I think that if we are to have a rate limiting interceptor we should look to have it much more generic so that other implementations can easily be added to the library. Maybe add a second implementation to tease out the details?

domgreen · 2019-05-20T18:29:34Z

ratelimit/DOC.md

+### Server Side Ratelimit Middleware
+It allows to use your own rate limiter (e.g. token bucket, leaky bucket, etc.) to do grpc rate limit.
+
+`ratelimit/tokenbucket`provides an implementation based on  token bucket `github.com/juju/ratelimit`.


double space on token

domgreen · 2019-05-20T18:31:12Z

ratelimit/DOC.md

+`grpc_ratelimit` a generic server-side ratelimit middleware for gRPC.
+
+### Server Side Ratelimit Middleware
+It allows to use your own rate limiter (e.g. token bucket, leaky bucket, etc.) to do grpc rate limit.


Can we tidy up this sentence?

domgreen · 2019-05-20T18:32:36Z

ratelimit/doc.go

+
+Please see examples for simple examples of use.
+*/
+package grpc_ratelimit


let's not continue this anti-pattern grpc_ratelimit > ratelimit

domgreen · 2019-05-20T18:33:05Z

ratelimit/doc.go

@@ -0,0 +1,15 @@
+// Copyright 2018 Zheng Dayu. All Rights Reserved.


please remove :)

domgreen · 2019-05-20T18:36:31Z

ratelimit/examples_test.go

+)
+
+// Simple example of server initialization code.
+func Example_initialization() {


Can this just be Example?
I'm really not keen on the way examples are done in this project but think this at least follows the current pattern.

domgreen · 2019-05-20T18:56:37Z

ratelimit/ratelimit.go

+	}
+}
+
+// StreamServerInterceptor returns a new stream server interceptors that performs request rate limit.


// StreamServerInterceptor returns a new stream server interceptor that performs rate limiting on the request.

domgreen · 2019-05-20T18:56:53Z

ratelimit/ratelimit.go

+		if ratelimiter.Wait() {
+			return handler(srv, stream)
+		}
+		return status.Errorf(codes.ResourceExhausted, "%s is rejected by grpc_ratelimit middleare, please retry later.", info.FullMethod)


again invert this logic to have the happy path non inverted

domgreen · 2019-05-20T18:58:10Z

ratelimit/ratelimit_test.go

+	}
+	var ctx context.Context
+	var req interface{}
+	var info *grpc.UnaryServerInfo


for the onces that can define them in a var block

var ( ... ... )

domgreen · 2019-05-20T18:58:59Z

ratelimit/ratelimit_test.go

+
+const errMsgFake = "fake error"
+
+func TestEmptyUnaryServerInterceptor(t *testing.T) {


TestUnaryServerInterceptor_EmptyCall

domgreen · 2019-05-20T18:59:06Z

ratelimit/ratelimit_test.go

+	assert.EqualError(t, err, errMsgFake)
+}
+
+func TestRateLimitUnaryServerInterceptor(t *testing.T) {


domgreen · 2019-05-20T19:12:20Z

@ceshihao sorry I didnt pick this up earlier lots of other stuff going on with the world of work and open source so great to see some more people adding keeping the project going 👍 and @EngHabu Thanks for the ping.

An alternative way to do this could be to pass in the interceptor to the middleware eg.

ratelimit.UnaryServerInterceptor(WithRateLimiter(limiter))

and limiter just conforms to the limiter interface with a Limit method so that people can pass any type of limiter and the grpc_middleware does not have to depend on all the custom providers of that logic.

Thoughts?

ceshihao · 2019-05-21T11:33:59Z

@domgreen Thanks for review. I refactor the codes according to your comments.

I change the Limiter interface to

type Limiter interface {
	Limit() bool
}

It makes everyone can pass his own limiter to the interceptor.

domgreen

Thanks for the updates ... I think this is getting there the main thing I want us to try and move away from is taking dependencies on all rate limit packages so if we can pull that outside and instead allow the user to specify their own we can simplify the code massively.
Thanks again. 👍

domgreen · 2019-05-22T09:17:05Z

ratelimit/examples_test.go

@@ -0,0 +1,33 @@
+// Copyright 2018 Zheng Dayu. All Rights Reserved.


please remove

domgreen · 2019-05-22T09:18:08Z

ratelimit/options.go

@@ -0,0 +1,16 @@
+// See LICENSE for licensing terms.


may as well remove all of this and let people view the LICENSE file ... when i get time will clean the rest of the codebase

domgreen · 2019-05-22T09:18:40Z

ratelimit/options.go

+
+type Option func(*rateLimiter)
+
+// WithRateLimiter customizes your limiter in the middleware


can this be a full sentence ... how does it customize? It does give much info to the consumer.

domgreen · 2019-05-22T09:19:04Z

ratelimit/ratelimit.go

+	}
+}
+
+// UnaryServerInterceptor returns a new unary server interceptors that performs request rate limit.


limit > limiting

domgreen · 2019-05-22T09:19:45Z

ratelimit/ratelimit.go

+	for _, opt := range opts {
+		opt(ratelimiter)
+	}
+	fmt.Println(ratelimiter.limiter.Limit())


remove Println

domgreen · 2019-05-22T09:19:54Z

ratelimit/ratelimit.go

+	}
+	fmt.Println(ratelimiter.limiter.Limit())
+	return func(srv interface{}, stream grpc.ServerStream, info *grpc.StreamServerInfo, handler grpc.StreamHandler) error {
+		fmt.Println(ratelimiter.limiter.Limit())


domgreen · 2019-05-22T09:20:13Z

ratelimit/tokenbucket/tokenbucket.go

@@ -0,0 +1,28 @@
+// Copyright 2018 Zheng Dayu. All Rights Reserved.
+// See LICENSE for licensing terms.


domgreen · 2019-05-22T09:22:01Z

ratelimit/ratelimit.go

+	fmt.Println(ratelimiter.limiter.Limit())
+	for _, opt := range opts {
+		opt(ratelimiter)
+	}


I think we could get rid of the opts here ... from what i can see there is only a single opt ... the ratelimiter that the user is passing in.
Lets change it to be something like:

func UnaryServerInterceptor(limiter Limiter) grpc.UnaryServerInterceptor {

domgreen · 2019-05-22T09:23:44Z

ratelimit/tokenbucket/tokenbucket_test.go

@@ -0,0 +1,22 @@
+package tokenbucket


If we are getting the end user to pass in the rate limiter we can remove the whole tokenbucket package from this codebase and not depoend on juju ourselves.
The user would then wrap it using the Limiter interface so that we allow them to use any rate limit algorithm.

I think the package can be kept in order to give user an easy implementation of Limiter.

And also user can implement his own limiter if it is not enough.

we can give an example without needing to import the dependency, the thinking behind passing in the rate limiter is so that we do not have to maintain the rate limiters but just wrap them to meet the Limiter interface that has been defined.

👌remove it from repo

domgreen · 2019-05-22T09:24:32Z

ratelimit/ratelimit.go

+)
+
+type Limiter interface {
+	Limit() bool


Add documentation on the the type and func :)

domgreen · 2019-05-22T09:27:14Z

Just done a round of review @andyxning @makkalot @EngHabu feel free to add your own comments. 👍

domgreen

Just a small amount of clean up needed

domgreen · 2019-05-29T08:35:36Z

ratelimit/doc.go

+
+It allows to do grpc rate limit by your own rate limiter (e.g. token bucket, leaky bucket, etc.)
+
+`ratelimit/tokenbucket`provides an implementation based on token bucket `github.com/juju/ratelimit`.


Can we update ... I dont think this is true anymore :)

domgreen · 2019-05-29T08:36:42Z

ratelimit/examples_test.go

+import (
+	"time"
+
+	"github.com/ceshihao/ratelimiter/tokenbucket"


This is no longer in the project.

Tio do the example can we create a test rate limiter that meets the interface 👍

Do you mean to add a fake limiter (e.g. allwaysPassLimiter) in the example instead of the one based on juju/ratelimit?

yep, lets just add a fake that returns true (or get creative) but one that is not exported ... dont really want the dep just for an example.

RafalKorepta mentioned this pull request Mar 11, 2019

Develop RafalKorepta/most-popular-committer#1

Open

ceshihao added 2 commits March 18, 2019 10:09

grpc ratelimit interceptor

2dbe578

refine ratelimit inteceptor options

5f7557b

ceshihao force-pushed the ratelimit_interceptor branch from 90223d8 to 5f7557b Compare March 18, 2019 02:09

add ratelimit interceptor unit test

6a919f1

domgreen self-requested a review May 20, 2019 18:28

domgreen suggested changes May 20, 2019

View reviewed changes

refactor ratelimit interceptor according to comments

4b75c00

ceshihao force-pushed the ratelimit_interceptor branch from 755c88a to 4b75c00 Compare May 21, 2019 13:48

domgreen suggested changes May 22, 2019

View reviewed changes

ceshihao added 3 commits May 22, 2019 19:03

remove useless options and file header

02ac7e2

remove tokenbucket ratelimiter implementation from repo

199f608

add ratelimit to README.md

b708755

domgreen suggested changes May 29, 2019

View reviewed changes

put an alwaysPassLimiter in example and remove external dependency

821b156

ceshihao force-pushed the ratelimit_interceptor branch from 3b69c1d to 821b156 Compare May 30, 2019 00:04

domgreen approved these changes Jun 3, 2019

View reviewed changes

domgreen merged commit 27f3801 into grpc-ecosystem:master Jun 3, 2019

ceshihao deleted the ratelimit_interceptor branch July 11, 2019 04:10

		@@ -0,0 +1,15 @@
		// Copyright 2018 Zheng Dayu. All Rights Reserved.


		const errMsgFake = "fake error"

		func TestEmptyUnaryServerInterceptor(t *testing.T) {

		@@ -0,0 +1,33 @@
		// Copyright 2018 Zheng Dayu. All Rights Reserved.


		type Option func(*rateLimiter)

		// WithRateLimiter customizes your limiter in the middleware

		@@ -0,0 +1,28 @@
		// Copyright 2018 Zheng Dayu. All Rights Reserved.
		// See LICENSE for licensing terms.


		It allows to do grpc rate limit by your own rate limiter (e.g. token bucket, leaky bucket, etc.)

		`ratelimit/tokenbucket`provides an implementation based on token bucket `github.com/juju/ratelimit`.

Ratelimit interceptor #181

Ratelimit interceptor #181

Conversation

ceshihao commented Dec 13, 2018

andyxning commented Dec 13, 2018

jsindy commented Mar 18, 2019

codecov-io commented Mar 18, 2019 • edited Loading

Codecov Report

ceshihao commented Mar 18, 2019

makkalot commented Mar 27, 2019

EngHabu commented May 20, 2019

domgreen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domgreen commented May 20, 2019

ceshihao commented May 21, 2019

domgreen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domgreen commented May 22, 2019

domgreen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Mar 18, 2019 •

edited

Loading