Test server dynamic delay by oschaaf · Pull Request #390 · envoyproxy/nighthawk

oschaaf · 2020-06-29T12:03:45Z

Add an extension which implements dynamic delay injection based on the number
of concurrent requests being handled, as well as static delays.

Fixes #389 and also #82. We may consider removing the direct fault filter configs
from our docs as well as emit a log warning when detecting usage to deprecate.
It would be nice if we could add the fault filter extension in code instead of imposing
configuration of that upon the end user, making that an implementation detail.

The following shows a per-request config example.
This header-provided configuration gets merged with any provided static configuration to
determine a final configuration for each request:

curl -H "x-nighthawk-test-server-config:{static_delay:\"1s\"}"  -vv 127.0.0.1:10000

Full yaml config for the http_filters section:

                http_filters:
                  - name: dynamic-delay
                    config:
                      concurrency_based_delay:
                        minimal_delay: 0.05s
                        concurrency_delay_factor: 0.010s                  
                  - name: test-server
                    config:
                      response_body_size: 10
                      response_headers:
                        - { header: { key: "x-nh", value: "1" } }

Signed-off-by: Otto van der Schaaf oschaaf@we-amp.com

State: first stab, manually verified to work. needs

discussion
unit tests
a bit of cleanup and
end to end test, to prove the number of active filter instances is tracked correctly and interaction with the fault filter works as intended (filed dynamic-delay filter: end to end test #393 to track).
docs.

Add an extension which implements dynamic delay injection based on the delta between target and current guage values. State: manually verified to work. needs tests, a bit of cleanup and docs. Fixes envoyproxy#389 and also envoyproxy#82, we may want to remove the direct fault filter configs from our docs as well as emit a log warning when detecting usage to deprecate. Per-request config example, this gets merged with any static configuratio to determine the active configuration: ``` curl -H "x-nighthawk-test-server-config:{static_delay:\"1s\"}" -vv 127.0.0.1:10000 ``` Full yaml config for the http_filters section: ```yaml http_filters: - name: dynamic-delay config: stats_based_delay: guage_name: "http.ingress_http.downstream_rq_active" guage_target_value: 10 delay_delta: 0.1s - name: envoy.fault config: max_active_faults: 100 delay: header_delay: {} percentage: numerator: 100 - name: test-server config: response_body_size: 10 response_headers: - { header: { key: "x-nh", value: "1" } } ``` Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

…ic-delay

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf · 2020-06-29T13:11:15Z

@mum4k @jmarantz opening this up as a draft to allow for early review. (See the issue description for state), but prevent merging (or the impression of this being in that shape yet).

I would like to invite you to assess the high level approach as well as the algorithm/parameterization to achieve the goal we're after here: being able to induce a sustained/fixed level of parallelism on the downstream side (e.g. a proxy running in front of the test server).

While working on this PR I was mostly focussed on wiring in the overall mechanics of the desired type of functionality,
but not so much the algorithm itself.

jmarantz

haven't been through all of it yet, but using arbitrary gauges brings a bit of complexity.

api/server/response_options.proto

source/server/http_dynamic_delay_filter.cc

api/server/response_options.proto

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

…ic-delay

oschaaf · 2020-06-29T22:51:40Z

@jmarantz @jc1e100 I pushed changes to partially address comments in 277c786, and updated the PR description.
I couldn't directly stick an atomic into the shared filter config, as that implicitly deletes the copy constructor, and prevents the build from succeeding with that. So it is static at this time. Let me know if this looks better.

jc1e100

Thanks, the new simplified config format LGTM.

api/server/response_options.proto

source/server/http_dynamic_delay_filter.cc

jmarantz · 2020-06-30T20:15:18Z

source/server/http_dynamic_delay_filter.cc

+std::atomic<uint64_t> HttpDynamicDelayDecoderFilterConfig::instances_(0);
+
+HttpDynamicDelayDecoderFilterConfig::HttpDynamicDelayDecoderFilterConfig(
+    nighthawk::server::ResponseOptions proto_config)


nighthawk::server::ResponseOptions&& ?

source/server/http_dynamic_delay_filter.cc

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf · 2020-07-01T15:16:35Z

This is ready for another pass; I punted a bug and a missing functional test to #392 and #393.

oschaaf · 2020-07-01T18:16:51Z

Linking the CI coverage report for convenience, this seems ok to me (combined with the issue that tracks that we need covering actual end-to-end functionality).

mum4k · 2020-07-01T19:38:45Z

@dubious90 please review and assign back to me once done.

Remove the python-based error flow test in favor of a c++ one. Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

source/server/http_dynamic_delay_filter.h

jmarantz

looks good but someone more expert in the NH code should also review.

test/server/http_dynamic_delay_filter_integration_test.cc

dubious90 · 2020-07-06T15:15:55Z

source/server/README.md

+- `response_headers` - list of headers to add to response. If `append` is set to
  `true`, then the header is appended.
-* `echo_request_headers` - if set to `true`, then append the dump of request headers to the response
+- `echo_request_headers` - if set to `true`, then append the dump of request headers to the response


Should add a bullet here for oneof_delay_options, which is your new field in ResponseOptions.

I think you missed this, unless this is dependent on the result of the larger thread about the use of the fault filter.

The later revisions have the new options documented in a new section right below this one, does that work for you?

source/server/README.md

source/server/common.h

dubious90 · 2020-07-06T16:26:21Z

source/server/http_dynamic_delay_filter.cc

+  if (base_config_.has_static_delay()) {
+    delay_ms = Envoy::Protobuf::util::TimeUtil::DurationToMilliseconds(base_config_.static_delay());
+  } else if (base_config_.has_concurrency_based_delay()) {
+    const nighthawk::server::ConcurrencyBasedDelay& concurrency_config =
+        base_config_.concurrency_based_delay();
+    const uint64_t current_value = config_->approximateInstances();
+    delay_ms = computeDelayMilliseconds(current_value, concurrency_config.minimal_delay(),
+                                        concurrency_config.concurrency_delay_factor());
+  }
+  if (delay_ms.has_value() && delay_ms > 0) {
+    // Emit header to communicate the delay we desire to the fault filter extension.
+    const Envoy::Http::LowerCaseString key("x-envoy-fault-delay-request");
+    headers.setCopy(key, absl::StrCat(*delay_ms));
+  }


I think it'd help the readability here if you abstracted this section into its own function, as this is the part of the function that is actually setting the delays, rather than just decoding headers.

Done, let me know if that looks like what you had in mind (see 8ee78db)

I was actually thinking grab lines 49 - 62 here, so including the part where it determines what the delay is. But I defer to you on what makes the most sense.

Done, let me know if that looks better.

source/server/http_dynamic_delay_filter.h

source/server/http_dynamic_delay_filter_config.cc

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

source/server/README.md

…ic-delay

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

source/server/common.h

dubious90 · 2020-07-08T16:53:10Z

source/server/README.md

+- `response_headers` - list of headers to add to response. If `append` is set to
  `true`, then the header is appended.
-* `echo_request_headers` - if set to `true`, then append the dump of request headers to the response
+- `echo_request_headers` - if set to `true`, then append the dump of request headers to the response


I think you missed this, unless this is dependent on the result of the larger thread about the use of the fault filter.

dubious90 · 2020-07-08T16:55:03Z

LGTM, after the two quick comments are resolved. @mum4k to approve.

…ver-dynamic-delay Partially moves to using the fault filter extension as a base class Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf

After some discussion via Slack with @htuch and @dubious90 I changed this to take the fault filter as a base class, so there are some new changes in here.

source/server/common.h

oschaaf · 2020-07-09T14:59:27Z

source/server/README.md

+- `response_headers` - list of headers to add to response. If `append` is set to
  `true`, then the header is appended.
-* `echo_request_headers` - if set to `true`, then append the dump of request headers to the response
+- `echo_request_headers` - if set to `true`, then append the dump of request headers to the response


The later revisions have the new options documented in a new section right below this one, does that work for you?

mum4k

Note - I started reviewing the implementation only after I commented my preference on the inherit / copy / ... the fault injection filter, so I only now noticed that we already invested into the inheritance approach. I would like us to minimize the churn on this PR, since it is already quite large, so let's leave the current approach in.

If you do feel this will add enough maintenance overhead for us to feel it, I think we should consider filing an issue to follow-up on this decision and address it later if we reach a different conclusion.

Since this is a fairly large PR, stopping the review mid stream and providing partial set of comments to avoid blocking you.

mum4k · 2020-07-09T20:22:37Z

source/server/common.h

+
+} // namespace TestServer
+
+class Utility {


I am wondering if we can improve the naming and structure of this new library. Both "common" and "Utility" are very generic names that a) don't tell the reader much and b) attract pretty much any helper / utility function. After all everything is an utility.

