Add BurstingRateLimiter for simulating super-bursty traffic. #14

oschaaf · 2019-04-18T20:28:52Z

Introduces a new --burst-size option. Implemented via BurstingRateLimiter, which accumulates acquisitions by the underlying configured RateLimiter, and starts releasing when the burst-size threshold is met. For example, --burst-size 100 --rps 1000 will make the client release 100 requests every 100 milliseconds.

Signed-off-by: Otto van der Schaaf [email protected]

Introduces a new `--burst-size` option. Implemented via `BurstingRateLimiter`, which accumulates acquisitions by the underlying configured `RateLimiter`, and starts releasing when the burst-size threshold is met. For example, `--burst-size 100 --rps 1000` will make the client release 100 requests every 100 milliseconds. Signed-off-by: Otto van der Schaaf <[email protected]>

Signed-off-by: Otto van der Schaaf <[email protected]>

oschaaf · 2019-04-21T21:44:19Z

Rebased to master and squashed. Has an extra commit to fix the build that broke after the rebase:

move to HeaderString::getStringView()

Force pushed the squashed version, but I'm leaving a note here to make sure I don't forget about the earlier ASAN failure in CI:
https://circleci.com/gh/envoyproxy/nighthawk/209. This seems worth looking into, but it seems unlikely that it was related to this PR.

May need tweaking the coverage threshold in here, it may err because it didn't consider the test origin when I adjusted it earlier.

/assign @htuch

oschaaf · 2019-04-22T08:28:30Z

It looks like the hack to avoid runtime conflicts in the integration test for the client came back to haunt us :(
That test now flakes with assert failure: thread_advancing_time_->isCurrentThreadId(). Details: time should only be advanced on one thread in the context of a test.

oschaaf · 2019-04-22T08:35:27Z

Reading the code, there's a comment in the BaseIntegrationTest which remarks about a hack where it calls sleep() [1]. I think that is where the time system initializes its thread-id for further checking later on.
If it wouldn't do that we would be fine... Unfortunately, the constructor that accepts a time system ignores whatever we pass in, so it seems that we also cannot pass in a different time system that doesn't perform this check to avoid the assert.

[1] https://github.com/envoyproxy/envoy/blob/b41ba5925a4e93d22a86c6501d63314ccf0d79f3/test/integration/integration.cc#L239

oschaaf · 2019-04-22T10:49:04Z

I think we should land #15 first, and then rebase this.

Signed-off-by: Otto van der Schaaf <[email protected]>

…iter

oschaaf · 2019-04-22T16:34:18Z

Note: just merged master into this, which should clean up the diff now that #15 was merged over there.

htuch · 2019-04-22T19:36:11Z

@oschaaf can you merge master again? Thanks

…iter Signed-off-by: Otto van der Schaaf <[email protected]>

oschaaf · 2019-04-22T19:55:54Z

@htuch done, merged master

Signed-off-by: Otto van der Schaaf <[email protected]>

htuch

Looks good modulo a few comments.
/wait

htuch · 2019-04-22T22:48:33Z

ci/run_coverage.sh

@@ -32,7 +32,7 @@ if [ "$VALIDATE_COVERAGE" == "true" ]
 then
    COVERAGE_VALUE=$(grep -Po '.*lines[.]*: \K(\d|\.)*' "${COVERAGE_SUMMARY}")
    # TODO(oschaaf): The target is 97.5%, so up this whenever possible in follow ups.
-    COVERAGE_THRESHOLD=96.6
+    COVERAGE_THRESHOLD=96.8


htuch · 2019-04-22T22:50:59Z

source/common/rate_limiter_impl.cc

+    // the system is lagging behind.
+    while (rate_limiter_->tryAcquireOne()) {
+      accumulated_++;
+      if ((accumulated_ % burst_size_) == 0) {


The use of % here together with both ++ on release and acquire is clever; it took me a minute to grok though, could you add a comment or two?

htuch · 2019-04-22T22:56:09Z

source/common/rate_limiter_impl.cc

+    accumulated_++;
+    ASSERT(accumulated_ <= burst_size_);
+  } else {
+    rate_limiter_->releaseOne();


Is it ever possible to execute this line?

No, good point, this didn't make sense. I tightened this up.

htuch · 2019-04-22T22:56:49Z

source/common/rate_limiter_impl.h

+ * 1. First it will be accumulating acquisitions by forwarding calls to the wrapped
+ *    rate limiter, until the accumulated acquisitions equals the specified burst size.
+ * 2. Release mode. In this mode, BatchingRateLimiter is in control and will be handling
+ *    acquisition calls (returning true and substracting from the accumulated total until


It cleverly uses increment to subtract! :)

htuch · 2019-04-22T22:57:23Z

test/rate_limiter_test.cc

+  EXPECT_CALL(unsafe_mock_rate_limiter, tryAcquireOne)
+      .Times(burst_size)
+      .WillRepeatedly(Return(true))
+      .RetiresOnSaturation();


What is RetireOnSaturation? Is this effectively the same as InSequence?

Thanks, InSequence specifies the intent here more clearly and is more compact, so I switched it over to use that.

Signed-off-by: Otto van der Schaaf <[email protected]>

oschaaf · 2019-04-23T14:35:42Z

@htuch this is ready for another round

htuch

/wait

htuch · 2019-04-23T16:17:49Z

source/common/rate_limiter_impl.cc

+void BurstingRateLimiter::releaseOne() {
+  ASSERT(accumulated_ < burst_size_);
+  ASSERT(previously_releasing_ != absl::nullopt && previously_releasing_ == true);
+  // The caller wasn't able to put its earlier successfull acquisition to good use, so we restore


Oh, this is actually different to what I thought you were doing. I thought you were doing:

During acquisition, keep increasing accumulated until % burst_size == 0.

During release, keep increasing accumulated until % burst_size ==0.

If you ++ and --, won't you run the risk of getting non-bursty behavior when released tokens get immediately reacquired and a thread spends a long period without requesting a burst?

htuch · 2019-04-23T16:18:18Z

source/client/factories_impl.cc

@@ -41,6 +41,10 @@ SequencerPtr SequencerFactoryImpl::create(Envoy::TimeSource& time_source,
  StatisticFactoryImpl statistic_factory(options_);
  RateLimiterPtr rate_limiter =
      std::make_unique<LinearRateLimiter>(time_source, Frequency(options_.requestsPerSecond()));
+  const uint64_t burst_size = options_.burstSize();
+  if (burst_size) {


Nit: prefer burst_size > 0

htuch

Clarified points offline, thanks, will merge.

oschaaf mentioned this pull request Apr 18, 2019

Performance Issues with Envoy envoyproxy/envoy#5536

Closed

oschaaf changed the title ~~Draft of a wrapper for rate limiter that batches~~ Add BurstingRateLimiter for simulating super-bursty traffic. Apr 21, 2019

oschaaf marked this pull request as ready for review April 21, 2019 20:41

oschaaf added 2 commits April 21, 2019 23:11

Fix build: move to HeaderString::getStringView()

4af229d

Signed-off-by: Otto van der Schaaf <[email protected]>

oschaaf force-pushed the batching-rate-limiter branch from 463060a to 4af229d Compare April 21, 2019 21:30

repokitteh-read-only bot assigned htuch Apr 21, 2019

oschaaf added 2 commits April 22, 2019 12:52

Adjust coverage threshold

94dd5d1

Signed-off-by: Otto van der Schaaf <[email protected]>

Merge remote-tracking branch 'upstream/master' into batching-rate-lim…

3b24b61

…iter

Merge remote-tracking branch 'upstream/master' into batching-rate-lim…

d625af7

…iter Signed-off-by: Otto van der Schaaf <[email protected]>

Add depedency so clang-tidy can find includes

3bf3424

Signed-off-by: Otto van der Schaaf <[email protected]>

htuch suggested changes Apr 22, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 22, 2019

Add comments, fix edge case, adjust tests

103e42b

Signed-off-by: Otto van der Schaaf <[email protected]>

repokitteh-read-only bot removed the waiting label Apr 23, 2019

Use InSequencer instead of RetiresOnSaturation

b4f8652

Signed-off-by: Otto van der Schaaf <[email protected]>

htuch suggested changes Apr 23, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 23, 2019

htuch approved these changes Apr 23, 2019

View reviewed changes

htuch merged commit 4cd0eff into envoyproxy:master Apr 23, 2019

oschaaf added this to the 0.1 milestone May 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add BurstingRateLimiter for simulating super-bursty traffic. #14

Add BurstingRateLimiter for simulating super-bursty traffic. #14

oschaaf commented Apr 18, 2019 •

edited

Loading

oschaaf commented Apr 21, 2019

oschaaf commented Apr 22, 2019

oschaaf commented Apr 22, 2019

oschaaf commented Apr 22, 2019

oschaaf commented Apr 22, 2019

htuch commented Apr 22, 2019

oschaaf commented Apr 22, 2019

htuch left a comment

htuch Apr 22, 2019

htuch Apr 22, 2019

oschaaf Apr 23, 2019

htuch Apr 22, 2019

oschaaf Apr 23, 2019

htuch Apr 22, 2019

htuch Apr 22, 2019

oschaaf Apr 23, 2019

oschaaf commented Apr 23, 2019

htuch left a comment

htuch Apr 23, 2019

htuch Apr 23, 2019

htuch left a comment

Add BurstingRateLimiter for simulating super-bursty traffic. #14

Add BurstingRateLimiter for simulating super-bursty traffic. #14

Conversation

oschaaf commented Apr 18, 2019 • edited Loading

oschaaf commented Apr 21, 2019

oschaaf commented Apr 22, 2019

oschaaf commented Apr 22, 2019

oschaaf commented Apr 22, 2019

oschaaf commented Apr 22, 2019

htuch commented Apr 22, 2019

oschaaf commented Apr 22, 2019

htuch 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

oschaaf commented Apr 23, 2019

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

oschaaf commented Apr 18, 2019 •

edited

Loading