[Bandwidth-Limit] Add response trailers for bandwidth delay by zhiyong-gayang · Pull Request #18267 · envoyproxy/envoy

zhiyong-gayang · 2021-09-27T01:31:00Z

Commit Message:

Add response trailers for the bandwidth limit filter delays.
Add new metric request_enforced and response_enforced.
Change the following metrics type from Gauge to Counter (request_incoming_size, response_incoming_size, request_allowed_size, response_allowed_size) to better calculate the network bytes per second.

Additional Description: This is useful when downstream want to understand how much delays that is caused by bandwidth limit filter.

Risk Level: Low
Testing: UT added.
Docs Changes: Updated.
Release Notes: Added.
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: gayang <gayang@microsoft.com>

repokitteh-read-only · 2021-09-27T01:31:03Z

Hi @garyyang2002, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #18267 was opened by garyyang2002.

see: more, trace.

repokitteh-read-only · 2021-09-27T01:31:08Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @htuch
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #18267 was opened by garyyang2002.

see: more, trace.

Signed-off-by: gayang <gayang@microsoft.com>

yanavlasov

/wait

yanavlasov · 2021-09-28T18:57:32Z

source/extensions/filters/http/bandwidth_limit/bandwidth_limit.cc

+      stats_(generateStats(config.stat_prefix(), scope)),
+      request_delay_trailer_(config.response_trailer_prefix().empty()
+                                 ? DefaultRequestDelayTrailer
+                                 : Http::LowerCaseString(config.response_trailer_prefix() + "-" +


absl::StrCat might be a better option here.

strcat seems can't concatenate two const strings, maybe another way I don't know.

See suggestion below

yanavlasov · 2021-09-28T19:02:15Z

source/extensions/filters/http/bandwidth_limit/bandwidth_limit.cc

  if (response_latency_) {
+    const auto& config = getConfig();
+
+    auto response_duration = response_latency_.get()->elapsed().count();


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?

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.

yanavlasov · 2021-09-28T19:25:14Z

api/envoy/extensions/filters/http/bandwidth_limit/v3alpha/bandwidth_limit.proto

+  // 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;


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

make sense, added property to enable it.

yanavlasov · 2021-09-28T19:27:03Z

source/extensions/filters/http/bandwidth_limit/bandwidth_limit.h

  std::unique_ptr<Envoy::Extensions::HttpFilters::Common::StreamRateLimiter> response_limiter_;
  Stats::TimespanPtr request_latency_;
  Stats::TimespanPtr response_latency_;
+  uint64_t request_duration_ = 0;


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

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.

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.

Signed-off-by: gayang <gayang@microsoft.com>

htuch

/wait

api/envoy/extensions/filters/http/bandwidth_limit/v3alpha/bandwidth_limit.proto

Signed-off-by: gayang <gayang@microsoft.com>

htuch

Much clearer comments, a bunch of RST nits.
/wait

docs/root/version_history/current.rst

api/envoy/extensions/filters/http/bandwidth_limit/v3alpha/bandwidth_limit.proto

Signed-off-by: gayang <gayang@microsoft.com>

htuch

LGTM API modulo nits.

htuch · 2021-10-11T15:53:52Z

docs/root/configuration/http/http_filters/bandwidth_limit_filter.rst

-  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


What does it mean to change from gauge to counter here?

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.

Why not a histogram?

My understanding is counter use less memory than histogram, and for this case, counter is good enough.

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.

Make sense, changed back to Gauge, and open the issue to track.
#18703

api/envoy/extensions/filters/http/bandwidth_limit/v3/bandwidth_limit.proto

docs/root/version_history/current.rst

htuch · 2021-10-11T15:55:29Z

@yanavlasov can you take this review through implementation review? Thanks.

Signed-off-by: gayang <gayang@microsoft.com>

htuch

/lgtm api

Signed-off-by: gayang <gayang@microsoft.com>

yanavlasov

/wait

yanavlasov · 2021-10-19T18:48:25Z

source/extensions/filters/http/bandwidth_limit/bandwidth_limit.cc

+                                  : Http::LowerCaseString(config.response_trailer_prefix() + "-" +
+                                                          DefaultResponseDelayTrailer.get())),


Suggested change

: Http::LowerCaseString(config.response_trailer_prefix() + "-" +

DefaultResponseDelayTrailer.get())),

: Http::LowerCaseString(absl::StrCat(config.response_trailer_prefix(), "-",

DefaultResponseDelayTrailer.get()))),

fixed, thanks.

yanavlasov · 2021-10-19T18:48:38Z

source/extensions/filters/http/bandwidth_limit/bandwidth_limit.cc

