rate-limiting: Add ScheduledStartingRateLimiter by oschaaf · Pull Request #281 · envoyproxy/nighthawk

oschaaf · 2020-01-15T09:09:05Z

As per discussion [1], this is intended to be used as a replacement
for the way we offset worker request release timings today.

[1] #219 (comment)

Signed-off-by: Otto van der Schaaf oschaaf@we-amp.com

As per discussion [1], this is intended to be used as a replacement for the way we offset worker request release timings today. [1] envoyproxy#219 (comment) Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

mum4k · 2020-01-15T15:05:18Z

@dubious90 please review and assign to me once done.

dubious90

This way of implementing this feature makes a lot of sense to me. Thanks for putting this change together. A few small comments/questions.

dubious90 · 2020-01-15T15:09:54Z

test/rate_limiter_test.cc

+        .Times(AtLeast(1))
+        .WillRepeatedly(Return(true));
+
+    if (starting_late) {


I'm not sure if starting_late is adding any extra coverage for this test case. This test should already be testing what happens when enough time has passed and when not enough time has passed.

I'm not sure I follow; the intent here is to skip the scheduled delay entirely, so the first call to acquireOne() will hit the line that logs the warning that the "first acquisition attempt was late":

https://github.com/envoyproxy/nighthawk/pull/281/files/de66b5e48997d31728125385f33265bfb62094df#diff-698caf1e153397f66dc8ca94b64a391fR72

Ah, okay. I didn't process the comments in line 74-76 properly when I read this code the first time. I follow now.

Is there a way to assert that the log line was actually hit? Or is this just for line code coverage?

This is just for coverage. Unfortunately we can't really mock the logging today and I couldn't find an efficient way to assert on hitting that line

dubious90 · 2020-01-15T15:10:32Z

test/rate_limiter_test.cc

+    }
+
+    // We should expect zero releases until it is time to start.
+    while (time_system.monotonicTime() < scheduled_starting_time) {


If this test case doesn't succeed, it will run infinitely rather than fail. Can you add in a tripping mechanism?

The simulated time system here is guaranteed to move forward in this loop with 1ms steps we add via the call to time_system.sleep(1ms) below.. afaict this is guaranteed to terminate?

Nevermind here. I was overcomplicated the situation in my head. Sorry about that!

dubious90 · 2020-01-15T15:15:29Z

source/common/rate_limiter_impl.cc

+  // about this happening in the logs.
+  if (!aquisition_attempted_) {
+    aquisition_attempted_ = true;
+    ENVOY_LOG(warn, "First acquisition attempt was late");


Can we add in an identifier here, so this message makes more sense for a debugger who might not have context into this specific rate limiter?
Something like
ScheduledStartingRateLimiter first acquisition attempt was late

dubious90 · 2020-01-15T15:18:57Z

source/common/rate_limiter_impl.cc

+}
+
+void ScheduledStartingRateLimiter::releaseOne() {
+  if (timeSource().monotonicTime() < scheduled_starting_time_) {


I'm probably missing something, but couldn't a cancel operation (not yet implemented) result in releasing before the time arrives? What are we trying to prevent by failing here?

Right now, it is only valid to release what has been acquired. So when we hit this, it ought to be impossible to have a preceding successful acquisition because we're not due to start yet. The intent is to catch programming mistakes early when consuming this, but possibly this is overly paranoid.. shall I remove this?

This explanation makes sense to me. I don't think it's necessary to remove this.

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

dubious90

Just one small followup left. In parallel assigning to mum4k for approval

dubious90 · 2020-01-15T16:24:46Z

source/common/rate_limiter_impl.cc

+}
+
+void ScheduledStartingRateLimiter::releaseOne() {
+  if (timeSource().monotonicTime() < scheduled_starting_time_) {


This explanation makes sense to me. I don't think it's necessary to remove this.

dubious90 · 2020-01-15T16:27:46Z

test/rate_limiter_test.cc

+        .Times(AtLeast(1))
+        .WillRepeatedly(Return(true));
+
+    if (starting_late) {


Ah, okay. I didn't process the comments in line 74-76 properly when I read this code the first time. I follow now.

Is there a way to assert that the log line was actually hit? Or is this just for line code coverage?

dubious90 · 2020-01-15T16:34:05Z

test/rate_limiter_test.cc

+    }
+
+    // We should expect zero releases until it is time to start.
+    while (time_system.monotonicTime() < scheduled_starting_time) {


Nevermind here. I was overcomplicated the situation in my head. Sorry about that!

…g-rate-limiter Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

rate-limiting: Add ScheduledStartingRateLimiter

c35d7c8

As per discussion [1], this is intended to be used as a replacement for the way we offset worker request release timings today. [1] envoyproxy#219 (comment) Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf added P2 waiting-for-review A PR waiting for a review. labels Jan 15, 2020

oschaaf mentioned this pull request Jan 15, 2020

Introduce Phases #219

Merged

1 task

oschaaf added 2 commits January 15, 2020 10:18

Remove stale comment

26e8f00

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Clean up

de66b5e

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

mum4k requested a review from dubious90 January 15, 2020 15:05

dubious90 suggested changes Jan 15, 2020

View reviewed changes

dubious90 added waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. and removed waiting-for-review A PR waiting for a review. labels Jan 15, 2020

Review feedback

0058216

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf added waiting-for-review A PR waiting for a review. and removed waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. labels Jan 15, 2020

dubious90 reviewed Jan 15, 2020

View reviewed changes

dubious90 assigned mum4k Jan 15, 2020

Merge remote-tracking branch 'upstream/master' into scheduled-startin…

41c0d6a

…g-rate-limiter Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

mum4k approved these changes Jan 15, 2020

View reviewed changes

mum4k merged commit 403be21 into envoyproxy:master Jan 15, 2020

Conversation

oschaaf commented Jan 15, 2020

Uh oh!

mum4k commented Jan 15, 2020

Uh oh!

dubious90 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dubious90 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants