Skip to content

[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.

Sign up for GitHub

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

Merged
htuch merged 20 commits into from
Nov 2, 2021
Merged

[Bandwidth-Limit] Add response trailers for bandwidth delay #18267

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
d657bc6
add response trailers for bandwidth limit
zhiyong-gayang Sep 27, 2021
f27b1c7
fix format
zhiyong-gayang Sep 27, 2021
0e063bf
fix format
zhiyong-gayang Sep 27, 2021
fee39ec
fix doc format
zhiyong-gayang Sep 27, 2021
13bd624
fix clang_tidy check
zhiyong-gayang Sep 28, 2021
3eabf33
update release notes
zhiyong-gayang Sep 28, 2021
e7cc99e
add attribute enable_response_trailer
zhiyong-gayang Sep 29, 2021
a35a303
fix format
zhiyong-gayang Sep 29, 2021
c1c73b3
update api comments
zhiyong-gayang Sep 30, 2021
77926bb
fix api comment typo
zhiyong-gayang Sep 30, 2021
938c352
update docs
zhiyong-gayang Oct 11, 2021
a492d9d
sync upstream
zhiyong-gayang Oct 11, 2021
70fd09b
fix current.rst merge issue
zhiyong-gayang Oct 11, 2021
445f82d
update api comments
zhiyong-gayang Oct 12, 2021
e4d2ae2
Merge remote-tracking branch 'upstream/main' into main
zhiyong-gayang Oct 19, 2021
ff204cc
fix comments
zhiyong-gayang Oct 21, 2021
36443a1
change metrics request_incoming_size, request_allowed_size, response_…
zhiyong-gayang Oct 21, 2021
e29061d
triger build
zhiyong-gayang Oct 22, 2021
fbd36a0
fix format
zhiyong-gayang Oct 22, 2021
b9cea1d
fix tests
zhiyong-gayang Oct 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion api/envoy/extensions/filters/http/bandwidth_limit/v3alpha/bandwidth_limit.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE;
// Bandwidth limit :ref:`configuration overview <config_http_filters_bandwidth_limit>`.
// [#extension: envoy.filters.http.bandwidth_limit]

// [#next-free-field: 6]
// [#next-free-field: 7]
message BandwidthLimit {
// Defines the mode for the bandwidth limit filter.
// Values represent bitmask.
Expand Down Expand Up @@ -67,4 +67,12 @@ message BandwidthLimit {
// Runtime flag that controls whether the filter is enabled or not. If not specified, defaults
// to enabled.
config.core.v3.RuntimeFeatureFlag runtime_enabled = 5;

// Optional the prefix for the response trailers of bandwidth decode/encode delays for stream transfer.
Comment thread
htuch marked this conversation as resolved.
Outdated
// If not set, use the default value "bandwidth-request-delay-ms" and "bandwidth-response-delay-ms".
// If set, the trailer name will be set as:
// request: response_trailer_prefix + "-bandwidth-request-delay-ms"
// response: response_trailer_prefix + "-bandwidth-response-delay-ms"
// If EnableMode is DISABLED or REQUEST or delay time = 0, the trailer will not be set.
string response_trailer_prefix = 6;

@yanavlasov yanavlasov Sep 28, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to make these trailers optional, such that an operator can enable or disable it?

@zhiyong-gayang zhiyong-gayang Sep 29, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sense, added property to enable it.

}
10 changes: 6 additions & 4 deletions docs/root/configuration/http/http_filters/bandwidth_limit_filter.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,14 +42,16 @@ The HTTP bandwidth limit filter outputs statistics in the ``<stat_prefix>.http_b
:widths: 1, 1, 2

request_enabled, Counter, Total number of request streams for which the bandwidth limiter was consulted
request_enforced, Counter, Total number of request streams for which the bandwidth limiter was enforced
request_pending, GAUGE, Number of request streams which are currently pending transfer in bandwidth limiter
request_incoming_size, GAUGE, Size in bytes of incoming request data to bandwidth limiter
request_allowed_size, GAUGE, Size in bytes of outgoing request data from bandwidth limiter
request_incoming_size, Counter, Size in bytes of incoming request data to bandwidth limiter
request_allowed_size, Counter, Size in bytes of outgoing request data from bandwidth limiter
request_transfer_duration, HISTOGRAM, Total time (including added delay) it took for the request stream transfer
response_enabled, Counter, Total number of response streams for which the bandwidth limiter was consulted
response_enforced, Counter, Total number of response streams for which the bandwidth limiter was enforced
response_pending, GAUGE, Number of response streams which are currently pending transfer in bandwidth limiter
response_incoming_size, GAUGE, Size in bytes of incoming response data to bandwidth limiter
response_allowed_size, GAUGE, Size in bytes of outgoing response data from bandwidth limiter
response_incoming_size, Counter, Size in bytes of incoming response data to bandwidth limiter
response_allowed_size, Counter, Size in bytes of outgoing response data from bandwidth limiter

@htuch htuch Oct 11, 2021

Copy link
Copy Markdown
Member

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?

@zhiyong-gayang zhiyong-gayang Oct 12, 2021

Copy link
Copy Markdown
Contributor Author

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.

@htuch htuch Oct 13, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not a histogram?

@zhiyong-gayang zhiyong-gayang Oct 13, 2021

Copy link
Copy Markdown
Contributor Author

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.

@yanavlasov yanavlasov Oct 20, 2021

Copy link
Copy Markdown
Contributor

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.

@zhiyong-gayang zhiyong-gayang Oct 21, 2021

Copy link
Copy Markdown
Contributor Author

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

response_transfer_duration, HISTOGRAM, Total time (including added delay) it took for the response stream transfer

.. _config_http_filters_bandwidth_limit_runtime:
Expand Down
2 changes: 2 additions & 0 deletions docs/root/version_history/current.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ Minor Behavior Changes
----------------------
*Changes that may cause incompatibilities for some users, but should not for most*

* bandwidth_limit: added response trailers when request or response delay are enforced.
Comment thread
htuch marked this conversation as resolved.
Outdated
* bandwidth_limit: added bandwidth limit stats request_enforced and response_enforced.
* client_ssl_auth filter: now sets additional termination details and **UAEX** response flag when the client certificate is not in the allowed-list.
* config: configuration files ending in .yml now load as YAML.
* config: configuration file extensions now ignore case when deciding the file type. E.g., .JSON file load as JSON.
Expand Down
62 changes: 54 additions & 8 deletions source/extensions/filters/http/bandwidth_limit/bandwidth_limit.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,29 @@ 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() + "-" +

@yanavlasov yanavlasov Sep 28, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

absl::StrCat might be a better option here.

@zhiyong-gayang zhiyong-gayang Sep 29, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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.

@yanavlasov yanavlasov Oct 19, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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())) {
if (per_route && !config.has_limit_kbps()) {
throw EnvoyException("bandwidthlimitfilter: limit must be set for per route filter config");
}
Expand Down Expand Up @@ -64,7 +79,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());
}
Expand All @@ -82,7 +102,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;
Expand Down Expand Up @@ -123,7 +143,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());
}
Expand All @@ -135,23 +160,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) {

@yanavlasov yanavlasov Oct 20, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also check enableResponseTrailers()

@zhiyong-gayang zhiyong-gayang Oct 21, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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 {
Expand All @@ -164,6 +200,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();
Expand All @@ -172,9 +209,18 @@ void BandwidthLimiter::updateStatsOnDecodeFinish() {

void BandwidthLimiter::updateStatsOnEncodeFinish() {
if (response_latency_) {
const auto& config = getConfig();

auto response_duration = response_latency_.get()->elapsed().count();

@yanavlasov yanavlasov Sep 28, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are planning to make stats write only, except for stat sinks. This code will end up broken in this case. Can a timestamp be used in this case?

@zhiyong-gayang zhiyong-gayang Sep 29, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To get from the stats will give me a consistent delay value for response trail and the stats.
Make stats write only seems a challenge work, can we read the stats here before you have a concrete plan how to remove the stats read reference.

if (trailers_ != nullptr && request_duration_ > 0) {
trailers_->setCopy(config.requestDelayTrailer(), std::to_string(request_duration_));
}
if (trailers_ != nullptr && 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();
}
}

Expand Down
16 changes: 12 additions & 4 deletions source/extensions/filters/http/bandwidth_limit/bandwidth_limit.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -71,6 +73,8 @@ 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_; }

private:
friend class FilterTest;
Expand All @@ -86,6 +90,8 @@ 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_;
};

using FilterConfigSharedPtr = std::shared_ptr<FilterConfig>;
Expand Down Expand Up @@ -142,6 +148,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;

@yanavlasov yanavlasov Sep 28, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you want to use std::chrono::milliseconds to make units explicit?

@zhiyong-gayang zhiyong-gayang Sep 29, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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.

@yanavlasov yanavlasov Oct 20, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The source is CompletableTimespan::ellapsed which returns std::milliseconds. I think you should keep the type around until you need to write it into the trailer.

Http::ResponseTrailerMap* trailers_;
};

} // namespace BandwidthLimitFilter
Expand Down
14 changes: 11 additions & 3 deletions source/extensions/filters/http/common/stream_rate_limiter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ StreamRateLimiter::StreamRateLimiter(
uint64_t max_kbps, uint64_t max_buffered_data, std::function<void()> pause_data_cb,
std::function<void()> resume_data_cb,
std::function<void(Buffer::Instance&, bool)> write_data_cb, std::function<void()> continue_cb,
std::function<void(uint64_t)> write_stats_cb, TimeSource& time_source,
std::function<void(uint64_t, bool)> write_stats_cb, TimeSource& time_source,
Event::Dispatcher& dispatcher, const ScopeTrackedObject& scope,
std::shared_ptr<TokenBucket> token_bucket, std::chrono::milliseconds fill_interval)
: fill_interval_(std::move(fill_interval)), write_data_cb_(write_data_cb),
Expand Down Expand Up @@ -63,7 +63,7 @@ void StreamRateLimiter::onTokenTimer() {
// Move the data to write into the output buffer with as little copying as possible.
// NOTE: This might be moving zero bytes, but that should work fine.
data_to_write.move(buffer_, bytes_to_write);
write_stats_cb_(bytes_to_write);
write_stats_cb_(bytes_to_write, buffer_.length() > 0);

// If the buffer still contains data in it, we couldn't get enough tokens, so schedule the next
// token available time.
Expand All @@ -88,10 +88,18 @@ void StreamRateLimiter::onTokenTimer() {
}
}