+      stats_(generateStats(config.stat_prefix(), scope)),
+      request_delay_trailer_(config.response_trailer_prefix().empty()
+                                 ? DefaultRequestDelayTrailer
+                                 : Http::LowerCaseString(config.response_trailer_prefix() + "-" +


See suggestion below

yanavlasov · 2021-10-20T14:18:17Z

source/extensions/filters/http/bandwidth_limit/bandwidth_limit.h

  std::unique_ptr<Envoy::Extensions::HttpFilters::Common::StreamRateLimiter> response_limiter_;
  Stats::TimespanPtr request_latency_;
  Stats::TimespanPtr response_latency_;
+  uint64_t request_duration_ = 0;


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.

yanavlasov · 2021-10-20T14:20:51Z

source/extensions/filters/http/bandwidth_limit/bandwidth_limit.cc

+    // 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) {


This should also check enableResponseTrailers()

fixed, thanks.

yanavlasov · 2021-10-20T14:34:08Z

test/extensions/filters/http/bandwidth_limit/config_test.cc

  const auto* config = dynamic_cast<const FilterConfig*>(route_config.get());
  EXPECT_EQ(config->limit(), 10);
  EXPECT_EQ(config->fillInterval().count(), 50);
+  // default trailers


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

added, thanks.

Signed-off-by: gayang <gayang@microsoft.com>

…incoming_size, response_allowed_size back to gauge Signed-off-by: gayang <gayang@microsoft.com>

Signed-off-by: gayang <gayang@microsoft.com>

htuch

LGTM, thanks!

add response trailers for bandwidth limit

d657bc6

Signed-off-by: gayang <gayang@microsoft.com>

zhiyong-gayang requested review from alyssawilk, mattklein123 and yanavlasov as code owners September 27, 2021 01:31

repokitteh-read-only bot added the api label Sep 27, 2021

repokitteh-read-only bot assigned htuch Sep 27, 2021

zhiyong-gayang added 3 commits September 27, 2021 02:20

fix format

f27b1c7

Signed-off-by: gayang <gayang@microsoft.com>

fix format

0e063bf

Signed-off-by: gayang <gayang@microsoft.com>

fix doc format

fee39ec

Signed-off-by: gayang <gayang@microsoft.com>

zhiyong-gayang mentioned this pull request Sep 28, 2021

Add response trailers for bandwidth limit filter #18040

Closed

zhiyong-gayang added 2 commits September 28, 2021 02:26

fix clang_tidy check

13bd624

Signed-off-by: gayang <gayang@microsoft.com>

update release notes

3eabf33

Signed-off-by: gayang <gayang@microsoft.com>

yanavlasov self-assigned this Sep 28, 2021

yanavlasov requested changes Sep 28, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 28, 2021

zhiyong-gayang added 2 commits September 29, 2021 11:40

add attribute enable_response_trailer

e7cc99e

Signed-off-by: gayang <gayang@microsoft.com>

fix format

a35a303

Signed-off-by: gayang <gayang@microsoft.com>

repokitteh-read-only bot removed the waiting label Sep 29, 2021

htuch suggested changes Sep 30, 2021

View reviewed changes

api/envoy/extensions/filters/http/bandwidth_limit/v3alpha/bandwidth_limit.proto Outdated Show resolved Hide resolved

api/envoy/extensions/filters/http/bandwidth_limit/v3alpha/bandwidth_limit.proto Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Sep 30, 2021

update api comments

c1c73b3

Signed-off-by: gayang <gayang@microsoft.com>

repokitteh-read-only bot removed the waiting label Sep 30, 2021

fix api comment typo

77926bb

Signed-off-by: gayang <gayang@microsoft.com>

htuch suggested changes Oct 1, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Oct 1, 2021

zhiyong-gayang added 2 commits October 11, 2021 03:40

update docs

938c352

Signed-off-by: gayang <gayang@microsoft.com>

sync upstream

a492d9d

Signed-off-by: gayang <gayang@microsoft.com>

repokitteh-read-only bot removed the waiting label Oct 11, 2021

fix current.rst merge issue

70fd09b

Signed-off-by: gayang <gayang@microsoft.com>

htuch reviewed Oct 11, 2021

View reviewed changes

update api comments

445f82d

Signed-off-by: gayang <gayang@microsoft.com>

htuch reviewed Oct 13, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Oct 13, 2021

Merge remote-tracking branch 'upstream/main' into main

e4d2ae2

Signed-off-by: gayang <gayang@microsoft.com>

yanavlasov requested changes Oct 20, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Oct 20, 2021

fix comments

ff204cc

Signed-off-by: gayang <gayang@microsoft.com>

repokitteh-read-only bot removed the waiting label Oct 21, 2021

change metrics request_incoming_size, request_allowed_size, response_…

36443a1

…incoming_size, response_allowed_size back to gauge Signed-off-by: gayang <gayang@microsoft.com>

zhiyong-gayang mentioned this pull request Oct 21, 2021

[Bandwidth-Limit] Gauge metrics not decrement correctly #18703

Open

zhiyong-gayang added 3 commits October 22, 2021 02:08

triger build

e29061d

Signed-off-by: gayang <gayang@microsoft.com>

fix format

fbd36a0

Signed-off-by: gayang <gayang@microsoft.com>

fix tests

b9cea1d

Signed-off-by: gayang <gayang@microsoft.com>

yanavlasov approved these changes Oct 26, 2021

View reviewed changes

zhiyong-gayang requested a review from htuch November 1, 2021 03:54

htuch approved these changes Nov 2, 2021

View reviewed changes

htuch merged commit 8b8ece8 into envoyproxy:main Nov 2, 2021

		: Http::LowerCaseString(config.response_trailer_prefix() + "-" +
		DefaultResponseDelayTrailer.get())),

Conversation

zhiyong-gayang commented Sep 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Sep 27, 2021

Uh oh!

repokitteh-read-only bot commented Sep 27, 2021

Uh oh!

yanavlasov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

htuch commented Oct 11, 2021

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

yanavlasov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhiyong-gayang commented Sep 27, 2021 •

edited

Loading