[common-go] Add keyed gRPC rate limits #9547

csweichel · 2022-04-26T10:20:51Z

Description

This PR adds support for message-keyed gRPC rate limits to common-go.
Using this functionality, we can e.g. add per-user rate limits in ws-manager.

For example:

{
    "rateLimits": {
        "/wsman.WorkspaceManager/StartWorkspace": {
            "bucketSize": 5,
            "refillInterval": "1s",
            "key": "metadata.owner",
            "keyCacheSize": 200
        }
    }
}

would impose a 5 workspaces per second rate limit per workspace owner.

How to test

See unit test
Manually add the config to ws-manager and use loadgen

Release Notes

[ws-manager] Support more fine-grained rate limits

test/go.sum

test/go.mod

csweichel · 2022-04-26T11:40:36Z

/werft run with-clean-slate

👍 started the job as gitpod-build-cw-grpc-rl-fields.4
(with .werft/ from main)

easyCZ · 2022-04-27T07:30:38Z

https://github.com/grpc-ecosystem/go-grpc-middleware offers a rate limit implementation, this may be possibly cheaper to use than building our own.

I've used it in the past and worked well. It has built in support for sharing the rate limits across instances (but can be used without it).

csweichel · 2022-04-27T09:28:50Z

https://github.com/grpc-ecosystem/go-grpc-middleware offers a rate limit implementation, this may be possibly cheaper to use than building our own.

I've used it in the past and worked well. It has built in support for sharing the rate limits across instances (but can be used without it).

We looked at that implementation prior to rolling our own.
AFAIK it does not support

per method rate limiting
"keyed" rate limiting as is introduced in this PR.

We use the exact same rate limiting package as go-grpc-middleware under the hood (basically took inspiration from that package), hence could also support rate limiting across multiple instances.

easyCZ

Looking good, left a couple comments. Could you please also add metrics to expose the following:

Rate limited count, by method
Hit rate on the LRU

Can be in seperate follow-up PRs, but we should be able to observe this before we deploy it to prod.

easyCZ · 2022-04-27T09:56:14Z

components/common-go/grpc/ratelimit.go

 // RatelimitingInterceptor limits how often a gRPC function may be called. If the limit has been
 // exceeded, we'll return resource exhausted.
-type RatelimitingInterceptor map[string]ratelimitedFunction
+type RatelimitingInterceptor map[string]*ratelimitedFunction


The map here should probably be made private. As implemented, updates to the map are not concurrency safe so we shouldn't be exposing it.

easyCZ · 2022-04-27T09:58:46Z

components/common-go/grpc/ratelimit.go