Secondly it doesn't seem necessary for this to be a class at all, since the two methods on it don't really need any state. How would you feel about changing these into regular functions and renaming the library into something more specific.

E.g. response_options.h

Feel free to choose any other name that is specific enough not to attract unrelated functions.

Done in 9be5334, let me know if that looks better.

mum4k · 2020-07-09T20:33:20Z

source/server/common.h

+
+namespace TestServer {
+
+class HeaderNameValues {


As per the comment below (ideally, please read that one first) - are we moving this definition here just because we don't have a better place for it and this file happens to have a generic enough name? And a similar question - does this have to be a class?

Alternative approach - this currently only seems to be used in a single location - how about we just inline the literal there, is it worth having a this defined as a constant?

If yes, can we define a new header specifically designed for this? And if we are doing that - is attaching these onto a class an idiom used in Envoy, or can we can try to figure out an approach that doesn't require a class? Even an ordinary function returning static constexpr might be preferred.

inline absl::string_view MyString() { static constexpr char kHello[] = "Hello"; return kHello; }

Like for above, see 9be5334. However, I kept the Envoy construct, because this is shared/reused across the two extensions, and having the LowerCaseString is convenient -- that is the form that consumers are probably going to end up needing when they want access to this.

mum4k · 2020-07-09T20:36:54Z

source/server/common.h

@@ -0,0 +1,50 @@
+#pragma once


Can we add unit tests for this library?

There are pre-existing unit tests for this. For example, see

nighthawk/test/server/http_test_server_filter_integration_test.cc

Line 303 in 06a6a02

TEST_F(HttpTestServerDecoderFilterTest, HeaderMerge) {

mum4k · 2020-07-09T20:37:49Z

source/server/common.h

@@ -0,0 +1,50 @@
+#pragma once


(for the future) Looks like the changes in this library may have been just indirectly related to the work in this PR. If reasonable, can we try to split such changes into their own PR in the future to minimize the size of PRs and the review complexity?

Yeah, you're right, sorry about that. Will extract these mechanical refactors out into a separate PR next time.

mum4k · 2020-07-09T20:39:08Z

source/server/http_dynamic_delay_filter.h

+namespace Server {
+
+/**
+ * Filter configuration container class for the dynamic delay extension.


Can we expand the class doc comment with a few more information, ideally at least an example of use to help the reader put this in context?

The public API of this class has a few functions that aren't obvious (or their use case isn't), so this can help with that too.

mum4k · 2020-07-09T20:39:20Z

source/server/http_dynamic_delay_filter.h

+class HttpDynamicDelayDecoderFilterConfig {
+
+public:
+  HttpDynamicDelayDecoderFilterConfig(nighthawk::server::ResponseOptions proto_config,


Can we document the constructor and the arguments it takes?

mum4k · 2020-07-09T20:47:24Z

source/server/http_dynamic_delay_filter.h

+namespace Server {
+
+/**
+ * Filter configuration container class for the dynamic delay extension.


Can we indicate the thread-safety of this class in the comment?

mum4k · 2020-07-09T20:47:47Z

source/server/http_dynamic_delay_filter.h

+  /**
+   * Increments the number of active instances.
+   */
+  void incrementInstanceCount() { instances()++; }


Can you help me understand what these are for and why is the getter getting an approximate amount rather than exact?

mum4k · 2020-07-09T20:48:21Z

source/server/http_dynamic_delay_filter.h

+   */
+  uint64_t approximateInstances() const { return instances(); }
+
+  Envoy::Runtime::Loader& runtime() { return runtime_; }


Can we document all public methods?

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf · 2020-07-13T19:18:32Z

@mum4k I pushed a enhancements based on your feedback in 9be5334 and
ad5e34c. Let me know of those two commits sufficiently address it.

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

mum4k

Looks good, just waiting for the CI checks to finish.

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf added 4 commits June 29, 2020 12:29

Merge remote-tracking branch 'upstream/master' into test-server-dynam…

b44cf2c

…ic-delay

Fix tests, improve naming

159599f

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

s/guage/gauge/

3ec87c0

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf requested review from jmarantz and mum4k June 29, 2020 13:11

oschaaf added the waiting-for-review A PR waiting for a review. label Jun 29, 2020

jmarantz reviewed Jun 29, 2020

View reviewed changes

api/server/response_options.proto Outdated Show resolved Hide resolved

source/server/http_dynamic_delay_filter.cc Outdated Show resolved Hide resolved

jmarantz mentioned this pull request Jun 29, 2020

stats: need mechanism for finding a stat by name in production code envoyproxy/envoy#11799

Closed

jc1e100 reviewed Jun 29, 2020

View reviewed changes

api/server/response_options.proto Outdated Show resolved Hide resolved

oschaaf added 2 commits June 30, 2020 00:46

Review feedback

277c786

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Merge remote-tracking branch 'upstream/master' into test-server-dynam…

a34b8cd

…ic-delay

jc1e100 reviewed Jun 30, 2020

View reviewed changes

api/server/response_options.proto Show resolved Hide resolved

jc1e100 reviewed Jun 30, 2020

View reviewed changes

api/server/response_options.proto Outdated Show resolved Hide resolved

jmarantz reviewed Jun 30, 2020

View reviewed changes

Review feedback + tests + some polish

5d6fb7e

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf mentioned this pull request Jul 1, 2020

Response options configuration merge vs proto durations #392

Open

Add TODO, cover bad header config in test.

d7bfb51

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf marked this pull request as ready for review July 1, 2020 15:11

mum4k requested a review from dubious90 July 1, 2020 19:38

oschaaf added 2 commits July 2, 2020 12:59

Enhance testing, (doc) comments, and assert on assumptions

caf8c78

Remove the python-based error flow test in favor of a c++ one. Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Add override keyword to destructor

aeddcdd

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

jmarantz reviewed Jul 2, 2020

View reviewed changes

source/server/http_dynamic_delay_filter.h Outdated Show resolved Hide resolved

jmarantz previously approved these changes Jul 2, 2020

View reviewed changes

test/server/http_dynamic_delay_filter_integration_test.cc Outdated Show resolved Hide resolved

oschaaf requested a review from htuch July 2, 2020 20:05

dubious90 suggested changes Jul 6, 2020

View reviewed changes

oschaaf added 2 commits July 6, 2020 23:34

Partial review feedback

c40e9ce

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

save state

6113345

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

dubious90 reviewed Jul 7, 2020

View reviewed changes

source/server/README.md Outdated Show resolved Hide resolved

oschaaf added 3 commits July 8, 2020 11:29

Merge remote-tracking branch 'upstream/master' into test-server-dynam…

1cd6cd7

…ic-delay

review feedback

8ee78db

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Move comment out of method body to fix coverage

3f006f6

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

dubious90 suggested changes Jul 8, 2020

View reviewed changes

dubious90 assigned mum4k Jul 8, 2020

oschaaf added 3 commits July 9, 2020 11:02

Merge commit '611334586db1c81d7e2c97875b769c867e8c6a95' into test-ser…

3a875cd

…ver-dynamic-delay Partially moves to using the fault filter extension as a base class Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

save state: Review feedback + fault filter base

3e3c6ef

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Update documentation & fix destructor

ac456f3

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf commented Jul 9, 2020

View reviewed changes

mum4k requested changes Jul 9, 2020

View reviewed changes

mum4k added waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. and removed waiting-for-review A PR waiting for a review. labels Jul 9, 2020

oschaaf added 2 commits July 13, 2020 20:27

Partial review feedback

9be5334

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Review feedback

ad5e34c

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf added waiting-for-review A PR waiting for a review. and removed waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. labels Jul 13, 2020

Fix missing dependency

3c80a0b

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

mum4k previously approved these changes Jul 13, 2020

View reviewed changes

clang-tidy: fix include

8f74535

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf dismissed mum4k’s stale review via 8f74535 July 13, 2020 21:16

mum4k approved these changes Jul 14, 2020

View reviewed changes

mum4k merged commit 05c4c8d into envoyproxy:master Jul 14, 2020

oschaaf mentioned this pull request Jul 15, 2020

Take ownership of the fault-filter delay configuration #82

Closed

oschaaf mentioned this pull request Aug 10, 2020

Test-server integration with the fault filter #21

Closed

Conversation

oschaaf commented Jun 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oschaaf commented Jun 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oschaaf commented Jun 29, 2020

Uh oh!

jc1e100 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

oschaaf commented Jul 1, 2020

Uh oh!

oschaaf commented Jul 1, 2020

Uh oh!

mum4k commented Jul 1, 2020

Uh oh!

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

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!

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dubious90 commented Jul 8, 2020

Uh oh!

oschaaf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mum4k 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

oschaaf commented Jun 29, 2020 •

edited

Loading

oschaaf commented Jun 29, 2020 •

edited

Loading