-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
adaptive concurrency: Gradient algorithm implementation #7908
Changes from 32 commits
3de83c1
1e29caa
372ef8b
57ecd6d
922575e
b54f3ce
6ac38af
94c89bd
1523555
b434bb8
c2a9684
5b9ba6c
d749973
e304699
fc6891e
a671b60
735536a
b9a8ee4
270961e
3d052f5
32cd018
270187d
f111452
676f2e3
87d59cc
fd2cc83
8df135e
b51e5e8
d399b6c
2f8795c
92d7583
2c94909
aba292d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,64 @@ | ||
syntax = "proto3"; | ||
|
||
package envoy.config.filter.http.adaptive_concurrency.v3alpha; | ||
package envoy.config.filter.http.adaptive_concurrency.v2alpha; | ||
|
||
option java_package = "io.envoyproxy.envoy.config.filter.http.adaptive_concurrency.v3alpha"; | ||
option java_package = "io.envoyproxy.envoy.config.filter.http.adaptive_concurrency.v2alpha"; | ||
option java_outer_classname = "AdaptiveConcurrencyProto"; | ||
option java_multiple_files = true; | ||
|
||
import "envoy/type/percent.proto"; | ||
|
||
import "google/protobuf/duration.proto"; | ||
import "google/api/annotations.proto"; | ||
import "google/protobuf/wrappers.proto"; | ||
|
||
import "validate/validate.proto"; | ||
|
||
// Configuration parameters for the gradient controller. | ||
message GradientControllerConfig { | ||
// The percentile to use when summarizing aggregated samples. Defaults to p50. | ||
envoy.type.Percent sample_aggregate_percentile = 1; | ||
|
||
// Parameters controlling the periodic recalculation of the concurrency limit from sampled request | ||
// latencies. | ||
message ConcurrencyLimitCalculationParams { | ||
// The maximum value the gradient is allowed to take. This influences how aggressively the | ||
// concurrency limit can increase. Defaults to 2.0. | ||
google.protobuf.DoubleValue max_gradient = 1 [(validate.rules).double.gt = 1.0]; | ||
|
||
// The allowed upper-bound on the calculated concurrency limit. Defaults to 1000. | ||
google.protobuf.UInt32Value max_concurrency_limit = 2 [(validate.rules).uint32.gt = 0]; | ||
|
||
// The period of time samples are taken to recalculate the concurrency limit. | ||
google.protobuf.Duration concurrency_update_interval = 3 [(validate.rules).duration = { | ||
required: true, | ||
gt: {seconds: 0} | ||
}]; | ||
} | ||
ConcurrencyLimitCalculationParams concurrency_limit_params = 2 | ||
[(validate.rules).message.required = true]; | ||
|
||
// Parameters controlling the periodic minRTT recalculation. | ||
message MinimumRTTCalculationParams { | ||
// The time interval between recalculating the minimum request round-trip time. | ||
google.protobuf.Duration interval = 1 [(validate.rules).duration = { | ||
required: true, | ||
gt: {seconds: 0} | ||
}]; | ||
|
||
// The number of requests to aggregate/sample during the minRTT recalculation window before | ||
// updating. Defaults to 50. | ||
google.protobuf.UInt32Value request_count = 2 [(validate.rules).uint32.gt = 0]; | ||
}; | ||
MinimumRTTCalculationParams min_rtt_calc_params = 3 [(validate.rules).message.required = true]; | ||
} | ||
|
||
message AdaptiveConcurrency { | ||
oneof concurrency_controller_config { | ||
option (validate.required) = true; | ||
|
||
// Gradient concurrency control will be used. | ||
GradientControllerConfig gradient_controller_config = 1 | ||
[(validate.rules).message.required = true]; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,13 +32,28 @@ Http::FilterHeadersStatus AdaptiveConcurrencyFilter::decodeHeaders(Http::HeaderM | |
return Http::FilterHeadersStatus::StopIteration; | ||
} | ||
|
||
rq_start_time_ = config_->timeSource().monotonicTime(); | ||
// When the deferred_sample_task_ object is destroyed, the time difference between its destruction | ||
// and the request start time is measured as the request latency. This value is sampled by the | ||
// concurrency controller either when encoding is complete or during destruction of this filter | ||
// object. | ||
deferred_sample_task_ = | ||
std::make_unique<Cleanup>([this, rq_start_time = config_->timeSource().monotonicTime()]() { | ||
const auto now = config_->timeSource().monotonicTime(); | ||
const std::chrono::nanoseconds rq_latency = now - rq_start_time; | ||
controller_->recordLatencySample(rq_latency); | ||
}); | ||
|
||
return Http::FilterHeadersStatus::Continue; | ||
} | ||
|
||
void AdaptiveConcurrencyFilter::encodeComplete() { | ||
const auto rq_latency = config_->timeSource().monotonicTime() - rq_start_time_; | ||
controller_->recordLatencySample(rq_latency); | ||
ASSERT(deferred_sample_task_); | ||
deferred_sample_task_.reset(); | ||
} | ||
|
||
void AdaptiveConcurrencyFilter::onDestroy() { | ||
deferred_sample_task_->cancel(); | ||
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. Unfortunately this is still not right. Also, I still think this pattern is a bit error prone and you would be better off just returning an |
||
controller_->cancelLatencySample(); | ||
} | ||
|
||
} // namespace AdaptiveConcurrency | ||
|
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 you don't end up reverting this change because of the TODO we discussed, can you add an explicit test for this somewhere in common tests in a follow up? If we keep this code I might also use an absl::optional for the functor instead of an explicit cancelled_ boolean, but that's a minor thing.
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.
Both this and the protobuf utility had explicit tests in
test/common/common/cleanup_test.cc
andtest/common/protobuf/utility_test.cc
.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.
Cool thanks, I missed that.