Rolling window limit support #193

walbertus · 2020-11-18T08:01:27Z

Add support for rolling window limit #32

src/redis/cache_impl.go

walbertus · 2020-11-30T02:16:48Z

Hi @mattklein123 , is there anything else I can do for this MR?

mattklein123

Thanks for implementing this. Flushing out the first round of comments. Thank you!

/wait

mattklein123 · 2020-11-30T17:25:47Z

src/redis/cache_impl.go

+			localCache,
+			s.NearLimitRatio)
+	} else {
+		logger.Fatalf("Unknown rate limit algorithm. %s\n", s.RateLimitAlgorithm)


I think this is going to crash in some strange way after returning nil? Would it be better to panic here or should we have some more graceful handling somewhere?

Throwing error and let the runner handle the error. Log level parser also trigger panic in the runner

mattklein123 · 2020-11-30T17:32:46Z

src/redis/windowed_cache_impl.go

+func maxInt64(a int64, b int64) int64 {
+	if a > b {
+		return a
+	}
+	return b
+}
+
+func minInt64(a int64, b int64) int64 {
+	if a < b {
+		return a
+	}
+	return b
+}


The fact that this is implemented here and not in the std lib makes me cry, but so it goes. If this is not present elsewhere in the code can you at least put it in a common utility? If it is already elsewhere please share it?

Moving max and min related code to utils

mattklein123 · 2020-11-30T17:34:09Z

src/redis/windowed_cache_impl.go

+func nanosecondsToDuration(nanoseconds int64) *duration.Duration {
+	nanos := nanoseconds
+	secs := nanos / 1e9
+	nanos -= secs * 1e9
+	return &duration.Duration{Seconds: secs, Nanos: int32(nanos)}
+}
+
+func secondsToNanoseconds(second int64) int64 {
+	return second * 1e9
+}
+
+func nanosecondsToSeconds(nanoseconds int64) int64 {
+	return nanoseconds / 1e9
+}


Are there no helpers in the Go proto code that already do this? There are for C++.

Cannot found it on proto duration
Should I move these code to utils too?

mattklein123 · 2020-11-30T17:35:38Z

src/redis/windowed_cache_impl.go

+	*pipeline = client.PipeAppend(*pipeline, result, "GET", key)
+}
+
+func windowedSetNewTatPipelineAppend(client Client, pipeline *Pipeline, key string, newTat int64, expirationSeconds int64) {


What is "Tat"? Can you spell this out or add more comments?

mattklein123 · 2020-11-30T17:36:14Z

src/redis/windowed_cache_impl.go

+	"golang.org/x/net/context"
+)
+
+type windowedRateLimitCacheImpl struct {


Can you add some high level comments on how rolling window limits are implemented? It will help the reader and my own review.

Adding some explanations and reference for GCRA

mattklein123 · 2020-11-30T17:38:50Z

src/redis/windowed_cache_impl.go

+		perSecondPipeline = nil
+	}
+
+	// Rate limit GCRA logic


This code needs more comments per above. Maybe an overview comment will help me.

Also, a bunch of non-trivial code in this file is shared with the fixed impl. Is it possible to share more code via a shared utility or mix-in?

Hi, can you please give me examples in ratelimit about the shared utility and mix-in?
Thanks

mattklein123 · 2020-12-07T17:52:41Z

@nezdolik could I kindly ask you to review this one also? It's going to conflict with the memcached PR and I want to make sure we are heading in the right direction and we sequence the PRs correctly. Thank you!

nezdolik

I have a comment about code structure below. Will take one more pass tomorrow.

nezdolik · 2020-12-09T18:57:17Z

src/redis/windowed_cache_impl.go

@@ -0,0 +1,262 @@
+package redis


2 things to possibly consider:

There is some difference between fixed window vs floating window codewise, but big piece of code is duplicated. Would it be possible to parametrise cache impl with algorithm object of user choice? The algorithm would be aware on how to launch redis pipeline and how to calculate duration until reset.

(Could be followup PR) Make algorithm to be agnostic of backend type. In case we parametrise cache impl with algorithm, we could further parametrise algorithm itself with an object that is aware of backend type (redis, memcache) , maybe interaction with backend should not even be part of algorithm type.

nezdolik · 2020-12-09T19:13:52Z

test/redis/windowed_cache_impl_test.go

