From 75360b2de2c360403e6c46a6b76c5fc434a94ba7 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Fri, 24 Jan 2020 13:48:08 +0100 Subject: [PATCH 1/5] Simple warmup: allow opting out The current single request we issue for the simple warmup pollutes some of the counters we emit in our output. Looking forward, when landing generic phases, we might want to default to opting out. This new flag could then serve as an opt-in afterwards, for those who want to preserve old behavior. Signed-off-by: Otto van der Schaaf --- README.md | 8 ++++++-- api/client/options.proto | 2 ++ include/nighthawk/client/options.h | 1 + source/client/client_worker_impl.cc | 10 +++++++--- source/client/client_worker_impl.h | 3 ++- source/client/options_impl.cc | 8 ++++++++ source/client/options_impl.h | 2 ++ source/client/process_impl.cc | 2 +- test/client_worker_test.cc | 2 +- test/integration/test_integration_basics.py | 6 ++---- test/mocks.h | 1 + test/options_test.cc | 6 ++++-- 12 files changed, 37 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index 35d05f66f..4d24e7dc9 100644 --- a/README.md +++ b/README.md @@ -43,8 +43,9 @@ bazel build -c opt //:nighthawk ``` USAGE: -bazel-bin/nighthawk_client [--request-source ] [--label -] ... [--multi-target-use-https] +bazel-bin/nighthawk_client [--no-simple-warmup] [--request-source ] [--label ] ... +[--multi-target-use-https] [--multi-target-path ] [--multi-target-endpoint ] ... [--experimental-h2-use-multiple-connections] @@ -77,6 +78,9 @@ format> Where: +--no-simple-warmup +Do not perform the simple warmup call. + --request-source Remote gRPC source that will deliver to-be-replayed traffic. Each worker will separately connect to this source. For example diff --git a/api/client/options.proto b/api/client/options.proto index 13bca0e17..fc627820b 100644 --- a/api/client/options.proto +++ b/api/client/options.proto @@ -187,4 +187,6 @@ message CommandLineOptions { repeated string labels = 28; // TransportSocket configuration to use in every request envoy.config.core.v3.TransportSocket transport_socket = 27; + // Do not perform the simple warmup call. + google.protobuf.BoolValue no_simple_warmup = 32; } diff --git a/include/nighthawk/client/options.h b/include/nighthawk/client/options.h index 7e96f23af..f10291db1 100644 --- a/include/nighthawk/client/options.h +++ b/include/nighthawk/client/options.h @@ -64,6 +64,7 @@ class Options { virtual std::string multiTargetPath() const PURE; virtual bool multiTargetUseHttps() const PURE; virtual std::vector labels() const PURE; + virtual bool noSimpleWarmup() const PURE; /** * Converts an Options instance to an equivalent CommandLineOptions instance in terms of option * values. diff --git a/source/client/client_worker_impl.cc b/source/client/client_worker_impl.cc index fea57e2a9..ab1c4f588 100644 --- a/source/client/client_worker_impl.cc +++ b/source/client/client_worker_impl.cc @@ -19,7 +19,8 @@ ClientWorkerImpl::ClientWorkerImpl(Envoy::Api::Api& api, Envoy::ThreadLocal::Ins const RequestSourceFactory& request_generator_factory, Envoy::Stats::Store& store, const int worker_number, const Envoy::MonotonicTime starting_time, - Envoy::Tracing::HttpTracerPtr& http_tracer) + Envoy::Tracing::HttpTracerPtr& http_tracer, + const bool simple_warmup) : WorkerImpl(api, tls, store), termination_predicate_factory_(termination_predicate_factory), sequencer_factory_(sequencer_factory), worker_scope_(store_.createScope("cluster.")), worker_number_scope_(worker_scope_->createScope(fmt::format("{}.", worker_number))), @@ -36,7 +37,8 @@ ClientWorkerImpl::ClientWorkerImpl(Envoy::Api::Api& api, Envoy::ThreadLocal::Ins termination_predicate_factory_.create( time_source_, *worker_number_scope_, starting_time), *worker_number_scope_, starting_time), - true)) {} + true)), + simple_warmup_(simple_warmup) {} void ClientWorkerImpl::simpleWarmup() { ENVOY_LOG(debug, "> worker {}: warmup start.", worker_number_); @@ -51,7 +53,9 @@ void ClientWorkerImpl::simpleWarmup() { void ClientWorkerImpl::work() { benchmark_client_->setShouldMeasureLatencies(false); request_generator_->initOnThread(); - simpleWarmup(); + if (simple_warmup_) { + simpleWarmup(); + } benchmark_client_->setShouldMeasureLatencies(phase_->shouldMeasureLatencies()); phase_->run(); diff --git a/source/client/client_worker_impl.h b/source/client/client_worker_impl.h index 9cde9b68e..2fd6b6c8c 100644 --- a/source/client/client_worker_impl.h +++ b/source/client/client_worker_impl.h @@ -31,7 +31,7 @@ class ClientWorkerImpl : public WorkerImpl, virtual public ClientWorker { const RequestSourceFactory& request_generator_factory, Envoy::Stats::Store& store, const int worker_number, const Envoy::MonotonicTime starting_time, - Envoy::Tracing::HttpTracerPtr& http_tracer); + Envoy::Tracing::HttpTracerPtr& http_tracer, const bool simple_warmup); StatisticPtrMap statistics() const override; const std::map& threadLocalCounterValues() override { @@ -59,6 +59,7 @@ class ClientWorkerImpl : public WorkerImpl, virtual public ClientWorker { PhasePtr phase_; Envoy::LocalInfo::LocalInfoPtr local_info_; std::map threadLocalCounterValues_; + const bool simple_warmup_; }; using ClientWorkerImplPtr = std::unique_ptr; diff --git a/source/client/options_impl.cc b/source/client/options_impl.cc index c13c71542..2acc7bc45 100644 --- a/source/client/options_impl.cc +++ b/source/client/options_impl.cc @@ -264,6 +264,9 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv) { "connect to this source. For example grpc://127.0.0.1:8443/.", false, "", "uri format", cmd); + TCLAP::SwitchArg no_simple_warmup("", "no-simple-warmup", + "Do not perform the simple warmup call.", cmd); + Utility::parseCommand(cmd, argc, argv); TCLAP_SET_IF_SPECIFIED(requests_per_second, requests_per_second_); @@ -365,6 +368,8 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv) { } } TCLAP_SET_IF_SPECIFIED(labels, labels_); + TCLAP_SET_IF_SPECIFIED(no_simple_warmup, no_simple_warmup_); + // CLI-specific tests. // TODO(oschaaf): as per mergconflicts's remark, it would be nice to aggregate // these and present everything we couldn't understand to the CLI user in on go. @@ -511,6 +516,7 @@ OptionsImpl::OptionsImpl(const nighthawk::client::CommandLineOptions& options) { PROTOBUF_GET_WRAPPED_OR_DEFAULT(options, experimental_h1_connection_reuse_strategy, experimental_h1_connection_reuse_strategy_); open_loop_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT(options, open_loop, open_loop_); + tls_context_.MergeFrom(options.tls_context()); if (options.has_transport_socket()) { @@ -535,6 +541,7 @@ OptionsImpl::OptionsImpl(const nighthawk::client::CommandLineOptions& options) { PROTOBUF_GET_WRAPPED_OR_DEFAULT(options, nighthawk_service, nighthawk_service_); h2_use_multiple_connections_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT( options, experimental_h2_use_multiple_connections, h2_use_multiple_connections_); + no_simple_warmup_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT(options, no_simple_warmup, no_simple_warmup_); std::copy(options.labels().begin(), options.labels().end(), std::back_inserter(labels_)); validate(); } @@ -691,6 +698,7 @@ CommandLineOptionsPtr OptionsImpl::toCommandLineOptionsInternal() const { for (const auto& label : labels_) { *command_line_options->add_labels() = label; } + command_line_options->mutable_no_simple_warmup()->set_value(no_simple_warmup_); return command_line_options; } diff --git a/source/client/options_impl.h b/source/client/options_impl.h index 84f5b6f55..042678562 100644 --- a/source/client/options_impl.h +++ b/source/client/options_impl.h @@ -79,6 +79,7 @@ class OptionsImpl : public Options, public Envoy::Logger::Loggable& arg, @@ -128,6 +129,7 @@ class OptionsImpl : public Options, public Envoy::Logger::Loggable labels_; + bool no_simple_warmup_{false}; }; } // namespace Client diff --git a/source/client/process_impl.cc b/source/client/process_impl.cc index f547ee5ac..1a0f12d0e 100644 --- a/source/client/process_impl.cc +++ b/source/client/process_impl.cc @@ -166,7 +166,7 @@ const std::vector& ProcessImpl::createWorkers(const uint32_t co workers_.push_back(std::make_unique( *api_, tls_, cluster_manager_, benchmark_client_factory_, termination_predicate_factory_, sequencer_factory_, request_generator_factory_, store_root_, worker_number, - first_worker_start + worker_delay, http_tracer_)); + first_worker_start + worker_delay, http_tracer_, !options_.noSimpleWarmup())); worker_number++; } return workers_; diff --git a/test/client_worker_test.cc b/test/client_worker_test.cc index 01c33f651..fa8cd794f 100644 --- a/test/client_worker_test.cc +++ b/test/client_worker_test.cc @@ -118,7 +118,7 @@ TEST_F(ClientWorkerTest, BasicTest) { auto worker = std::make_unique( *api_, tls_, cluster_manager_ptr_, benchmark_client_factory_, termination_predicate_factory_, sequencer_factory_, request_generator_factory_, store_, worker_number, - time_system_.monotonicTime() + 10ms, http_tracer_); + time_system_.monotonicTime() + 10ms, http_tracer_, true); worker->start(); worker->waitForCompletion(); diff --git a/test/integration/test_integration_basics.py b/test/integration/test_integration_basics.py index 2ab0cd8d2..df7ed4c1b 100644 --- a/test/integration/test_integration_basics.py +++ b/test/integration/test_integration_basics.py @@ -635,7 +635,7 @@ def test_http_request_release_timing(http_test_server_fixture, qps_parameterizat http_test_server_fixture.getTestServerRootUri(), "--duration", str(duration_parameterization_fixture), "--rps", str(qps_parameterization_fixture), "--concurrency", - str(concurrency) + str(concurrency), "--no-simple-warmup" ]) total_requests = qps_parameterization_fixture * concurrency * duration_parameterization_fixture @@ -648,6 +648,4 @@ def test_http_request_release_timing(http_test_server_fixture, qps_parameterizat assertEqual( int(global_histograms["benchmark_http_client.queue_to_connect"]["count"]), total_requests) - # When it comes to qps/rps we also expect one warmup call per worker. We'll get rid - # of this when we land the next part of the work with respect to phases. - assertCounterEqual(counters, "benchmark.http_2xx", (total_requests) + concurrency) + assertCounterEqual(counters, "benchmark.http_2xx", (total_requests)) diff --git a/test/mocks.h b/test/mocks.h index d8b6bfd80..4db34570a 100644 --- a/test/mocks.h +++ b/test/mocks.h @@ -108,6 +108,7 @@ class MockOptions : public Client::Options { MOCK_CONST_METHOD0(multiTargetPath, std::string()); MOCK_CONST_METHOD0(multiTargetUseHttps, bool()); MOCK_CONST_METHOD0(labels, std::vector()); + MOCK_CONST_METHOD0(noSimpleWarmup, bool()); }; class MockBenchmarkClientFactory : public Client::BenchmarkClientFactory { diff --git a/test/options_test.cc b/test/options_test.cc index e5c396a71..4af14181f 100644 --- a/test/options_test.cc +++ b/test/options_test.cc @@ -74,7 +74,8 @@ TEST_F(OptionsImplTest, AlmostAll) { "--termination-predicate t1:1 --termination-predicate t2:2 --failure-predicate f1:1 " "--failure-predicate f2:2 --jitter-uniform .00001s " "--experimental-h2-use-multiple-connections " - "--experimental-h1-connection-reuse-strategy lru --label label1 --label label2 {}", + "--experimental-h1-connection-reuse-strategy lru --label label1 --label label2 {} " + "--no-simple-warmup", client_name_, "{name:\"envoy.transport_sockets.tls\"," "typed_config:{\"@type\":\"type.googleapis.com/envoy.api.v2.auth.UpstreamTlsContext\"," @@ -126,7 +127,7 @@ TEST_F(OptionsImplTest, AlmostAll) { options->h1ConnectionReuseStrategy()); const std::vector expected_labels{"label1", "label2"}; EXPECT_EQ(expected_labels, options->labels()); - + EXPECT_TRUE(options->noSimpleWarmup()); // Check that our conversion to CommandLineOptionsPtr makes sense. CommandLineOptionsPtr cmd = options->toCommandLineOptions(); EXPECT_EQ(cmd->requests_per_second().value(), options->requestsPerSecond()); @@ -178,6 +179,7 @@ TEST_F(OptionsImplTest, AlmostAll) { EXPECT_EQ(cmd->experimental_h1_connection_reuse_strategy().value(), options->h1ConnectionReuseStrategy()); EXPECT_THAT(cmd->labels(), ElementsAreArray(expected_labels)); + EXPECT_EQ(cmd->no_simple_warmup().value(), options->noSimpleWarmup()); OptionsImpl options_from_proto(*cmd); std::string s1 = Envoy::MessageUtil::getYamlStringFromMessage( From 5940d5601eaa9924782c57be5a67895a7a1dfdd5 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Fri, 24 Jan 2020 19:56:05 +0100 Subject: [PATCH 2/5] Review feedback, opt out by default Signed-off-by: Otto van der Schaaf --- README.md | 4 ++-- api/client/options.proto | 2 +- docs/root/version_history.md | 4 +++- include/nighthawk/client/options.h | 2 +- source/client/client_worker_impl.cc | 6 +++--- source/client/client_worker_impl.h | 7 +++++-- source/client/options_impl.cc | 10 +++++----- source/client/options_impl.h | 4 ++-- source/client/process_impl.cc | 4 +++- test/client_worker_test.cc | 3 ++- test/integration/test_integration_basics.py | 6 +++--- test/mocks.h | 2 +- test/options_test.cc | 6 +++--- 13 files changed, 34 insertions(+), 26 deletions(-) diff --git a/README.md b/README.md index 4d24e7dc9..e2afbfb21 100644 --- a/README.md +++ b/README.md @@ -43,7 +43,7 @@ bazel build -c opt //:nighthawk ``` USAGE: -bazel-bin/nighthawk_client [--no-simple-warmup] [--request-source ] [--label ] ... [--multi-target-use-https] [--multi-target-path ] @@ -78,7 +78,7 @@ format> Where: ---no-simple-warmup +--simple-warmup Do not perform the simple warmup call. --request-source diff --git a/api/client/options.proto b/api/client/options.proto index fc627820b..be1cf104a 100644 --- a/api/client/options.proto +++ b/api/client/options.proto @@ -188,5 +188,5 @@ message CommandLineOptions { // TransportSocket configuration to use in every request envoy.config.core.v3.TransportSocket transport_socket = 27; // Do not perform the simple warmup call. - google.protobuf.BoolValue no_simple_warmup = 32; + google.protobuf.BoolValue simple_warmup = 32; } diff --git a/docs/root/version_history.md b/docs/root/version_history.md index 5696db863..35100c4ea 100644 --- a/docs/root/version_history.md +++ b/docs/root/version_history.md @@ -8,7 +8,9 @@ Version history - In `service.proto` a change was made to allow both `Output` and `error_detail` to co-exist at the same time. - Both `nighthawk_client` and `nighthawk_service` will indicate execution failure (respectively through exit code or grpc reply) when connection errors and/or status code errors are observed by default. - +- The simple warmup we performed earlier has been removed, to eliminate counter pollution. This will be restored + when configuration of phases lands in a next release. For those who need the old behavior, `--simple-warmup` + can be configured to opt-in to the old-style behavior again. ### Changelist diff --git a/include/nighthawk/client/options.h b/include/nighthawk/client/options.h index f10291db1..19329339e 100644 --- a/include/nighthawk/client/options.h +++ b/include/nighthawk/client/options.h @@ -64,7 +64,7 @@ class Options { virtual std::string multiTargetPath() const PURE; virtual bool multiTargetUseHttps() const PURE; virtual std::vector labels() const PURE; - virtual bool noSimpleWarmup() const PURE; + virtual bool simpleWarmup() const PURE; /** * Converts an Options instance to an equivalent CommandLineOptions instance in terms of option * values. diff --git a/source/client/client_worker_impl.cc b/source/client/client_worker_impl.cc index ab1c4f588..e6000765c 100644 --- a/source/client/client_worker_impl.cc +++ b/source/client/client_worker_impl.cc @@ -20,7 +20,7 @@ ClientWorkerImpl::ClientWorkerImpl(Envoy::Api::Api& api, Envoy::ThreadLocal::Ins Envoy::Stats::Store& store, const int worker_number, const Envoy::MonotonicTime starting_time, Envoy::Tracing::HttpTracerPtr& http_tracer, - const bool simple_warmup) + const HardCodedWarmupStyle hardcoded_warmup_style) : WorkerImpl(api, tls, store), termination_predicate_factory_(termination_predicate_factory), sequencer_factory_(sequencer_factory), worker_scope_(store_.createScope("cluster.")), worker_number_scope_(worker_scope_->createScope(fmt::format("{}.", worker_number))), @@ -38,7 +38,7 @@ ClientWorkerImpl::ClientWorkerImpl(Envoy::Api::Api& api, Envoy::ThreadLocal::Ins time_source_, *worker_number_scope_, starting_time), *worker_number_scope_, starting_time), true)), - simple_warmup_(simple_warmup) {} + hardcoded_warmup_style_(hardcoded_warmup_style) {} void ClientWorkerImpl::simpleWarmup() { ENVOY_LOG(debug, "> worker {}: warmup start.", worker_number_); @@ -53,7 +53,7 @@ void ClientWorkerImpl::simpleWarmup() { void ClientWorkerImpl::work() { benchmark_client_->setShouldMeasureLatencies(false); request_generator_->initOnThread(); - if (simple_warmup_) { + if (hardcoded_warmup_style_ == HardCodedWarmupStyle::ON) { simpleWarmup(); } benchmark_client_->setShouldMeasureLatencies(phase_->shouldMeasureLatencies()); diff --git a/source/client/client_worker_impl.h b/source/client/client_worker_impl.h index 2fd6b6c8c..5009f3602 100644 --- a/source/client/client_worker_impl.h +++ b/source/client/client_worker_impl.h @@ -23,6 +23,8 @@ namespace Client { class ClientWorkerImpl : public WorkerImpl, virtual public ClientWorker { public: + enum class HardCodedWarmupStyle { OFF, ON }; + ClientWorkerImpl(Envoy::Api::Api& api, Envoy::ThreadLocal::Instance& tls, Envoy::Upstream::ClusterManagerPtr& cluster_manager, const BenchmarkClientFactory& benchmark_client_factory, @@ -31,7 +33,8 @@ class ClientWorkerImpl : public WorkerImpl, virtual public ClientWorker { const RequestSourceFactory& request_generator_factory, Envoy::Stats::Store& store, const int worker_number, const Envoy::MonotonicTime starting_time, - Envoy::Tracing::HttpTracerPtr& http_tracer, const bool simple_warmup); + Envoy::Tracing::HttpTracerPtr& http_tracer, + const HardCodedWarmupStyle hardcoded_warmup_style); StatisticPtrMap statistics() const override; const std::map& threadLocalCounterValues() override { @@ -59,7 +62,7 @@ class ClientWorkerImpl : public WorkerImpl, virtual public ClientWorker { PhasePtr phase_; Envoy::LocalInfo::LocalInfoPtr local_info_; std::map threadLocalCounterValues_; - const bool simple_warmup_; + const HardCodedWarmupStyle hardcoded_warmup_style_; }; using ClientWorkerImplPtr = std::unique_ptr; diff --git a/source/client/options_impl.cc b/source/client/options_impl.cc index 2acc7bc45..09e59c153 100644 --- a/source/client/options_impl.cc +++ b/source/client/options_impl.cc @@ -264,8 +264,8 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv) { "connect to this source. For example grpc://127.0.0.1:8443/.", false, "", "uri format", cmd); - TCLAP::SwitchArg no_simple_warmup("", "no-simple-warmup", - "Do not perform the simple warmup call.", cmd); + TCLAP::SwitchArg simple_warmup("", "simple-warmup", "Do not perform the simple warmup call.", + cmd); Utility::parseCommand(cmd, argc, argv); @@ -368,7 +368,7 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv) { } } TCLAP_SET_IF_SPECIFIED(labels, labels_); - TCLAP_SET_IF_SPECIFIED(no_simple_warmup, no_simple_warmup_); + TCLAP_SET_IF_SPECIFIED(simple_warmup, simple_warmup_); // CLI-specific tests. // TODO(oschaaf): as per mergconflicts's remark, it would be nice to aggregate @@ -541,7 +541,7 @@ OptionsImpl::OptionsImpl(const nighthawk::client::CommandLineOptions& options) { PROTOBUF_GET_WRAPPED_OR_DEFAULT(options, nighthawk_service, nighthawk_service_); h2_use_multiple_connections_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT( options, experimental_h2_use_multiple_connections, h2_use_multiple_connections_); - no_simple_warmup_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT(options, no_simple_warmup, no_simple_warmup_); + simple_warmup_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT(options, simple_warmup, simple_warmup_); std::copy(options.labels().begin(), options.labels().end(), std::back_inserter(labels_)); validate(); } @@ -698,7 +698,7 @@ CommandLineOptionsPtr OptionsImpl::toCommandLineOptionsInternal() const { for (const auto& label : labels_) { *command_line_options->add_labels() = label; } - command_line_options->mutable_no_simple_warmup()->set_value(no_simple_warmup_); + command_line_options->mutable_simple_warmup()->set_value(simple_warmup_); return command_line_options; } diff --git a/source/client/options_impl.h b/source/client/options_impl.h index 042678562..8fdfe0f1f 100644 --- a/source/client/options_impl.h +++ b/source/client/options_impl.h @@ -79,7 +79,7 @@ class OptionsImpl : public Options, public Envoy::Logger::Loggable& arg, @@ -129,7 +129,7 @@ class OptionsImpl : public Options, public Envoy::Logger::Loggable labels_; - bool no_simple_warmup_{false}; + bool simple_warmup_{false}; }; } // namespace Client diff --git a/source/client/process_impl.cc b/source/client/process_impl.cc index 1a0f12d0e..7b7d3c6d1 100644 --- a/source/client/process_impl.cc +++ b/source/client/process_impl.cc @@ -166,7 +166,9 @@ const std::vector& ProcessImpl::createWorkers(const uint32_t co workers_.push_back(std::make_unique( *api_, tls_, cluster_manager_, benchmark_client_factory_, termination_predicate_factory_, sequencer_factory_, request_generator_factory_, store_root_, worker_number, - first_worker_start + worker_delay, http_tracer_, !options_.noSimpleWarmup())); + first_worker_start + worker_delay, http_tracer_, + options_.simpleWarmup() ? ClientWorkerImpl::HardCodedWarmupStyle::ON + : ClientWorkerImpl::HardCodedWarmupStyle::OFF)); worker_number++; } return workers_; diff --git a/test/client_worker_test.cc b/test/client_worker_test.cc index fa8cd794f..543ece43d 100644 --- a/test/client_worker_test.cc +++ b/test/client_worker_test.cc @@ -118,7 +118,8 @@ TEST_F(ClientWorkerTest, BasicTest) { auto worker = std::make_unique( *api_, tls_, cluster_manager_ptr_, benchmark_client_factory_, termination_predicate_factory_, sequencer_factory_, request_generator_factory_, store_, worker_number, - time_system_.monotonicTime() + 10ms, http_tracer_, true); + time_system_.monotonicTime() + 10ms, http_tracer_, + ClientWorkerImpl::HardCodedWarmupStyle::ON); worker->start(); worker->waitForCompletion(); diff --git a/test/integration/test_integration_basics.py b/test/integration/test_integration_basics.py index df7ed4c1b..dafe3db07 100644 --- a/test/integration/test_integration_basics.py +++ b/test/integration/test_integration_basics.py @@ -85,7 +85,7 @@ def test_http_h1_mini_stress_test_with_client_side_queueing(http_test_server_fix counters = mini_stress_test(http_test_server_fixture, [ http_test_server_fixture.getTestServerRootUri(), "--rps", "999999", "--max-pending-requests", "10", "--connections", "1", "--duration", "100", "--termination-predicate", - "benchmark.http_2xx:99" + "benchmark.http_2xx:99", "--simple-warmup" ]) assertCounterEqual(counters, "upstream_rq_pending_total", 11) assertCounterEqual(counters, "upstream_cx_overflow", 10) @@ -112,7 +112,7 @@ def test_http_h2_mini_stress_test_with_client_side_queueing(http_test_server_fix counters = mini_stress_test(http_test_server_fixture, [ http_test_server_fixture.getTestServerRootUri(), "--rps", "999999", "--max-pending-requests", "10", "--h2", "--max-active-requests", "1", "--connections", "1", "--duration", "100", - "--termination-predicate", "benchmark.http_2xx:99" + "--termination-predicate", "benchmark.http_2xx:99", "--simple-warmup" ]) assertCounterEqual(counters, "upstream_rq_pending_total", 1) assertCounterEqual(counters, "upstream_rq_pending_overflow", 10) @@ -635,7 +635,7 @@ def test_http_request_release_timing(http_test_server_fixture, qps_parameterizat http_test_server_fixture.getTestServerRootUri(), "--duration", str(duration_parameterization_fixture), "--rps", str(qps_parameterization_fixture), "--concurrency", - str(concurrency), "--no-simple-warmup" + str(concurrency) ]) total_requests = qps_parameterization_fixture * concurrency * duration_parameterization_fixture diff --git a/test/mocks.h b/test/mocks.h index 4db34570a..22961ab3d 100644 --- a/test/mocks.h +++ b/test/mocks.h @@ -108,7 +108,7 @@ class MockOptions : public Client::Options { MOCK_CONST_METHOD0(multiTargetPath, std::string()); MOCK_CONST_METHOD0(multiTargetUseHttps, bool()); MOCK_CONST_METHOD0(labels, std::vector()); - MOCK_CONST_METHOD0(noSimpleWarmup, bool()); + MOCK_CONST_METHOD0(simpleWarmup, bool()); }; class MockBenchmarkClientFactory : public Client::BenchmarkClientFactory { diff --git a/test/options_test.cc b/test/options_test.cc index 4af14181f..d0c55faa7 100644 --- a/test/options_test.cc +++ b/test/options_test.cc @@ -75,7 +75,7 @@ TEST_F(OptionsImplTest, AlmostAll) { "--failure-predicate f2:2 --jitter-uniform .00001s " "--experimental-h2-use-multiple-connections " "--experimental-h1-connection-reuse-strategy lru --label label1 --label label2 {} " - "--no-simple-warmup", + "--simple-warmup", client_name_, "{name:\"envoy.transport_sockets.tls\"," "typed_config:{\"@type\":\"type.googleapis.com/envoy.api.v2.auth.UpstreamTlsContext\"," @@ -127,7 +127,7 @@ TEST_F(OptionsImplTest, AlmostAll) { options->h1ConnectionReuseStrategy()); const std::vector expected_labels{"label1", "label2"}; EXPECT_EQ(expected_labels, options->labels()); - EXPECT_TRUE(options->noSimpleWarmup()); + EXPECT_TRUE(options->simpleWarmup()); // Check that our conversion to CommandLineOptionsPtr makes sense. CommandLineOptionsPtr cmd = options->toCommandLineOptions(); EXPECT_EQ(cmd->requests_per_second().value(), options->requestsPerSecond()); @@ -179,7 +179,7 @@ TEST_F(OptionsImplTest, AlmostAll) { EXPECT_EQ(cmd->experimental_h1_connection_reuse_strategy().value(), options->h1ConnectionReuseStrategy()); EXPECT_THAT(cmd->labels(), ElementsAreArray(expected_labels)); - EXPECT_EQ(cmd->no_simple_warmup().value(), options->noSimpleWarmup()); + EXPECT_EQ(cmd->simple_warmup().value(), options->simpleWarmup()); OptionsImpl options_from_proto(*cmd); std::string s1 = Envoy::MessageUtil::getYamlStringFromMessage( From bb70db154224cb50d06d315eb7e3359b25a8be9f Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Fri, 24 Jan 2020 20:16:54 +0100 Subject: [PATCH 3/5] Review feedback: update docs/comments Signed-off-by: Otto van der Schaaf --- README.md | 4 +++- api/client/options.proto | 4 +++- source/client/options_impl.cc | 8 ++++++-- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index e2afbfb21..5c42dd608 100644 --- a/README.md +++ b/README.md @@ -79,7 +79,9 @@ format> Where: --simple-warmup -Do not perform the simple warmup call. +Perform a simple single warmup request (per worker) before starting +execution. Note that this will be reflected in the counters that +Nighthawk writes to the output. Default is false. --request-source Remote gRPC source that will deliver to-be-replayed traffic. Each diff --git a/api/client/options.proto b/api/client/options.proto index be1cf104a..36f01b176 100644 --- a/api/client/options.proto +++ b/api/client/options.proto @@ -187,6 +187,8 @@ message CommandLineOptions { repeated string labels = 28; // TransportSocket configuration to use in every request envoy.config.core.v3.TransportSocket transport_socket = 27; - // Do not perform the simple warmup call. + // Perform a simple single warmup request (per worker) before starting + // execution. Note that this will be reflected in the counters that + // Nighthawk writes to the output. Default is false. google.protobuf.BoolValue simple_warmup = 32; } diff --git a/source/client/options_impl.cc b/source/client/options_impl.cc index 09e59c153..dbd064b46 100644 --- a/source/client/options_impl.cc +++ b/source/client/options_impl.cc @@ -264,8 +264,12 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv) { "connect to this source. For example grpc://127.0.0.1:8443/.", false, "", "uri format", cmd); - TCLAP::SwitchArg simple_warmup("", "simple-warmup", "Do not perform the simple warmup call.", - cmd); + TCLAP::SwitchArg simple_warmup( + "", "simple-warmup", + "Perform a simple single warmup request (per worker) before starting execution. Note that " + "this will be reflected in the counters that Nighthawk writes to the output. Default is " + "false.", + cmd); Utility::parseCommand(cmd, argc, argv); From 90eb27b4fc90bd9026fc4f68e32652dcddf94344 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Fri, 24 Jan 2020 21:02:14 +0100 Subject: [PATCH 4/5] Stabilize asan/tsan tests via --simple-warmup Signed-off-by: Otto van der Schaaf --- test/client_test.cc | 1 + test/integration/test_connection_management.py | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/test/client_test.cc b/test/client_test.cc index 47762daf5..0840c2c3c 100644 --- a/test/client_test.cc +++ b/test/client_test.cc @@ -43,6 +43,7 @@ TEST_F(ClientTest, AutoConcurrencyRun) { argv.push_back("1"); argv.push_back("--verbosity"); argv.push_back("error"); + argv.push_back("--simple-warmup"); argv.push_back("http://localhost:63657/"); Main program(argv.size(), argv.data()); EXPECT_FALSE(program.run()); diff --git a/test/integration/test_connection_management.py b/test/integration/test_connection_management.py index 2460084c2..c696c72cd 100644 --- a/test/integration/test_connection_management.py +++ b/test/integration/test_connection_management.py @@ -109,7 +109,7 @@ def countLogLinesWithSubstring(logs, substring): _, logs = http_test_server_fixture.runNighthawkClient([ "--rps 20", "-v", "trace", "--connections", "2", "--prefetch-connections", "--experimental-h1-connection-reuse-strategy", "mru", "--termination-predicate", - "benchmark.http_2xx:10", + "benchmark.http_2xx:10", "--simple-warmup", http_test_server_fixture.getTestServerRootUri() ]) From 33452473c663735c56d0e6fd4e3b7db39aad663e Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Fri, 24 Jan 2020 21:06:12 +0100 Subject: [PATCH 5/5] two more tests that need "--simple-warmup" Signed-off-by: Otto van der Schaaf --- test/integration/test_integration_basics.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/integration/test_integration_basics.py b/test/integration/test_integration_basics.py index dafe3db07..8a0153714 100644 --- a/test/integration/test_integration_basics.py +++ b/test/integration/test_integration_basics.py @@ -140,7 +140,7 @@ def test_http_h1_mini_stress_test_open_loop(http_test_server_fixture): counters = mini_stress_test(http_test_server_fixture, [ http_test_server_fixture.getTestServerRootUri(), "--rps", "10000", "--max-pending-requests", "1", "--open-loop", "--max-active-requests", "1", "--connections", "1", "--duration", "100", - "--termination-predicate", "benchmark.http_2xx:99" + "--termination-predicate", "benchmark.http_2xx:99", "--simple-warmup" ]) # we expect pool overflows assertCounterGreater(counters, "benchmark.pool_overflow", 10) @@ -154,7 +154,7 @@ def test_http_h2_mini_stress_test_open_loop(http_test_server_fixture): counters = mini_stress_test(http_test_server_fixture, [ http_test_server_fixture.getTestServerRootUri(), "--rps", "10000", "--max-pending-requests", "1", "--h2", "--open-loop", "--max-active-requests", "1", "--duration", "100", - "--termination-predicate", "benchmark.http_2xx:99" + "--termination-predicate", "benchmark.http_2xx:99", "--simple-warmup" ]) # we expect pool overflows assertCounterGreater(counters, "benchmark.pool_overflow", 10)