-
Notifications
You must be signed in to change notification settings - Fork 94
Linear ramping and probabilistic ramping #218
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 35 commits
90bd582
bf5ae1d
28011ef
be93d1a
76158b9
d29c95c
ebd857c
1d62a00
c86cb92
7fc2992
9def3cd
abda9db
4c54837
7883d59
e458ea5
e7b65c1
a8c94d8
cc4fead
ea43285
4e7eb0a
a69d811
b5b5fd2
fb85732
8a268fe
34f649b
54aa937
f34708f
58a7566
1f90162
e1fa16d
af45a0a
5c8637c
89557e8
79b641c
bec3b95
fe7bb0c
324a4ee
6329377
5cfa171
31e4cbe
9d248f2
183677c
d79f2a2
f480248
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -78,6 +78,43 @@ void LinearRateLimiter::releaseOne() { | |
| acquired_count_--; | ||
| } | ||
|
|
||
| LinearRampingRateLimiterImpl::LinearRampingRateLimiterImpl(Envoy::TimeSource& time_source, | ||
| const std::chrono::nanoseconds ramp_time, | ||
| const Frequency frequency) | ||
| : RateLimiterBaseImpl(time_source), ramp_time_(ramp_time), frequency_(frequency) { | ||
| if (frequency_.value() <= 0) { | ||
| throw NighthawkException("frequency must be > 0"); | ||
| } | ||
| if (ramp_time <= 0ns) { | ||
| throw NighthawkException("duration must be positive"); | ||
| } | ||
| } | ||
|
|
||
| bool LinearRampingRateLimiterImpl::tryAcquireOne() { | ||
| if (acquireable_count_) { | ||
| acquired_count_++; | ||
| return acquireable_count_--; | ||
| } | ||
| const auto elapsed_time = elapsed() + 1ns; | ||
|
mum4k marked this conversation as resolved.
Outdated
mum4k marked this conversation as resolved.
Outdated
|
||
| double elapsed_fraction = 1.0; | ||
| if (elapsed_time < ramp_time_) { | ||
| elapsed_fraction -= static_cast<double>(ramp_time_.count() - elapsed_time.count()) / | ||
| static_cast<double>(ramp_time_.count()); | ||
| } | ||
|
|
||
| const double current_frequency = elapsed_fraction * (frequency_.value() * 1.0); | ||
| const double chrono_seconds = | ||
| std::chrono::duration_cast<std::chrono::duration<double>>(elapsed_time).count(); | ||
| const double total = chrono_seconds * current_frequency / 2.0; | ||
|
mum4k marked this conversation as resolved.
Outdated
|
||
| acquireable_count_ = std::round(total) - acquired_count_; | ||
| return acquireable_count_ > 0 ? tryAcquireOne() : false; | ||
| } | ||
|
|
||
| void LinearRampingRateLimiterImpl::releaseOne() { | ||
| acquireable_count_++; | ||
| acquired_count_--; | ||
| } | ||
|
|
||
| DelegatingRateLimiterImpl::DelegatingRateLimiterImpl( | ||
| RateLimiterPtr&& rate_limiter, RateLimiterDelegate random_distribution_generator) | ||
| : ForwardingRateLimiterImpl(std::move(rate_limiter)), | ||
|
|
@@ -118,4 +155,27 @@ bool FilteringRateLimiterImpl::tryAcquireOne() { | |
| return rate_limiter_->tryAcquireOne() ? filter_() : false; | ||
| } | ||
|
|
||
| GraduallyOpeningRateLimiterFilter::GraduallyOpeningRateLimiterFilter( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is nice to hear, thanks! |
||
| const std::chrono::nanoseconds ramp_time, DiscreteNumericDistributionSamplerPtr&& provider, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we validate some of the arguments? |
||
| RateLimiterPtr&& rate_limiter) | ||
| : FilteringRateLimiterImpl( | ||
| std::move(rate_limiter), | ||
| [this]() { | ||
| if (elapsed() < ramp_time_) { | ||
| const double chance_percentage = | ||
| 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; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| } | ||
| return true; | ||
| }), | ||
| provider_(std::move(provider)), ramp_time_(ramp_time) {} | ||
|
|
||
| ZipfRateLimiterImpl::ZipfRateLimiterImpl(RateLimiterPtr&& rate_limiter, bool deterministic, | ||
| double q, double v) | ||
| : FilteringRateLimiterImpl(std::move(rate_limiter), | ||
| [this]() { return deterministic_ ? dist_(mt_) : dist_(g_); }), | ||
| dist_(1, q, v), deterministic_(deterministic) {} | ||
|
|
||
| } // namespace Nighthawk | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,8 @@ | |
|
|
||
| #include "common/frequency.h" | ||
|
|
||
| #include "absl/random/random.h" | ||
| #include "absl/random/zipf_distribution.h" | ||
| #include "absl/types/optional.h" | ||
|
|
||
| namespace Nighthawk { | ||
|
|
@@ -55,6 +57,24 @@ class LinearRateLimiter : public RateLimiterBaseImpl, | |
| const Frequency frequency_; | ||
| }; | ||
|
|
||
| /** | ||
| * A rate limiter which linearly ramps up to the desired frequency over the specified period. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (nit) Would it make sense to sync the terminology? The comment refers to "the specified period" while the argument is called ramp_time. |
||
| */ | ||
| class LinearRampingRateLimiterImpl : public RateLimiterBaseImpl, | ||
| public Envoy::Logger::Loggable<Envoy::Logger::Id::main> { | ||
| public: | ||
| LinearRampingRateLimiterImpl(Envoy::TimeSource& time_source, | ||
| const std::chrono::nanoseconds ramp_time, const Frequency frequency); | ||
| bool tryAcquireOne() override; | ||
| void releaseOne() override; | ||
|
|
||
| private: | ||
| int64_t acquireable_count_{0}; | ||
| uint64_t acquired_count_{0}; | ||
| const std::chrono::nanoseconds ramp_time_; | ||
| const Frequency frequency_; | ||
| }; | ||
|
|
||
| /** | ||
| * Base for a rate limiter which wraps another rate limiter, and forwards | ||
| * some calls. | ||
|
|
@@ -157,4 +177,44 @@ class FilteringRateLimiterImpl : public ForwardingRateLimiterImpl, | |
| const RateLimiterFilter filter_; | ||
| }; | ||
|
|
||
| /** | ||
| * Takes a probabilistic approach to suppressing an arbitrary wrapper rate limiter. | ||
|
mum4k marked this conversation as resolved.
Outdated
|
||
| */ | ||
| class GraduallyOpeningRateLimiterFilter : public FilteringRateLimiterImpl { | ||
| public: | ||
| GraduallyOpeningRateLimiterFilter(const std::chrono::nanoseconds ramp_time, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add some description of the arguments and how they affect the functionality?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe this comment still applies after the latest changes. Maybe it got lost? |
||
| DiscreteNumericDistributionSamplerPtr&& provider, | ||
| RateLimiterPtr&& rate_limiter); | ||
|
|
||
| private: | ||
| DiscreteNumericDistributionSamplerPtr provider_; | ||
| const std::chrono::nanoseconds ramp_time_; | ||
| }; | ||
|
|
||
| /** | ||
| * 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 | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
| * actual achieved frequency. | ||
| */ | ||
| class ZipfRateLimiterImpl : public FilteringRateLimiterImpl { | ||
|
mum4k marked this conversation as resolved.
Outdated
|
||
| public: | ||
| /** | ||
| * From the absl header associated to the zipf distribution: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we also try to say what 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 |
||
| * Preconditions: v > 0, q > 1 | ||
| * The precondidtions are validated when NDEBUG is not defined via | ||
| * a pair of assert() directives. | ||
| * If NDEBUG is defined and either or both of these parameters take invalid | ||
| * values, the behavior of the class is undefined. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to hope that the users will read the comment or will it be better to throw when the values aren't in the expected range? |
||
| */ | ||
| ZipfRateLimiterImpl(RateLimiterPtr&& rate_limiter, bool deterministic, double q = 2.0, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (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: 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?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like that; +1
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we should have a place to put code-level guidance like this? |
||
| double v = 1.0); | ||
|
|
||
| private: | ||
| absl::zipf_distribution<uint64_t> dist_; | ||
| absl::InsecureBitGen g_; | ||
| std::mt19937_64 mt_; | ||
| bool deterministic_; | ||
| }; | ||
|
|
||
| } // namespace Nighthawk | ||
Uh oh!
There was an error while loading. Please reload this page.