Linear ramping and probabilistic ramping by oschaaf · Pull Request #218 · envoyproxy/nighthawk

oschaaf · 2019-11-27T15:02:03Z

Contains new RateLimiter features:

A linear rate limiter, which ramps up to a frequency over a configurable period.
Precise and relatively easy to reason about
A probabilistic rate limiter filter, which gradually opens up over a configurable period.
Useful for ramping up when wrapping an underlying rate limiter which is non-deterministic,
and possibly in distributed scenarios as well to add some entropy / jitter.

This needs a bit of polish, but ideally we discuss this early so we can cherry pick what we like
from this and possibly discard what we don't.

TODO:

Land rate-limiting: test enhancement + refactoring #223
Make RampingLinearRateLimiter take a ramp duration argument & final frequency to unify the interface with the probabilistic filtering one

In a follow up we can wire up options for these new rate limiters.
(May also be done in a follow-up PR instead).

In conjunction with the upcoming Phases this would allow implementing a better warmup phase, which ramps up to the targeted frequency.

Groundwork for #31

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

…te-limiter

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

…te-limiter

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

Prerequisite to envoyproxy#217 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

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

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

Modulo an improved test for the batching rate limiter, this pulls off some refactoring for upcoming new rate limiters. Split out off from draft PR envoyproxy#218 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

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

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

Modulo an improved test for the batching rate limiter, this pulls off some refactoring for upcoming new rate limiters. This has no functional changes, and is an intermediate step for draft PR #218, which contains new rate limiters (linear & probabilistic ramping, first stab at a zipf distribution support). Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

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

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

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

mum4k

Partial review to start the discussion.

source/common/rate_limiter_impl.h

mum4k · 2019-12-23T02:32:30Z

source/common/rate_limiter_impl.h

+class ZipfRateLimiterImpl : public FilteringRateLimiterImpl {
+public:
+  /**
+   * From the absl header associated to the zipf distribution:


Can we also try to say what q and v are? Either something like "The parameters v and q determine the skew of the distribution." (also copied from the header).

Or copy the relevant portion from the zipf_distribution class'es comment:

zipf_distribution produces random integer-values in the range [0, k], distributed according to the discrete probability function:

P(x) = (v + x) ^ -q

mum4k · 2019-12-23T02:35:00Z

source/common/rate_limiter_impl.h

+   * If NDEBUG is defined and either or both of these parameters take invalid
+   * values, the behavior of the class is undefined.
+   */
+  ZipfRateLimiterImpl(RateLimiterPtr&& rate_limiter, bool deterministic, double q = 2.0,


(optional) Not sure what we prefer in this codebase. I am generally wary of boolean "flag" arguments since they produce client code that is hard to read.

E.g:
ZipfRateLimiterImpl(rate_limiter, true, v, q) // what does true mean?

Of course the client can name it by creating a local variable. We could consider shaping the API in a way that unreadable usage won't be possible, say by defining a well named enum instead.

E.g.:

enum class ZipfBehavior { ZIPF_DETERMINISTIC, ZIPF_NON_DETERMINISTIC }; // Or maybe a bit more transparent: enum class ZipfBehavior { ZIPF_PSEUDO_RANDOM, ZIPF_RANDOM };

WDYT?

I like that; +1

Maybe we should have a place to put code-level guidance like this?

mum4k · 2019-12-23T02:42:44Z

source/common/rate_limiter_impl.h

+/**
+ * Thin wrapper around absl::zipf_distribution that will pull zeroes and ones from the distribution
+ * with the intent to probabilistically suppress the wrapped rate limiter.
+ * This may need further consideration, because it will shoot holes in the pacing, lowering the


Did you have any pre-existing discussions on this with @htuch, is this going to be an issue considering the use cases of this rate limiter? What are the use cases of this rate limiter?

source/common/rate_limiter_impl.h

source/common/rate_limiter_impl.cc

test/rate_limiter_test.cc

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

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

- Back out 1 ns adjustment - Update test to use microsecond step resolution now that we can - Add comment with explanation Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

mum4k

Complete review now.

mum4k · 2019-12-24T00:41:48Z

source/common/rate_limiter_impl.h

 };

+/**
+ * A rate limiter which linearly ramps up to the desired frequency over the specified period.


(nit) Would it make sense to sync the terminology? The comment refers to "the specified period" while the argument is called ramp_time.

mum4k · 2019-12-24T01:44:42Z

source/common/rate_limiter_impl.cc

+                  100.0 - (static_cast<double>(ramp_time_.count() - elapsed().count()) /
+                           (ramp_time_.count() * 1.0)) *
+                              100.0;
+              return std::round(provider_->getValue() / 10000.0) <= chance_percentage;


I am trying to understand the fixed division by 10k. It seems to imply some expectations about the discrete numeric distribution provider that don't seem to be specified on the API.

This may be related to my comment that we should add more documentation next to the constructor for the GraduallyOpeningRateLimiterFilter that will explain the arguments and their roles.

mum4k · 2019-12-24T01:45:53Z

source/common/rate_limiter_impl.cc

 }

+GraduallyOpeningRateLimiterFilter::GraduallyOpeningRateLimiterFilter(
+    const std::chrono::nanoseconds ramp_time, DiscreteNumericDistributionSamplerPtr&& provider,


Should we validate some of the arguments?

mum4k · 2019-12-24T01:49:32Z

test/rate_limiter_test.cc

+  // and the rate limiter computes at nanosecond precision internally. As we
+  // want to have microsecond level precision, this should be more then sufficient.
+  EXPECT_EQ(getAcquisitionTimings(5_Hz, 5s),
+            std::vector<int64_t>({1000050, 1732100, 2236100, 2645800, 3000000, 3316650, 3605600,


Tests apart from testing are often times used by the readers to understand what the code does. (i.e. as additional documentation). Feels like the values that we chose for these tests don't really help the reader. Additionally the unit of these values isn't obvious since the last change to micro-seconds.

I am wondering if we could choose different parameters than 5Hz over 5sec or maybe add test cases with other parameters that will be easier / more obvious for the reader to comprehend. We don't want to decrease test coverage, but we should aim towards self-documenting and readable test cases. Can we try to find a balance?

@mum4k I couldn't really obtain easy-to-grok numbers by picking different parameter values here; but how about this: 9d248f2 ?

Instead of matching expectations against hard-coded numbers, that uses the second law of motion to figure out control acquisition timings for comparison, and adds a little slack to when verifying expectations with a comment explaining the need for that. I'm hoping that will help future readers of this code. wdyt, will this be better?

(As a side-effect, this approach also allowed adding tests with higher frequencies / duration arguments)

mum4k · 2019-12-24T01:53:39Z

source/common/rate_limiter_impl.cc

  return rate_limiter_->tryAcquireOne() ? filter_() : false;
 }

+GraduallyOpeningRateLimiterFilter::GraduallyOpeningRateLimiterFilter(


(fyi) Now that I read the code and understand the implementation here, I have to say that this is truly beautiful, including your use of class inheritance for the ForwardingRateLimiter.

I have learned something today and I appreciate it, thanks.

That is nice to hear, thanks!

- Check expectations on the distribution argument to GraduallyOpeningRateLimiterFilterTest - Clean up the computation in GraduallyOpeningRateLimiterFilter - Add a bunch of explanatory comments What's left is getting easy to grok numbers in test expectations. Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Instead of matching hard-coded expectations for release timings, use the second law of motion as a control method for verifying the acquisition timings the rate limiter. Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

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

oschaaf · 2019-12-29T15:04:43Z

@mum4k I think I addressed all comments, so marking this as ready-for-review. Thanks for the review!

mum4k · 2020-01-03T00:12:05Z

include/nighthawk/common/rate_limiter.h

 public:
  virtual ~DiscreteNumericDistributionSampler() = default;
  virtual uint64_t getValue() PURE;
+  virtual uint64_t min() const PURE;


Can we document the new methods?

(optional / unrelated) Since we are here, can we document the entire interface?

mum4k · 2020-01-03T00:17:08Z

source/common/rate_limiter_impl.cc

+                  1.0 - static_cast<double>(ramp_time_.count() - elapsed_time.count()) /
+                            (ramp_time_.count() * 1.0);
+              // Get a random number r, where 0 < r ≤ 1.
+              const double random_between_0_and_1 = 1.0 * provider_->getValue() / provider_->max();


Thank you, this is easier to follow with the new comments and the min() and max() methods.

mum4k · 2020-01-03T00:24:14Z

test/rate_limiter_test.cc

 public:
-  std::vector<int64_t> getAcquisitionTimings(const Frequency frequency,
-                                             const std::chrono::seconds duration) {
+  void checkAcquisitionTimings(const Frequency frequency, const std::chrono::seconds duration) {


Can we document this helper? Specifically its arguments.

mum4k · 2020-01-03T00:25:33Z

test/rate_limiter_test.cc

-                                  3873000, 4123150, 4358900, 4582600, 4795850, 5000000}));
-  EXPECT_EQ(getAcquisitionTimings(4_Hz, 2s),
-            std::vector<int64_t>({707150, 1224750, 1581150, 1870850}));
+  checkAcquisitionTimings(1_Hz, 3s);


This is indeed much better for readability, but i am debating with myself whether it doesn't decrease our coverage. Could there later be a bug that would go undetected? Should we add at least one test case that verifies the exact values as before? WDYT?

yeah this makes sense, done (f480248)

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

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

oschaaf · 2020-01-03T12:13:10Z

@mum4k added f480248 based on your feedback, thanks!

oschaaf added 16 commits November 16, 2019 23:54

save state on ramping rate limiter

90bd582

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

Merge remote-tracking branch 'upstream/master' into ramping-linear-ra…

bf5ae1d

…te-limiter

Merge remote-tracking branch 'upstream/master' into ramping-linear-ra…

28011ef

…te-limiter

Merge remote-tracking branch 'upstream/master' into ramping-linear-ra…

be93d1a

…te-limiter

Add tests, clean up

76158b9

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

Refactor + LinearlyOpeningRateLimiterFilter

d29c95c

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

save state

ebd857c

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

save state

1d62a00

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

Back out change to constness in Frequency

c86cb92

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

Save state, wire in zipf / foo ZipfRateLimiter

7fc2992

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

Some comments & tidying up

9def3cd

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

Merge remote-tracking branch 'upstream/master' into ramping-linear-ra…

abda9db

…te-limiter

small cleanup

4c54837

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

Update Envoy dep for access to enableHRTimer()

7883d59

Prerequisite to envoyproxy#217 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Whoops, amend bad copying

e458ea5

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

Also, sync .bazelrc while at it

e7b65c1

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

oschaaf changed the title ~~[DRAFT] Ramping linear rate limiter~~ [DRAFT] Rate limiter enhancements & features Dec 6, 2019

Merge remote-tracking branch 'upstream/master' into ramping-linear-ra…

a8c94d8

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

oschaaf mentioned this pull request Dec 6, 2019

rate-limiting: test enhancement + refactoring #223

Merged

oschaaf added 2 commits December 11, 2019 11:01

Review feedback

cc4fead

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

Merge remote-tracking branch 'upstream/master' into ramping-linear-ra…

ea43285

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

oschaaf added 7 commits December 12, 2019 08:28

Merge remote-tracking branch 'upstream/master' into ramping-linear-ra…

4e7eb0a

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

Clean up the diff

a69d811

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

Merge remote-tracking branch 'upstream/master' into update-envoy-dep-8

b5b5fd2

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

Update Envoy to the latest

fb85732

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

Move to the sha that has our target

8a268fe

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

Unbreak it

34f649b

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

Merge remote-tracking branch 'origin/update-envoy-dep-8' into ramping…

54aa937

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

mum4k requested changes Dec 23, 2019

View reviewed changes

mum4k 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 Dec 23, 2019

oschaaf added 2 commits December 23, 2019 11:51

Merge remote-tracking branch 'upstream/master' into ramping-linear-ra…

fe7bb0c

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

Remove zipf stuff

324a4ee

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

oschaaf mentioned this pull request Dec 23, 2019

Add zipf distributing ratelimiter #251

Merged

oschaaf changed the title ~~Linear ramping, probabilistic ramping, Zipf distributing~~ Linear ramping and probabilistic ramping Dec 23, 2019

Review feedback

6329377

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 Dec 23, 2019

Review: 1ns adjustment

5cfa171

- Back out 1 ns adjustment - Update test to use microsecond step resolution now that we can - Add comment with explanation Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

mum4k requested changes Dec 24, 2019

View reviewed changes

mum4k 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 Dec 24, 2019

oschaaf added 3 commits December 26, 2019 00:52

Review-feedback: use second law of motion

9d248f2

Instead of matching hard-coded expectations for release timings, use the second law of motion as a control method for verifying the acquisition timings the rate limiter. Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

clang-tidy: fix complaint

183677c

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 Dec 29, 2019

mum4k requested changes Jan 3, 2020

View reviewed changes

mum4k 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 3, 2020

oschaaf added 2 commits January 3, 2020 12:29

Merge remote-tracking branch 'upstream/master' into ramping-linear-ra…

d79f2a2

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

Review feedback

f480248

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 3, 2020

mum4k approved these changes Jan 3, 2020

View reviewed changes

mum4k merged commit 9b45e0c into envoyproxy:master Jan 3, 2020

Conversation

oschaaf commented Nov 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mum4k left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mum4k 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!

oschaaf commented Dec 29, 2019

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!

oschaaf commented Jan 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

oschaaf commented Nov 27, 2019 •

edited

Loading