-
Notifications
You must be signed in to change notification settings - Fork 5.5k
[Bandwidth-Limit] Add response trailers for bandwidth delay #18267
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 15 commits
d657bc6
f27b1c7
0e063bf
fee39ec
13bd624
3eabf33
e7cc99e
a35a303
c1c73b3
77926bb
938c352
a492d9d
70fd09b
445f82d
e4d2ae2
ff204cc
36443a1
e29061d
fbd36a0
b9cea1d
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 | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -16,14 +16,30 @@ namespace Extensions { | |||||||||
| namespace HttpFilters { | ||||||||||
| namespace BandwidthLimitFilter { | ||||||||||
|
|
||||||||||
| namespace { | ||||||||||
| const Http::LowerCaseString DefaultRequestDelayTrailer = | ||||||||||
| Http::LowerCaseString("bandwidth-request-delay-ms"); | ||||||||||
| const Http::LowerCaseString DefaultResponseDelayTrailer = | ||||||||||
| Http::LowerCaseString("bandwidth-response-delay-ms"); | ||||||||||
| } // namespace | ||||||||||
|
|
||||||||||
| FilterConfig::FilterConfig(const BandwidthLimit& config, Stats::Scope& scope, | ||||||||||
| Runtime::Loader& runtime, TimeSource& time_source, bool per_route) | ||||||||||
| : runtime_(runtime), time_source_(time_source), enable_mode_(config.enable_mode()), | ||||||||||
| limit_kbps_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, limit_kbps, 0)), | ||||||||||
| fill_interval_(std::chrono::milliseconds(PROTOBUF_GET_MS_OR_DEFAULT( | ||||||||||
| config, fill_interval, StreamRateLimiter::DefaultFillInterval.count()))), | ||||||||||
| enabled_(config.runtime_enabled(), runtime), | ||||||||||
| stats_(generateStats(config.stat_prefix(), scope)) { | ||||||||||
| stats_(generateStats(config.stat_prefix(), scope)), | ||||||||||
| request_delay_trailer_(config.response_trailer_prefix().empty() | ||||||||||
| ? DefaultRequestDelayTrailer | ||||||||||
| : Http::LowerCaseString(config.response_trailer_prefix() + "-" + | ||||||||||
|
Contributor
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.
Contributor
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. strcat seems can't concatenate two const strings, maybe another way I don't know.
Contributor
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. See suggestion below |
||||||||||
| DefaultRequestDelayTrailer.get())), | ||||||||||
| response_delay_trailer_(config.response_trailer_prefix().empty() | ||||||||||
| ? DefaultResponseDelayTrailer | ||||||||||
| : Http::LowerCaseString(config.response_trailer_prefix() + "-" + | ||||||||||
| DefaultResponseDelayTrailer.get())), | ||||||||||
|
Contributor
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.
Suggested change
Contributor
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. fixed, thanks. |
||||||||||
| enable_response_trailers_(config.enable_response_trailers()) { | ||||||||||
| if (per_route && !config.has_limit_kbps()) { | ||||||||||
| throw EnvoyException("bandwidthlimitfilter: limit must be set for per route filter config"); | ||||||||||
| } | ||||||||||
|
|
@@ -64,7 +80,12 @@ Http::FilterHeadersStatus BandwidthLimiter::decodeHeaders(Http::RequestHeaderMap | |||||||||
| updateStatsOnDecodeFinish(); | ||||||||||
| decoder_callbacks_->continueDecoding(); | ||||||||||
| }, | ||||||||||
| [config](uint64_t len) { config.stats().request_allowed_size_.set(len); }, | ||||||||||
| [&config](uint64_t len, bool limit_enforced) { | ||||||||||
| config.stats().request_allowed_size_.add(len); | ||||||||||
| if (limit_enforced) { | ||||||||||
| config.stats().request_enforced_.inc(); | ||||||||||
| } | ||||||||||
| }, | ||||||||||
| const_cast<FilterConfig*>(&config)->timeSource(), decoder_callbacks_->dispatcher(), | ||||||||||
| decoder_callbacks_->scope(), config.tokenBucket(), config.fillInterval()); | ||||||||||
| } | ||||||||||
|
|
@@ -82,7 +103,7 @@ Http::FilterDataStatus BandwidthLimiter::decodeData(Buffer::Instance& data, bool | |||||||||
| const_cast<FilterConfig*>(&config)->timeSource()); | ||||||||||
| config.stats().request_pending_.inc(); | ||||||||||
| } | ||||||||||
| config.stats().request_incoming_size_.set(data.length()); | ||||||||||
| config.stats().request_incoming_size_.add(data.length()); | ||||||||||
|
|
||||||||||
| request_limiter_->writeData(data, end_stream); | ||||||||||
| return Http::FilterDataStatus::StopIterationNoBuffer; | ||||||||||
|
|
@@ -123,7 +144,12 @@ Http::FilterHeadersStatus BandwidthLimiter::encodeHeaders(Http::ResponseHeaderMa | |||||||||
| updateStatsOnEncodeFinish(); | ||||||||||
| encoder_callbacks_->continueEncoding(); | ||||||||||
| }, | ||||||||||
| [config](uint64_t len) { config.stats().response_allowed_size_.set(len); }, | ||||||||||
| [&config](uint64_t len, bool limit_enforced) { | ||||||||||
| config.stats().response_allowed_size_.add(len); | ||||||||||
| if (limit_enforced) { | ||||||||||
| config.stats().response_enforced_.inc(); | ||||||||||
| } | ||||||||||
| }, | ||||||||||
| const_cast<FilterConfig*>(&config)->timeSource(), encoder_callbacks_->dispatcher(), | ||||||||||
| encoder_callbacks_->scope(), config.tokenBucket(), config.fillInterval()); | ||||||||||
| } | ||||||||||
|
|
@@ -135,23 +161,34 @@ Http::FilterDataStatus BandwidthLimiter::encodeData(Buffer::Instance& data, bool | |||||||||
| if (response_limiter_ != nullptr) { | ||||||||||
| const auto& config = getConfig(); | ||||||||||
|
|
||||||||||
| // Adds encoded trailers. May only be called in encodeData when end_stream is set to true. | ||||||||||
| // If upstream has trailers, addEncodedTrailers won't be called | ||||||||||
| bool trailer_added = false; | ||||||||||
| if (end_stream) { | ||||||||||
|
Contributor
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. This should also check
Contributor
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. fixed, thanks. |
||||||||||
| trailers_ = &encoder_callbacks_->addEncodedTrailers(); | ||||||||||
| trailer_added = true; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| if (!response_latency_) { | ||||||||||
| response_latency_ = std::make_unique<Stats::HistogramCompletableTimespanImpl>( | ||||||||||
| config.stats().response_transfer_duration_, | ||||||||||
| const_cast<FilterConfig*>(&config)->timeSource()); | ||||||||||
| config.stats().response_pending_.inc(); | ||||||||||
| } | ||||||||||
| config.stats().response_incoming_size_.set(data.length()); | ||||||||||
| config.stats().response_incoming_size_.add(data.length()); | ||||||||||
|
|
||||||||||
| response_limiter_->writeData(data, end_stream); | ||||||||||
| response_limiter_->writeData(data, end_stream, trailer_added); | ||||||||||
| return Http::FilterDataStatus::StopIterationNoBuffer; | ||||||||||
| } | ||||||||||
| ENVOY_LOG(debug, "BandwidthLimiter <encode data>: response_limiter not set"); | ||||||||||
| return Http::FilterDataStatus::Continue; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| Http::FilterTrailersStatus BandwidthLimiter::encodeTrailers(Http::ResponseTrailerMap&) { | ||||||||||
| Http::FilterTrailersStatus | ||||||||||
| BandwidthLimiter::encodeTrailers(Http::ResponseTrailerMap& response_trailers) { | ||||||||||
| if (response_limiter_ != nullptr) { | ||||||||||
| trailers_ = &response_trailers; | ||||||||||
|
|
||||||||||
| if (response_limiter_->onTrailers()) { | ||||||||||
| return Http::FilterTrailersStatus::StopIteration; | ||||||||||
| } else { | ||||||||||
|
|
@@ -164,6 +201,7 @@ Http::FilterTrailersStatus BandwidthLimiter::encodeTrailers(Http::ResponseTraile | |||||||||
|
|
||||||||||
| void BandwidthLimiter::updateStatsOnDecodeFinish() { | ||||||||||
| if (request_latency_) { | ||||||||||
| request_duration_ = request_latency_.get()->elapsed().count(); | ||||||||||
| request_latency_->complete(); | ||||||||||
| request_latency_.reset(); | ||||||||||
| getConfig().stats().request_pending_.dec(); | ||||||||||
|
|
@@ -172,9 +210,21 @@ void BandwidthLimiter::updateStatsOnDecodeFinish() { | |||||||||
|
|
||||||||||
| void BandwidthLimiter::updateStatsOnEncodeFinish() { | ||||||||||
| if (response_latency_) { | ||||||||||
| const auto& config = getConfig(); | ||||||||||
|
|
||||||||||
| if (config.enableResponseTrailers() && trailers_ != nullptr) { | ||||||||||
| auto response_duration = response_latency_.get()->elapsed().count(); | ||||||||||
| if (request_duration_ > 0) { | ||||||||||
| trailers_->setCopy(config.requestDelayTrailer(), std::to_string(request_duration_)); | ||||||||||
| } | ||||||||||
| if (response_duration > 0) { | ||||||||||
| trailers_->setCopy(config.responseDelayTrailer(), std::to_string(response_duration)); | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| response_latency_->complete(); | ||||||||||
| response_latency_.reset(); | ||||||||||
| getConfig().stats().response_pending_.dec(); | ||||||||||
| config.stats().response_pending_.dec(); | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,12 +32,14 @@ namespace BandwidthLimitFilter { | |
| #define ALL_BANDWIDTH_LIMIT_STATS(COUNTER, GAUGE, HISTOGRAM) \ | ||
| COUNTER(request_enabled) \ | ||
| COUNTER(response_enabled) \ | ||
| COUNTER(request_enforced) \ | ||
| COUNTER(response_enforced) \ | ||
| GAUGE(request_pending, Accumulate) \ | ||
| GAUGE(response_pending, Accumulate) \ | ||
| GAUGE(request_incoming_size, Accumulate) \ | ||
| GAUGE(response_incoming_size, Accumulate) \ | ||
| GAUGE(request_allowed_size, Accumulate) \ | ||
| GAUGE(response_allowed_size, Accumulate) \ | ||
| COUNTER(request_incoming_size) \ | ||
| COUNTER(response_incoming_size) \ | ||
| COUNTER(request_allowed_size) \ | ||
| COUNTER(response_allowed_size) \ | ||
| HISTOGRAM(request_transfer_duration, Milliseconds) \ | ||
| HISTOGRAM(response_transfer_duration, Milliseconds) | ||
|
|
||
|
|
@@ -70,6 +72,9 @@ class FilterConfig : public ::Envoy::Router::RouteSpecificFilterConfig { | |
| EnableMode enableMode() const { return enable_mode_; }; | ||
| const std::shared_ptr<SharedTokenBucketImpl> tokenBucket() const { return token_bucket_; } | ||
| std::chrono::milliseconds fillInterval() const { return fill_interval_; } | ||
| const Http::LowerCaseString& requestDelayTrailer() const { return request_delay_trailer_; } | ||
| const Http::LowerCaseString& responseDelayTrailer() const { return response_delay_trailer_; } | ||
| bool enableResponseTrailers() const { return enable_response_trailers_; } | ||
|
|
||
| private: | ||
| friend class FilterTest; | ||
|
|
@@ -85,6 +90,9 @@ class FilterConfig : public ::Envoy::Router::RouteSpecificFilterConfig { | |
| mutable BandwidthLimitStats stats_; | ||
| // Filter chain's shared token bucket | ||
| std::shared_ptr<SharedTokenBucketImpl> token_bucket_; | ||
| const Http::LowerCaseString request_delay_trailer_; | ||
| const Http::LowerCaseString response_delay_trailer_; | ||
| const bool enable_response_trailers_; | ||
| }; | ||
|
|
||
| using FilterConfigSharedPtr = std::shared_ptr<FilterConfig>; | ||
|
|
@@ -141,6 +149,8 @@ class BandwidthLimiter : public Http::StreamFilter, Logger::Loggable<Logger::Id: | |
| std::unique_ptr<Envoy::Extensions::HttpFilters::Common::StreamRateLimiter> response_limiter_; | ||
| Stats::TimespanPtr request_latency_; | ||
| Stats::TimespanPtr response_latency_; | ||
| uint64_t request_duration_ = 0; | ||
|
Contributor
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. Would you want to use
Contributor
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. the source of request_duration_ is uint64, and this variable will be write to response trailer as string, use milliseconds will need additional convert from unit64 to milliseconds, then to string.
Contributor
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. The source is |
||
| Http::ResponseTrailerMap* trailers_; | ||
| }; | ||
|
|
||
| } // namespace BandwidthLimitFilter | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,6 +37,7 @@ TEST(Factory, RouteSpecificFilterConfig) { | |
| enable_mode: REQUEST_AND_RESPONSE | ||
| limit_kbps: 10 | ||
| fill_interval: 0.1s | ||
| response_trailer_prefix: test | ||
| )"; | ||
|
|
||
| BandwidthLimitFilterConfig factory; | ||
|
|
@@ -53,6 +54,10 @@ TEST(Factory, RouteSpecificFilterConfig) { | |
| EXPECT_EQ(config->fillInterval().count(), 100); | ||
| EXPECT_EQ(config->enableMode(), EnableMode::BandwidthLimit_EnableMode_REQUEST_AND_RESPONSE); | ||
| EXPECT_FALSE(config->tokenBucket() == nullptr); | ||
| EXPECT_EQ(const_cast<FilterConfig*>(config)->requestDelayTrailer(), | ||
| Http::LowerCaseString("test-bandwidth-request-delay-ms")); | ||
| EXPECT_EQ(const_cast<FilterConfig*>(config)->responseDelayTrailer(), | ||
| Http::LowerCaseString("test-bandwidth-response-delay-ms")); | ||
| } | ||
|
|
||
| TEST(Factory, RouteSpecificFilterConfigDisabledByDefault) { | ||
|
|
@@ -96,6 +101,11 @@ TEST(Factory, RouteSpecificFilterConfigDefaultFillInterval) { | |
| const auto* config = dynamic_cast<const FilterConfig*>(route_config.get()); | ||
| EXPECT_EQ(config->limit(), 10); | ||
| EXPECT_EQ(config->fillInterval().count(), 50); | ||
| // default trailers | ||
|
Contributor
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 you also add tests for the
Contributor
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. added, thanks. |
||
| EXPECT_EQ(const_cast<FilterConfig*>(config)->requestDelayTrailer(), | ||
| Http::LowerCaseString("bandwidth-request-delay-ms")); | ||
| EXPECT_EQ(const_cast<FilterConfig*>(config)->responseDelayTrailer(), | ||
| Http::LowerCaseString("bandwidth-response-delay-ms")); | ||
| } | ||
|
|
||
| TEST(Factory, PerRouteConfigNoLimits) { | ||
|
|
||
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.
What does it mean to change from gauge to counter 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.
Counter seems more reasonable for incoming size and allowed size, which allow us to calculate the incoming and allowed size per second for better monitoring of the bandwidth usage.
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.
Why not a histogram?
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.
My understanding is counter use less memory than histogram, and for this case, counter is good enough.
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.
The original change looks buggy. I suspect the original intent was to keep track how much data the bandwidth limit filter has currently in its buffers. But I think the call to decrement the gauge when data is removed from the buffer is missing. This may have happened when the stream_rate_limiter.cc was refactored.
I suggest filing an Issue for the gauge value not being tracked correctly and keeping it as gauge. I do not think there is particular value in the counter, the number is almost meaningless.
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.
Make sense, changed back to Gauge, and open the issue to track.
#18703