-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
add rps_threshold & max_rejection_probability #16742
Changes from 5 commits
b5b8523
3ee18a3
543d3ab
80668e7
68e3535
7c508c1
0cd8d14
363a387
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,6 +31,8 @@ using GrpcStatus = Grpc::Status::GrpcStatus; | |
|
||
static constexpr double defaultAggression = 1.0; | ||
static constexpr double defaultSuccessRateThreshold = 95.0; | ||
static constexpr uint32_t defaultRpsThreshold = 0; | ||
static constexpr double defaultMaxRejectionProbability = 80.0; | ||
|
||
AdmissionControlFilterConfig::AdmissionControlFilterConfig( | ||
const AdmissionControlProto& proto_config, Runtime::Loader& runtime, | ||
|
@@ -45,6 +47,13 @@ AdmissionControlFilterConfig::AdmissionControlFilterConfig( | |
sr_threshold_(proto_config.has_sr_threshold() ? std::make_unique<Runtime::Percentage>( | ||
proto_config.sr_threshold(), runtime) | ||
: nullptr), | ||
rps_threshold_(proto_config.has_rps_threshold() | ||
? std::make_unique<Runtime::UInt32>(proto_config.rps_threshold(), runtime) | ||
: nullptr), | ||
max_rejection_probability_(proto_config.has_max_rejection_probability() | ||
? std::make_unique<Runtime::Percentage>( | ||
proto_config.max_rejection_probability(), runtime) | ||
: nullptr), | ||
response_evaluator_(std::move(response_evaluator)) {} | ||
|
||
double AdmissionControlFilterConfig::aggression() const { | ||
|
@@ -56,6 +65,16 @@ double AdmissionControlFilterConfig::successRateThreshold() const { | |
return std::min<double>(pct, 100.0) / 100.0; | ||
} | ||
|
||
uint32_t AdmissionControlFilterConfig::rpsThreshold() const { | ||
return rps_threshold_ ? rps_threshold_->value() : defaultRpsThreshold; | ||
} | ||
|
||
double AdmissionControlFilterConfig::maxRejectionProbability() const { | ||
const double ret = max_rejection_probability_ ? max_rejection_probability_->value() | ||
: defaultMaxRejectionProbability; | ||
return ret / 100.0; | ||
} | ||
|
||
AdmissionControlFilter::AdmissionControlFilter(AdmissionControlFilterConfigSharedPtr config, | ||
const std::string& stats_prefix) | ||
: config_(std::move(config)), stats_(generateStats(config_->scope(), stats_prefix)), | ||
|
@@ -68,6 +87,11 @@ Http::FilterHeadersStatus AdmissionControlFilter::decodeHeaders(Http::RequestHea | |
return Http::FilterHeadersStatus::Continue; | ||
} | ||
|
||
if (config_->getController().averageRps() < config_->rpsThreshold()) { | ||
ENVOY_LOG(debug, "Current rps: {} is below rps_threshold: {}, continue"); | ||
return Http::FilterHeadersStatus::Continue; | ||
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. Consider throwing a debug log in here. |
||
} | ||
|
||
if (shouldRejectRequest()) { | ||
// We do not want to sample requests that we are rejecting, since this taints the measurements | ||
// that should be describing the upstreams. In addition, if we were to record the requests | ||
|
@@ -148,6 +172,7 @@ bool AdmissionControlFilter::shouldRejectRequest() const { | |
if (aggression != 1.0) { | ||
probability = std::pow(probability, 1.0 / aggression); | ||
} | ||
probability = std::min<double>(probability, config_->maxRejectionProbability()); | ||
|
||
// Choosing an accuracy of 4 significant figures for the probability. | ||
static constexpr uint64_t accuracy = 1e4; | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -17,6 +17,16 @@ ThreadLocalControllerImpl::ThreadLocalControllerImpl(TimeSource& time_source, | |||||||||||||||||
std::chrono::seconds sampling_window) | ||||||||||||||||||
: time_source_(time_source), sampling_window_(sampling_window) {} | ||||||||||||||||||
|
||||||||||||||||||
uint32_t ThreadLocalControllerImpl::averageRps() const { | ||||||||||||||||||
if (historical_data_.empty() || global_data_.requests == 0) { | ||||||||||||||||||
return 0; | ||||||||||||||||||
} | ||||||||||||||||||
using namespace std::chrono; | ||||||||||||||||||
auto count_of_seconds = duration_cast<seconds>(ageOfOldestSample()).count(); | ||||||||||||||||||
|
||||||||||||||||||
return count_of_seconds == 0 ? 0 : global_data_.requests / count_of_seconds; | ||||||||||||||||||
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 think you can just do something like below. There's just less going on that way IMO.
Suggested change
|
||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
void ThreadLocalControllerImpl::maybeUpdateHistoricalData() { | ||||||||||||||||||
// Purge stale samples. | ||||||||||||||||||
while (!historical_data_.empty() && ageOfOldestSample() >= sampling_window_) { | ||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is true and these values are clamped, can you make this more clear in the API docs? If it's not true can you make it more clear here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't true all the time-- it's just explaining how to read the configuration above. Github cut out this line just above the additions:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah OK sorry, I missed that, thanks.