void StreamRateLimiter::writeData(Buffer::Instance& incoming_buffer, bool end_stream) {
void StreamRateLimiter::writeData(Buffer::Instance& incoming_buffer, bool end_stream,
bool trailer_added) {
auto len = incoming_buffer.length();
buffer_.move(incoming_buffer);
saw_end_stream_ = end_stream;
// If trailer_added is true, set saw_trailers_ to true to continue encode trailers, added
// after buffer_.move to ensure buffer has data and won't invoke continue_cb_ before
// processing the data in last data frame.
if (trailer_added) {
saw_trailers_ = true;
}

ENVOY_LOG(debug,
"StreamRateLimiter <writeData>: got new {} bytes of data. token "
"timer {} scheduled.",
Expand Down
10 changes: 5 additions & 5 deletions source/extensions/filters/http/common/stream_rate_limiter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,17 +49,17 @@ class StreamRateLimiter : Logger::Loggable<Logger::Id::filter> {
StreamRateLimiter(uint64_t max_kbps, uint64_t max_buffered_data,
std::function<void()> pause_data_cb, std::function<void()> resume_data_cb,
std::function<void(Buffer::Instance&, bool)> write_data_cb,
std::function<void()> continue_cb, std::function<void(uint64_t)> write_stats_cb,
TimeSource& time_source, Event::Dispatcher& dispatcher,
const ScopeTrackedObject& scope,
std::function<void()> continue_cb,
std::function<void(uint64_t, bool)> write_stats_cb, TimeSource& time_source,
Event::Dispatcher& dispatcher, const ScopeTrackedObject& scope,
std::shared_ptr<TokenBucket> token_bucket = nullptr,
std::chrono::milliseconds fill_interval = DefaultFillInterval);

/**
* Called by the stream to write data. All data writes happen asynchronously, the stream should
* be stopped after this call (all data will be drained from incoming_buffer).
*/
void writeData(Buffer::Instance& incoming_buffer, bool end_stream);
void writeData(Buffer::Instance& incoming_buffer, bool end_stream, bool trailer_added = false);

/**
* Called if the stream receives trailers.
Expand All @@ -83,7 +83,7 @@ class StreamRateLimiter : Logger::Loggable<Logger::Id::filter> {
const std::chrono::milliseconds fill_interval_;
const std::function<void(Buffer::Instance&, bool)> write_data_cb_;
const std::function<void()> continue_cb_;
const std::function<void(uint64_t)> write_stats_cb_;
const std::function<void(uint64_t, bool)> write_stats_cb_;
const ScopeTrackedObject& scope_;
std::shared_ptr<TokenBucket> token_bucket_;
Event::TimerPtr token_timer_;
Expand Down
2 changes: 1 addition & 1 deletion source/extensions/filters/http/fault/fault_filter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ void FaultFilter::maybeSetupResponseRateLimit(const Http::RequestHeaderMap& requ
encoder_callbacks_->injectEncodedDataToFilterChain(data, end_stream);
},
[this] { encoder_callbacks_->continueEncoding(); },
[](uint64_t) {
[](uint64_t, bool) {
// write stats callback.
},
config_->timeSource(), decoder_callbacks_->dispatcher(), decoder_callbacks_->scope());
Expand Down
10 changes: 10 additions & 0 deletions test/extensions/filters/http/bandwidth_limit/config_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ TEST(Factory, RouteSpecificFilterConfig) {
enable_mode: REQUEST_AND_RESPONSE
limit_kbps: 10
fill_interval: 0.1s
response_trailer_prefix: test
)";

BandwidthLimitFilterConfig factory;
Expand All @@ -54,6 +55,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) {
Expand Down Expand Up @@ -97,6 +102,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

@yanavlasov yanavlasov Oct 20, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add tests for the enable_response_trailers config value, please?

@zhiyong-gayang zhiyong-gayang Oct 21, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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) {
Expand Down
Loading