+}
+
+func TestRedisWindowedWithJitter(t *testing.T) {
+	assert := assert.New(t)


can the code which sets up all the mocks and creates cache impl object be pulled from each test into some sort of SetUp method and be called in each test?

nezdolik

Could you add integration test?

nezdolik · 2020-12-14T18:49:10Z

src/redis/windowed_cache_impl.go

+	nearLimitRatio             float32
+}
+
+func nanosecondsToDuration(nanoseconds int64) *duration.Duration {


should those methods be part of some sort of time util?

stale · 2020-12-24T11:44:20Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

walbertus · 2020-12-29T09:38:53Z

Hi, sorry for the delay. I'll continue working on the integration test and refactor this week.

mattklein123 · 2021-01-05T17:33:17Z

OK I think with @nezdolik refactor in, hopefully there should be less duplicate code now.

/wait

Signed-off-by: zufardhiyaulhaq <[email protected]>

zufardhiyaulhaq · 2021-02-10T06:23:21Z

is the CI is flaky somehow? I don't see any error when executing
make docker_tests in my local. I cannot retry the CI in GitHub actions

ok      github.com/envoyproxy/ratelimit/test/integration        58.436s
ok      github.com/envoyproxy/ratelimit/test/memcached  (cached)
?       github.com/envoyproxy/ratelimit/test/mocks      [no test files]
?       github.com/envoyproxy/ratelimit/test/mocks/algorithm    [no test files]
?       github.com/envoyproxy/ratelimit/test/mocks/config       [no test files]
?       github.com/envoyproxy/ratelimit/test/mocks/limiter      [no test files]
?       github.com/envoyproxy/ratelimit/test/mocks/memcached/driver     [no test files]
?       github.com/envoyproxy/ratelimit/test/mocks/redis/driver [no test files]
?       github.com/envoyproxy/ratelimit/test/mocks/rls  [no test files]
?       github.com/envoyproxy/ratelimit/test/mocks/runtime/loader       [no test files]
?       github.com/envoyproxy/ratelimit/test/mocks/runtime/snapshot     [no test files]
?       github.com/envoyproxy/ratelimit/test/mocks/utils        [no test files]
ok      github.com/envoyproxy/ratelimit/test/redis      (cached)
ok      github.com/envoyproxy/ratelimit/test/server     (cached)
ok      github.com/envoyproxy/ratelimit/test/service    (cached)

Signed-off-by: zufardhiyaulhaq <[email protected]>

zufardhiyaulhaq · 2021-02-10T11:38:27Z

@dweitzman we try to refactor the code so it can support another algorithm in the future. GCRA is like a leaky bucket but only uses time as a measurement. I am still not sure how https://blog.cloudflare.com/counting-things-a-lot-of-different-things/. you can read the GCRA calculation here https://blog.ian.stapletoncordas.co/2018/12/understanding-generic-cell-rate-limiting.html

give some diagram to better understand GCRA

@mattklein123 @nezdolik finish adding a unit test for the algorithm and rolling window implementation for Memcached & Redis, can you try to review it? Thanks

nezdolik · 2021-02-10T19:33:54Z

Thanks @zufardhiyaulhaq, will take one more pass tomorrow.

zufardhiyaulhaq · 2021-02-11T04:55:43Z

@nezdolik @mattklein123 give you the result of testing the rolling window rate limit, testing with Istio.

Memcached rolling window rate limit (10 requests/second in 4 minutes, rate-limited for 10 requests/minute)

"requests": 2400,
  "rate": 10.004171883777294,
  "throughput": 0.20424766658417626,
  "success": 0.020416666666666666,
  "status_codes": {
    "200": 49,
    "429": 2348,
    "500": 3
  },
  "errors": [
    "429 Too Many Requests",
    "500 Internal Server Error"
  ]

Redis rolling window rate limit (10 requests/second in 4 minutes, rate-limited for 10 requests/minute)

  "requests": 2400,
  "rate": 10.004173279566375,
  "throughput": 0.2000788245626525,
  "success": 0.02,
  "status_codes": {
    "200": 48,
    "429": 2347,
    "500": 5
  },
  "errors": [
    "429 Too Many Requests",
    "500 Internal Server Error"
  ]

nezdolik

@zufardhiyaulhaq spent some time today refactoring duplicated code across fixed and rolling windows, check out this commit: nezdolik@0ff4b0b
Feel free to use the code, it could further be improved. There was bunch of duplicated code across window implementations and it was not a big effort to refactor it.
As per @dweitzman comment, it would be valuable to compare two algorithms and reason which one should end up in upstream.

nezdolik · 2021-02-15T12:57:47Z

README.md

+Fixed window algorithm does not care when did the request arrive, all 60 can arrive at 01:01 or 01:50 and the limit will still reset at 02:00.
+
+2. Rolling window
+For a limit of 60 requests per hour. Initially it is able to take a burst of 60 requests at once, then the limit is restored by 1 each minute. Requests are allowed as long as there's still some available limit.


:s/it is able/it is possible

It would be even more clear to rephrase like 'Initially rate limiter can take a burst of...`

zufardhiyaulhaq · 2021-02-17T03:19:42Z

thanks @nezdolik will check tomorrow

Signed-off-by: zufardhiyaulhaq <[email protected]>

zufardhiyaulhaq · 2021-02-20T11:53:03Z

@nezdolik @mattklein123 refactor done, I also add a unit test for the base algorithm. Testing in Istio

Redis rolling window rate limit (10 requests/second in 4 minutes, rate-limited for 10 requests/minute)

  "requests": 2400,
  "rate": 10.00416761918139,
  "throughput": 0.20424889245049338,
  "success": 0.020416666666666666,
  "status_codes": {
    "200": 49,
    "429": 2347,
    "500": 4
  },
  "errors": [
    "429 Too Many Requests",
    "500 Internal Server Error"
  ]

Memcached rolling window rate limit (10 requests/second in 4 minutes, rate-limited for 10 requests/minute)

  "requests": 2400,
  "rate": 10.004172146579775,
  "throughput": 0.20425008105086626,
  "success": 0.020416666666666666,
  "status_codes": {
    "200": 49,
    "429": 2350,
    "500": 1
  },
  "errors": [
    "429 Too Many Requests",
    "500 Internal Server Error"
  ]

zufardhiyaulhaq · 2021-02-25T04:07:55Z

@nezdolik @mattklein123 can you help review again? thanks

nezdolik · 2021-02-25T09:41:58Z

@zufardhiyaulhaq i will be able to take one more pass tomorrow.

mattklein123 · 2021-03-01T19:07:18Z

Thanks for this feature. This PR is too many lines of code to review. Can we please split this into 2 parts:

Part 1: Just the refactor to move files, split logic, etc.
Part 2: Actually add the rolling window support

Thank you

/wait

nezdolik

+1 on splitting the PR into two parts, would be much easier to review.

nezdolik · 2021-03-01T20:24:32Z

src/memcached/windowed_cache_impl.go

+	timeSource                 utils.TimeSource
+	jitterRand                 *rand.Rand
+	expirationJitterMaxSeconds int64
+	localCache                 *freecache.Cache


we no longer need this field in both window impls right? (as it is part of WindowImpl now)

nezdolik · 2021-03-01T20:34:24Z

src/algorithm/rolling_window.go

+
+type RollingWindowImpl struct {
+	timeSource        utils.TimeSource
+	cacheKeyGenerator utils.CacheKeyGenerator


same for cacheKeyGenerator, we no longer need this field in both window impls, as it has been moved to base window.

zufardhiyaulhaq · 2021-03-04T04:17:18Z

should we create a new MR for this feature?
@mattklein123 @nezdolik

mattklein123 · 2021-03-04T19:25:31Z

should we create a new MR for this feature?

I'm not sure what an MR is but as I mentioned this PR is too large to review/merge. Please do code movement / refactoring first.

github-actions · 2021-06-24T20:07:53Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

github-actions · 2021-07-01T20:08:12Z

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

mattklein123 self-assigned this Nov 18, 2020

mattklein123 added the waiting label Nov 18, 2020

repokitteh-read-only bot removed the waiting label Nov 19, 2020

mattklein123 added the waiting label Nov 19, 2020

walbertus force-pushed the rolling-window-limit branch from d87b48e to 247d5bc Compare November 23, 2020 03:01

repokitteh-read-only bot removed the waiting label Nov 23, 2020

walbertus marked this pull request as ready for review November 23, 2020 03:08

walbertus changed the title ~~[WIP] Rolling window limit support~~ Rolling window limit support Nov 23, 2020

mattklein123 reviewed Nov 23, 2020

View reviewed changes

src/redis/cache_impl.go Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Nov 23, 2020

walbertus force-pushed the rolling-window-limit branch from 630b89f to b224c08 Compare November 25, 2020 04:52

repokitteh-read-only bot removed the waiting label Nov 25, 2020

mattklein123 requested changes Nov 30, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Nov 30, 2020

mattklein123 mentioned this pull request Nov 30, 2020

Implement BACKEND_TYPE=memcache as an alternative k/v store to redis #172

Merged

walbertus force-pushed the rolling-window-limit branch from d3d78e9 to 79481f3 Compare December 7, 2020 15:22

repokitteh-read-only bot removed the waiting label Dec 7, 2020

mattklein123 assigned nezdolik Dec 7, 2020

nezdolik reviewed Dec 9, 2020

View reviewed changes

nezdolik reviewed Dec 14, 2020

View reviewed changes

stale bot added the stale label Dec 24, 2020

mattklein123 added the waiting label Dec 28, 2020

stale bot removed the stale label Dec 28, 2020

repokitteh-read-only bot removed the waiting label Jan 5, 2021

walbertus marked this pull request as draft January 5, 2021 09:54

add fixed algorithm unit test

10404f4

Signed-off-by: zufardhiyaulhaq <[email protected]>

add rolling window algorithm unit test

3ec9cca

Signed-off-by: zufardhiyaulhaq <[email protected]>

nezdolik reviewed Feb 15, 2021

View reviewed changes

zufardhiyaulhaq force-pushed the rolling-window-limit branch 2 times, most recently from 650aa12 to 092ad2c Compare February 20, 2021 06:39

Kateryna Nezdolii and others added 2 commits February 20, 2021 07:50

Refactor fixed and rolling window

73df311

Signed-off-by: zufardhiyaulhaq <[email protected]>

refactor & add base window testing

a400046

Signed-off-by: zufardhiyaulhaq <[email protected]>

zufardhiyaulhaq force-pushed the rolling-window-limit branch from 092ad2c to a400046 Compare February 20, 2021 06:56

zufardhiyaulhaq mentioned this pull request Feb 28, 2021

Proposal: Add WithinLimit Counter to RateLimitStats #230

Open

repokitteh-read-only bot added the waiting label Mar 1, 2021

nezdolik reviewed Mar 1, 2021

View reviewed changes

zufardhiyaulhaq mentioned this pull request Apr 11, 2021

refactor storage implementation #243

Closed

mattklein123 removed the no stalebot label May 25, 2021

github-actions bot added the stale label Jun 24, 2021

github-actions bot closed this Jul 1, 2021

chrishowell mentioned this pull request Jul 27, 2021

Which algorithm ratelimit process is using ? #269

Closed

ysawa0 mentioned this pull request Dec 10, 2021

Rolling window limits? #32

Open

Rolling window limit support #193

Rolling window limit support #193

Conversation

walbertus commented Nov 18, 2020

walbertus commented Nov 30, 2020 • edited Loading

mattklein123 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

mattklein123 commented Dec 7, 2020

nezdolik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nezdolik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Dec 24, 2020

walbertus commented Dec 29, 2020

mattklein123 commented Jan 5, 2021

zufardhiyaulhaq commented Feb 10, 2021

zufardhiyaulhaq commented Feb 10, 2021

nezdolik commented Feb 10, 2021

zufardhiyaulhaq commented Feb 11, 2021 • edited Loading

nezdolik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zufardhiyaulhaq commented Feb 17, 2021

zufardhiyaulhaq commented Feb 20, 2021 • edited Loading

zufardhiyaulhaq commented Feb 25, 2021

nezdolik commented Feb 25, 2021

mattklein123 commented Mar 1, 2021

nezdolik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zufardhiyaulhaq commented Mar 4, 2021

mattklein123 commented Mar 4, 2021

github-actions bot commented Jun 24, 2021

github-actions bot commented Jul 1, 2021

walbertus commented Nov 30, 2020 •

edited

Loading

zufardhiyaulhaq commented Feb 11, 2021 •

edited

Loading

zufardhiyaulhaq commented Feb 20, 2021 •

edited

Loading