From f321d11bd315f8bf86165c235f9fcf5de435e57c Mon Sep 17 00:00:00 2001 From: maxime Date: Tue, 7 May 2024 16:31:44 +0100 Subject: [PATCH 1/9] Add some tests to redis limiter. --- redis/BUILD | 21 +++++++++++- redis/limiter_test.go | 79 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+), 1 deletion(-) create mode 100644 redis/limiter_test.go diff --git a/redis/BUILD b/redis/BUILD index 0f9c1d02..bc36efec 100644 --- a/redis/BUILD +++ b/redis/BUILD @@ -16,7 +16,10 @@ sh_cmd( go_library( name = "redis", - srcs = glob(["*.go"]), + srcs = glob( + ["*.go"], + exclude = ["*_test.go"], + ), visibility = ["PUBLIC"], deps = [ "///third_party/go/github.com_go-redis_redis_v8//:v8", @@ -24,3 +27,19 @@ go_library( "///third_party/go/golang.org_x_time//rate", ], ) + +go_test( + name = "redis_test", + srcs = glob( + ["*_test.go"], + ), + deps = [ + ":redis", + "///third_party/go/github.com_go-redis_redis_v8//:v8", + "///third_party/go/github.com_stretchr_testify//assert", + "///third_party/go/github.com_stretchr_testify//require", + "///third_party/go/golang.org_x_time//rate", + ], +) + + diff --git a/redis/limiter_test.go b/redis/limiter_test.go new file mode 100644 index 00000000..b6290291 --- /dev/null +++ b/redis/limiter_test.go @@ -0,0 +1,79 @@ +package redis + +import ( + "context" + "errors" + "testing" + + "github.com/go-redis/redis/v8" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/time/rate" +) + +func TestLimiterAllow(t *testing.T) { + tests := map[string]struct { + l *Limiter + + expectedError bool + }{ + "returns true if rate limiter has some tokens left": { + l: &Limiter{limiter: rate.NewLimiter(0.1, 1)}, + expectedError: false, + }, + "returns false if rate limiter has no token left": { + l: &Limiter{limiter: &rate.Limiter{}}, + expectedError: true, + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + err := test.l.Allow() + if test.expectedError { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} + +func TestLimiterReportResult(t *testing.T) { + tests := map[string]struct { + result error + + allowMore bool + }{ + "allows nil errors": { + allowMore: true, + }, + "allows redis.Nil errors": { + result: redis.Nil, + allowMore: true, + }, + "allows context canceled errors": { + result: context.Canceled, + allowMore: true, + }, + "other errors are not allowed and count towards rate limit": { + result: errors.New("random infra error"), + allowMore: false, + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + // Allows one error every 10 seconds + l := &Limiter{limiter: rate.NewLimiter(0.1, 1)} + require.NoError(t, l.Allow()) + l.ReportResult(test.result) + if test.allowMore { + assert.NoError(t, l.Allow()) + } else { + assert.Error(t, l.Allow()) + } + }) + } + +} From c0fda9c506796c58fa441c07392e3a740c547ade Mon Sep 17 00:00:00 2001 From: maxime Date: Tue, 7 May 2024 16:32:04 +0100 Subject: [PATCH 2/9] Dont take into account context canceled in limiter. --- redis/limiter.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/redis/limiter.go b/redis/limiter.go index 9e72362d..55fdf343 100644 --- a/redis/limiter.go +++ b/redis/limiter.go @@ -1,6 +1,7 @@ package redis import ( + "context" "errors" "github.com/go-redis/redis/v8" @@ -24,7 +25,7 @@ func (l *Limiter) Allow() error { // ReportResult records any non-nil error against the rate limiter. func (l *Limiter) ReportResult(err error) { - if err != nil && err != redis.Nil { + if err != nil && !errors.Is(err, redis.Nil) && !errors.Is(err, context.Canceled) { l.limiter.Reserve() } } From 49122040281898bf33897170a98ffcb6e78b5d50 Mon Sep 17 00:00:00 2001 From: maxime Date: Tue, 7 May 2024 16:36:19 +0100 Subject: [PATCH 3/9] Bump version. --- ChangeLog | 5 +++++ VERSION | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index 61f83b23..c084051c 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,8 @@ +Version 11.10.2 +-------------- + * Add some tests to redis limiter. + * Dont take into account context canceled in limiter. + Version 11.10.1 -------------- * Stop printing warnings when we get redis.Nil errors. diff --git a/VERSION b/VERSION index 46577f54..bd0f144b 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -11.10.1 +11.10.2 From c43406a4bee71101a5f8d11a2ea81e14347794e2 Mon Sep 17 00:00:00 2001 From: maxime Date: Tue, 7 May 2024 16:54:25 +0100 Subject: [PATCH 4/9] Add option to set min number of idle connections to redis. --- redis/options.go | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/redis/options.go b/redis/options.go index 0f0f0a7a..ea90920e 100644 --- a/redis/options.go +++ b/redis/options.go @@ -23,19 +23,21 @@ const DefaultMaxSize int64 = 200 * 1012 // 200 Kelly-Bootle standard units // logic that calls the clients. This is just so we have a single place where // this option is defined. type Opts struct { - URL string `long:"url" env:"REDIS_URL" description:"host:port of Redis server"` - ReadURL string `long:"read_url" env:"REDIS_READ_URL" description:"host:port of a Redis read replica, if set any read operation will be routed to it"` - Password string `long:"password" description:"AUTH password"` - PasswordFile string `long:"password_file" env:"REDIS_PASSWORD_FILE" description:"File containing AUTH password"` - PoolSize int `long:"pool_size" env:"REDIS_POOL_SIZE" default:"10" description:"Size of connection pool on primary redis client"` - ReadPoolSize int `long:"read_pool_size" env:"REDIS_READ_POOL_SIZE" default:"10" description:"Size of connection pool on reading redis client"` - PoolTimeout flags.Duration `long:"pool_timeout" env:"REDIS_POOL_TIMEOUT" default:"5s" description:"Timeout waiting for free connection to primary redis"` - ReadPoolTimeout flags.Duration `long:"read_pool_timeout" env:"REDIS_READ_POOL_TIMEOUT" default:"5s" description:"Timeout waiting for free connection to read replicas"` - ReadTimeout flags.Duration `long:"read_timeout" env:"REDIS_READ_TIMEOUT" default:"1s" description:"Timeout on network read (not read commands)"` - WriteTimeout flags.Duration `long:"write_timeout" env:"REDIS_WRITE_TIMEOUT" default:"1m" description:"Timeout on network write (not write commands)"` - CAFile string `long:"ca_file" env:"REDIS_CA_FILE" description:"File containing the Redis instance CA cert"` - TLS bool `long:"tls" description:"Use TLS for connecting to Redis"` - MaxSize int64 `long:"max_size" env:"REDIS_MAX_SIZE" default:"202400" description:"Max size of objects indexed on redis"` // default is 200 Kelly-Bootle standard units + URL string `long:"url" env:"REDIS_URL" description:"host:port of Redis server"` + ReadURL string `long:"read_url" env:"REDIS_READ_URL" description:"host:port of a Redis read replica, if set any read operation will be routed to it"` + Password string `long:"password" description:"AUTH password"` + PasswordFile string `long:"password_file" env:"REDIS_PASSWORD_FILE" description:"File containing AUTH password"` + PoolSize int `long:"pool_size" env:"REDIS_POOL_SIZE" default:"10" description:"Size of connection pool on primary redis client"` + ReadPoolSize int `long:"read_pool_size" env:"REDIS_READ_POOL_SIZE" default:"10" description:"Size of connection pool on reading redis client"` + MinIdleConns int `long:"min_idle_conns" env:"REDIS_MIN_IDLE_CONNS" default:"0" description:"Minimum number of idle connections to primary redis."` + ReadMinIdleConns int `long:"read_min_idle_conns" env:"REDIS_READ_MIN_IDLE_CONNS" default:"0" description:"Minimum number of idle connections to read replicas."` + PoolTimeout flags.Duration `long:"pool_timeout" env:"REDIS_POOL_TIMEOUT" default:"5s" description:"Timeout waiting for free connection to primary redis"` + ReadPoolTimeout flags.Duration `long:"read_pool_timeout" env:"REDIS_READ_POOL_TIMEOUT" default:"5s" description:"Timeout waiting for free connection to read replicas"` + ReadTimeout flags.Duration `long:"read_timeout" env:"REDIS_READ_TIMEOUT" default:"1s" description:"Timeout on network read (not read commands)"` + WriteTimeout flags.Duration `long:"write_timeout" env:"REDIS_WRITE_TIMEOUT" default:"1m" description:"Timeout on network write (not write commands)"` + CAFile string `long:"ca_file" env:"REDIS_CA_FILE" description:"File containing the Redis instance CA cert"` + TLS bool `long:"tls" description:"Use TLS for connecting to Redis"` + MaxSize int64 `long:"max_size" env:"REDIS_MAX_SIZE" default:"202400" description:"Max size of objects indexed on redis"` // default is 200 Kelly-Bootle standard units } // Clients sets up clients to both primary and read replicas. If no read URL @@ -63,6 +65,7 @@ func (r Opts) Clients() (primary, read *redis.Client) { ReadTimeout: time.Duration(r.ReadTimeout), WriteTimeout: time.Duration(r.WriteTimeout), PoolTimeout: time.Duration(r.PoolTimeout), + MinIdleConns: r.MinIdleConns, Limiter: limiter, }) if r.ReadURL != "" { @@ -74,6 +77,7 @@ func (r Opts) Clients() (primary, read *redis.Client) { ReadTimeout: time.Duration(r.ReadTimeout), WriteTimeout: time.Duration(r.WriteTimeout), PoolTimeout: time.Duration(r.ReadPoolTimeout), + MinIdleConns: r.ReadMinIdleConns, Limiter: limiter, }) } else { From 72b2fe09de2a542bcae21533af2844eac9e4c725 Mon Sep 17 00:00:00 2001 From: maxime Date: Tue, 7 May 2024 16:55:09 +0100 Subject: [PATCH 5/9] Update changelog. --- ChangeLog | 1 + 1 file changed, 1 insertion(+) diff --git a/ChangeLog b/ChangeLog index c084051c..279bda49 100644 --- a/ChangeLog +++ b/ChangeLog @@ -2,6 +2,7 @@ Version 11.10.2 -------------- * Add some tests to redis limiter. * Dont take into account context canceled in limiter. + * Add option to set mininum number of idle connections to redis. Version 11.10.1 -------------- From a3d73562dd3a5a749898662d0921c264bb768105 Mon Sep 17 00:00:00 2001 From: maxime Date: Tue, 7 May 2024 17:03:09 +0100 Subject: [PATCH 6/9] Remove trailing new lines. --- redis/BUILD | 2 -- 1 file changed, 2 deletions(-) diff --git a/redis/BUILD b/redis/BUILD index bc36efec..faa69884 100644 --- a/redis/BUILD +++ b/redis/BUILD @@ -41,5 +41,3 @@ go_test( "///third_party/go/golang.org_x_time//rate", ], ) - - From bd1f725df242229c8b93791b9045180c3f0091c0 Mon Sep 17 00:00:00 2001 From: maxime Date: Wed, 8 May 2024 07:56:19 +0100 Subject: [PATCH 7/9] Prevent limiter from counting its own errors towards rate limit. --- redis/limiter.go | 10 ++++++++-- redis/limiter_test.go | 4 ++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/redis/limiter.go b/redis/limiter.go index 55fdf343..73153c73 100644 --- a/redis/limiter.go +++ b/redis/limiter.go @@ -8,6 +8,9 @@ import ( "golang.org/x/time/rate" ) +// ErrRateLimitReached is returned when the rate limit of errors is reached. +var ErrRateLimitReached = errors.New("limiter error rate exceeded, skipping Redis call") + // Limiter implements redis.Limiter so we can rate limit calls to Redis if // they fail too often. type Limiter struct { @@ -18,14 +21,17 @@ type Limiter struct { // we won't allow the redis call. func (l *Limiter) Allow() error { if l.limiter.Tokens() < 1.0 { - return errors.New("limiter error rate exceeded, skipping Redis call") + return ErrRateLimitReached } return nil } // ReportResult records any non-nil error against the rate limiter. func (l *Limiter) ReportResult(err error) { - if err != nil && !errors.Is(err, redis.Nil) && !errors.Is(err, context.Canceled) { + if err != nil && + !errors.Is(err, redis.Nil) && + !errors.Is(err, ErrRateLimitReached) && + !errors.Is(err, context.Canceled) { l.limiter.Reserve() } } diff --git a/redis/limiter_test.go b/redis/limiter_test.go index b6290291..bc72233e 100644 --- a/redis/limiter_test.go +++ b/redis/limiter_test.go @@ -56,6 +56,10 @@ func TestLimiterReportResult(t *testing.T) { result: context.Canceled, allowMore: true, }, + "allows rate limit errors": { + result: ErrRateLimitReached, + allowMore: true, + }, "other errors are not allowed and count towards rate limit": { result: errors.New("random infra error"), allowMore: false, From 054198b2716c2bb34a087229582eb13b1af7820f Mon Sep 17 00:00:00 2001 From: maxime Date: Wed, 8 May 2024 07:57:21 +0100 Subject: [PATCH 8/9] Update changelog. --- ChangeLog | 1 + 1 file changed, 1 insertion(+) diff --git a/ChangeLog b/ChangeLog index 279bda49..cd295122 100644 --- a/ChangeLog +++ b/ChangeLog @@ -3,6 +3,7 @@ Version 11.10.2 * Add some tests to redis limiter. * Dont take into account context canceled in limiter. * Add option to set mininum number of idle connections to redis. + * Prevent limiter from counting its own errors towards rate limit. Version 11.10.1 -------------- From 1d1beb1068c6ffee6204aa302633662e860cdf11 Mon Sep 17 00:00:00 2001 From: Maxime Fischer Date: Wed, 8 May 2024 10:12:26 +0100 Subject: [PATCH 9/9] Update ChangeLog Co-authored-by: Tatiana Iljushenko <33573204+Liu-ko@users.noreply.github.com> --- ChangeLog | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index cd295122..027ac3c8 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,7 +1,7 @@ Version 11.10.2 -------------- * Add some tests to redis limiter. - * Dont take into account context canceled in limiter. + * Don't take into account context canceled in limiter. * Add option to set mininum number of idle connections to redis. * Prevent limiter from counting its own errors towards rate limit.