+
+	GlobalLimit *rate.Limiter
+	Key         keyFunc
+	KeyedLimit  *lru.Cache
 }

 // UnaryInterceptor creates a unary interceptor that implements the rate limiting
 func (r RatelimitingInterceptor) UnaryInterceptor() grpc.UnaryServerInterceptor {
 	return func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (resp interface{}, err error) {


Suggested change

return func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (resp interface{}, err error) {

return func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) {

we're not using the named returns

they indicate what's returned though :)

geropl · 2022-04-27T15:45:33Z

components/common-go/grpc/ratelimit.go

+				return nil, err
+			}
+
+			f.KeyedLimit.ContainsOrAdd(key, f.RateLimit.Limiter())


question: this immediately throws creates and trashes a limiter struct on stack AFAIU which could be avoided by not using ContainsOrAdd.

Agreed - not doing it this way requires extra locking because this operation can happen concurrently.

We could also ignore the concurrency for the time being and go with a Peek followed by an Add, running the risk to mis-account for a key during very high call-rate situations.

geropl

Code LGTM, and seems to work. Left minor comments.

csweichel · 2022-04-28T18:06:47Z

Added metrics

easyCZ · 2022-04-28T19:37:00Z

components/common-go/grpc/ratelimit.go

 }

 // NewRatelimitingInterceptor creates a new rate limiting interceptor
 func NewRatelimitingInterceptor(f map[string]RateLimit) RatelimitingInterceptor {
-	funcs := make(map[string]ratelimitedFunction, len(f))
+	callCounter := prometheus.NewCounterVec(prometheus.CounterOpts{


I believe the metrics here also need to be registered, to be picked up by prometheus, otherwise they are just local counters which won't show up on /metrics endpoint.

See https://pkg.go.dev/github.com/prometheus/client_golang/prometheus#Register

In this case, I'd actually hoist the counter definition outside of this instantiation and make it package private variable. You can use https://pkg.go.dev/github.com/prometheus/client_golang/prometheus/promauto to simplify the registration.

This will work because the metrics scraper also adds labels for the pod/service so we can differentiate the various metrics when querying.

It registers them here:

gitpod/components/ws-manager/cmd/run.go

Line 122 in 29ba28f

metrics.Registry.MustRegister(ratelimits)

My bad, missed this. Normally, I'd expect the registration to happen in this file and automatically without requiring an extra call. The risk here is that a component uses the rate limiting interceptor but doesn't register the metric.

The other issue is that this would panic on startup, but CI doesn't catch this.

One way to address this would be accept the metrics registry when constructing the interceptor. This would signal to the caller that they control the registry and need to register it.

It would also make it possible to inject a registry in tests which would allow us to test the metrics, if we wanted to.

For a while now we've adopted the pattern to make things implement the prometheus.Collector interface so that they can be registered.
Testing is just as easy as passing in the registry, but it doesn't mandate that someone uses metrics.
The "registration error at runtime" behaviour is also the same.

Thanks. I'd personally like the need for registration to be more explicit (and to use DI to pass the registry in) but given this is an existing pattern, better to stick to it than introduce yet another. Thanks for the info.

easyCZ · 2022-04-28T19:39:31Z

components/common-go/grpc/ratelimit.go

+	cacheHitCounter := prometheus.NewCounterVec(prometheus.CounterOpts{
+		Namespace: "grpc",
+		Subsystem: "server",
+		Name:      "rate_limiter_calls_total",


Copy-paste error with the name in callCounter. Once the metrics are registered (other comment), this would cause a panic/error.

sagor999

just need to fix metrics name copy paste error.

sagor999 · 2022-04-29T01:55:06Z

components/common-go/grpc/ratelimit.go

 }

 // NewRatelimitingInterceptor creates a new rate limiting interceptor
 func NewRatelimitingInterceptor(f map[string]RateLimit) RatelimitingInterceptor {
-	funcs := make(map[string]ratelimitedFunction, len(f))
+	callCounter := prometheus.NewCounterVec(prometheus.CounterOpts{


It registers them here:

gitpod/components/ws-manager/cmd/run.go

Line 122 in 29ba28f

metrics.Registry.MustRegister(ratelimits)

sagor999 · 2022-04-29T02:01:48Z

/hold to make sure metrics name gets fixed before merge. feel free to remove hold once addressed.

csweichel · 2022-04-29T08:20:32Z

fixed the metrics name

csweichel · 2022-04-29T08:35:54Z

/hold cancel

felladrin

Checked the code, and also took into account other reviewers comments and the test case. ✅

roboquat added do-not-merge/work-in-progress release-note size/L labels Apr 26, 2022

csweichel force-pushed the cw/grpc-rl-fields branch from 2f17da9 to a467582 Compare April 26, 2022 10:21

aledbf reviewed Apr 26, 2022

View reviewed changes

test/go.sum Outdated Show resolved Hide resolved

aledbf reviewed Apr 26, 2022

View reviewed changes

test/go.mod Outdated Show resolved Hide resolved

csweichel force-pushed the cw/grpc-rl-fields branch 2 times, most recently from f57737e to 681fb56 Compare April 26, 2022 10:29

[common-go] Add keyed gRPC rate limits

cd88685

csweichel force-pushed the cw/grpc-rl-fields branch from 681fb56 to cd88685 Compare April 27, 2022 07:29

csweichel marked this pull request as ready for review April 27, 2022 07:29

csweichel requested a review from a team April 27, 2022 07:29

csweichel requested a review from sagor999 as a code owner April 27, 2022 07:29

csweichel requested a review from a team April 27, 2022 07:29

csweichel requested a review from geropl as a code owner April 27, 2022 07:29

csweichel requested a review from a team April 27, 2022 07:29

roboquat removed the do-not-merge/work-in-progress label Apr 27, 2022

github-actions bot added team: IDE team: webapp Issue belongs to the WebApp team team: workspace Issue belongs to the Workspace team labels Apr 27, 2022

easyCZ reviewed Apr 27, 2022

View reviewed changes

geropl assigned easyCZ and geropl Apr 27, 2022

geropl reviewed Apr 27, 2022

View reviewed changes

geropl approved these changes Apr 27, 2022

View reviewed changes

[common-go] Add metrics to the gRPC rate limiter

11c5bac

roboquat removed the size/L label Apr 28, 2022

roboquat added the size/XL label Apr 28, 2022

easyCZ reviewed Apr 28, 2022

View reviewed changes

sagor999 approved these changes Apr 29, 2022

View reviewed changes

roboquat added the do-not-merge/hold label Apr 29, 2022

[ws-manager] Expose gRPC rate limiter metrics

264ab74

csweichel force-pushed the cw/grpc-rl-fields branch from 29ba28f to 264ab74 Compare April 29, 2022 08:20

roboquat removed the do-not-merge/hold label Apr 29, 2022

easyCZ approved these changes Apr 29, 2022

View reviewed changes

felladrin approved these changes Apr 29, 2022

View reviewed changes

roboquat merged commit ff6c381 into main Apr 29, 2022

roboquat deleted the cw/grpc-rl-fields branch April 29, 2022 10:31

roboquat added the deployed: webapp Meta team change is running in production label May 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[common-go] Add keyed gRPC rate limits #9547

[common-go] Add keyed gRPC rate limits #9547

csweichel commented Apr 26, 2022

csweichel commented Apr 26, 2022 •

edited by werft-gitpod-dev-com bot

Loading

easyCZ commented Apr 27, 2022 •

edited

Loading

csweichel commented Apr 27, 2022 •

edited

Loading

easyCZ left a comment

easyCZ Apr 27, 2022

easyCZ Apr 27, 2022

csweichel Apr 28, 2022

geropl Apr 27, 2022

csweichel Apr 27, 2022 •

edited

Loading

geropl left a comment

csweichel commented Apr 28, 2022

easyCZ Apr 28, 2022

easyCZ Apr 28, 2022 •

edited

Loading

sagor999 Apr 29, 2022

easyCZ Apr 29, 2022

easyCZ Apr 29, 2022 •

edited

Loading

csweichel Apr 29, 2022

easyCZ Apr 29, 2022

easyCZ Apr 28, 2022 •

edited

Loading

sagor999 left a comment

sagor999 Apr 29, 2022

sagor999 commented Apr 29, 2022

csweichel commented Apr 29, 2022

csweichel commented Apr 29, 2022

felladrin left a comment

	return func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (resp interface{}, err error) {
	return func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) {

[common-go] Add keyed gRPC rate limits #9547

[common-go] Add keyed gRPC rate limits #9547

Conversation

csweichel commented Apr 26, 2022

Description

How to test

Release Notes

csweichel commented Apr 26, 2022 • edited by werft-gitpod-dev-com bot Loading

easyCZ commented Apr 27, 2022 • edited Loading

csweichel commented Apr 27, 2022 • edited Loading

easyCZ 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

csweichel Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

geropl left a comment

Choose a reason for hiding this comment

csweichel commented Apr 28, 2022

Choose a reason for hiding this comment

easyCZ Apr 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easyCZ Apr 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easyCZ Apr 28, 2022 • edited Loading

Choose a reason for hiding this comment

sagor999 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sagor999 commented Apr 29, 2022

csweichel commented Apr 29, 2022

csweichel commented Apr 29, 2022

felladrin left a comment

Choose a reason for hiding this comment

csweichel commented Apr 26, 2022 •

edited by werft-gitpod-dev-com bot

Loading

easyCZ commented Apr 27, 2022 •

edited

Loading

csweichel commented Apr 27, 2022 •

edited

Loading

csweichel Apr 27, 2022 •

edited

Loading

easyCZ Apr 28, 2022 •

edited

Loading

easyCZ Apr 29, 2022 •

edited

Loading

easyCZ Apr 28, 2022 •

edited

Loading