Pending requests: measure time spend waiting on a full queue #97

oschaaf · 2019-07-08T10:24:34Z

When allowing client-side queuing, this makes us report how much time we spend
being waiting on a full queue as blocking.
This allows us to get a sense of how much time we spend in closed loop mode
during a benchmark.

Addendum which fixes #88

- Switch to Envoy@ec47fee6604468bc392937967415c736f19fb22129929881270a1635ad216d87 - Fixes to build with this version - Remove C++ integration tests that broke unrepairably - Add Python-based integration testing - Sets us up with some basic orchestration to run servers (like the one from Nighthawk in these tests) - Add python format checking (CI) / fixing - Update grpc stub generation to a more sane approach Fixes envoyproxy#50 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

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

Now that we have progressed I think we can drop a couple of entries. Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

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

- Adds a the following fields to the benchmark client proto (and matching CLI args): ``` envoy.api.v2.auth.UpstreamTlsContext tls_context = 13; uint32 max_pending_requests = 14 [(validate.rules).uint32 = {gte: 1}]; uint32 max_active_requests = 15 [(validate.rules).uint32 = {gte: 1}]; uint32 max_requests_per_connection = 16 [(validate.rules).uint32 = {gte: 1}]; ``` This allows for - cipher suite selection in `https` runs, - tuning of connection re-use, - tuning client side request queueing - tuning the maximum amount of active requests When specifying `max_pending_requests > 1` the benchmark client will stop guarding the exact pacing of requests, allowing open-loop test scenarios. - Unifies validation in the CLI and service paths. - Consolidates default values into a header and improves handling. - Updates the options proto to use 32 bits where relevant, as well as switches fields to WKT's to enable us to make the distinction between `not-set` and type-default. Resolves both envoyproxy#45 Resolves envoyproxy#32 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

…tion-prototype

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

- Add sample server stats counter expectations - Add tests for cipher suite selection Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

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

Plus some other minor changes addressing review feedback Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

- Pass in the strict validation visitor instead of the NOP implementation - Uncomment a test to prove validation works - Audit NullValidationVisitor everywhere & fix Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

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

oschaaf · 2019-07-08T10:25:19Z

This needs #88 to go in first.

…cking

htuch · 2019-07-09T23:11:57Z

source/client/benchmark_client_impl.cc

@@ -189,16 +189,18 @@ void BenchmarkClientHttpImpl::setRequestHeader(absl::string_view key, absl::stri
 }

 bool BenchmarkClientHttpImpl::tryStartOne(std::function<void()> caller_completion_callback) {
+  if (!cluster_->resourceManager(Envoy::Upstream::ResourcePriority::Default)


Not sure I see how this diff relates to the PR description..

Without this change, we would see the overflow stats rise excessively. With it we will end up reporting the time we spend not being able to queue u a new request as time spend blocking (we indicate we could not by returning false). Hope that clarifies this?

I think so, but can you add comments and a test?
/wait

Done, and done. The test is implemented with python, wdyt?

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

htuch

Thanks, this is clearer.
/wait

htuch · 2019-07-11T19:24:22Z

integration/test_integration_basics.py

+    return counters
+
+  def test_h1_mini_stress_test_with_client_side_queueing(self):
+    counters = self.mini_stress_test_h1([


For all the actual test_*, can you add Pydoc style comments describing what each one does? This is pretty standard style in Google Python style.

htuch · 2019-07-11T19:27:15Z

integration/test_integration_basics.py

+    # we should observe both lots of successfull requests as well as time spend in blocking mode.,
+    parsed_json = self.runNighthawkClient(args)
+    counters = self.getNighthawkCounterMapFromJson(parsed_json)
+    self.assertGreater(counters["benchmark.http_2xx"], 1000)


This (and below) are coupled with the call site, maybe try and have one source of truth for the number of reqs.

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

htuch

Thanks!

oschaaf added 25 commits June 27, 2019 10:04

CI image update, Py3, Integration test sync

792c61e

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

Update CI image version & formatting

b2d3cbb

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

Bump bazel-compilation-database to 0.3.5

be4b41f

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

Sanitize requirements.txt

8d36fab

Now that we have progressed I think we can drop a couple of entries. Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Merge remote-tracking branch 'upstream/master' into py3-and-sync-inte…

c21ac98

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

Merge remote-tracking branch 'upstream/master' into cluster-configura…

0462810

…tion-prototype

Back out Sequencer::cancel()

4566492

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

Clean up literak string in options_impl.cc

2c08ff3

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

Test update

6b592b5

- Add sample server stats counter expectations - Add tests for cipher suite selection Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Fix format

b58796e

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

Back out accidentally committed .bazelrc change

eb47562

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

NOLINT clang-tidy warning originating from TCLAP

21ccb95

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

do_ci.sh: clean dif, revert whitespace changes

261cfd5

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

Update README.md

d98b80d

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

benchmark_client_impl.cc: clean up diff

bbd1155

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

Another cleanup round

ad8b8e1

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

Make sure we catch exception when parsing json

ca64cfe

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

Fix uint64_t's that pight to be be uint32_t

e84cc0b

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

Add & use TCLAP_SET_IF_SPECIFIED macro

698ede0

Plus some other minor changes addressing review feedback Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Process review feedback

7bfa2fc

- Pass in the strict validation visitor instead of the NOP implementation - Uncomment a test to prove validation works - Audit NullValidationVisitor everywhere & fix Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Tidy up macro definition

a1cf135

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

Add test for uint32_t TCLAP parsing range

94c5ffb

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

Open- & closed-loop: measure blocking

8cdbb73

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

Merge remote-tracking branch 'upstream/master' into open-loop-and-blo…

7db2a8c

…cking

htuch reviewed Jul 9, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 11, 2019

Update comments

08ba684

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

Add integration test

3eff871

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

repokitteh-read-only bot removed the waiting label Jul 11, 2019

Python formatting

204f333

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

htuch suggested changes Jul 11, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 11, 2019

Add PyDoc comments, add TODO + more of expecations

f4dc3de

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

repokitteh-read-only bot removed the waiting label Jul 11, 2019

oschaaf added 2 commits July 11, 2019 23:55

Define a constant for reuse x-expecations

f7e3cda

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

Amend formatting issues

a25af07

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

htuch approved these changes Jul 15, 2019

View reviewed changes

htuch merged commit 7defc6c into envoyproxy:master Jul 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pending requests: measure time spend waiting on a full queue #97

Pending requests: measure time spend waiting on a full queue #97

oschaaf commented Jul 8, 2019

oschaaf commented Jul 8, 2019

htuch Jul 9, 2019

oschaaf Jul 10, 2019

htuch Jul 11, 2019

oschaaf Jul 11, 2019

htuch left a comment

htuch Jul 11, 2019

oschaaf Jul 11, 2019

htuch Jul 11, 2019

oschaaf Jul 11, 2019

htuch left a comment

Pending requests: measure time spend waiting on a full queue #97

Pending requests: measure time spend waiting on a full queue #97

Conversation

oschaaf commented Jul 8, 2019

oschaaf commented Jul 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment