diff --git a/README.md b/README.md index 35d05f66f..5c42dd608 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 [--simple-warmup] [--request-source ] [--label ] ... +[--multi-target-use-https] [--multi-target-path ] [--multi-target-endpoint ] ... [--experimental-h2-use-multiple-connections] @@ -77,6 +78,11 @@ format> Where: +--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. + --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..36f01b176 100644 --- a/api/client/options.proto +++ b/api/client/options.proto @@ -187,4 +187,8 @@ message CommandLineOptions { repeated string labels = 28; // TransportSocket configuration to use in every request envoy.config.core.v3.TransportSocket transport_socket = 27; + // 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/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 7e96f23af..19329339e 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 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 bc8483eb5..db80d0c04 100644 --- a/source/client/client_worker_impl.cc +++ b/source/client/client_worker_impl.cc @@ -20,7 +20,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 HardCodedWarmupStyle hardcoded_warmup_style) : WorkerImpl(api, tls, store), time_source_(std::make_unique(*dispatcher_)), termination_predicate_factory_(termination_predicate_factory), @@ -39,7 +40,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)), + hardcoded_warmup_style_(hardcoded_warmup_style) {} void ClientWorkerImpl::simpleWarmup() { ENVOY_LOG(debug, "> worker {}: warmup start.", worker_number_); @@ -54,7 +56,9 @@ void ClientWorkerImpl::simpleWarmup() { void ClientWorkerImpl::work() { benchmark_client_->setShouldMeasureLatencies(false); request_generator_->initOnThread(); - simpleWarmup(); + if (hardcoded_warmup_style_ == HardCodedWarmupStyle::ON) { + 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 9a4ddef11..47a14398a 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); + Envoy::Tracing::HttpTracerPtr& http_tracer, + const HardCodedWarmupStyle hardcoded_warmup_style); StatisticPtrMap statistics() const override; const std::map& threadLocalCounterValues() override { @@ -60,6 +63,7 @@ class ClientWorkerImpl : public WorkerImpl, virtual public ClientWorker { PhasePtr phase_; Envoy::LocalInfo::LocalInfoPtr local_info_; std::map threadLocalCounterValues_; + 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 c13c71542..dbd064b46 100644 --- a/source/client/options_impl.cc +++ b/source/client/options_impl.cc @@ -264,6 +264,13 @@ 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", + "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); TCLAP_SET_IF_SPECIFIED(requests_per_second, requests_per_second_); @@ -365,6 +372,8 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv) { } } TCLAP_SET_IF_SPECIFIED(labels, labels_); + TCLAP_SET_IF_SPECIFIED(simple_warmup, 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 +520,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 +545,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_); + 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(); } @@ -691,6 +702,7 @@ CommandLineOptionsPtr OptionsImpl::toCommandLineOptionsInternal() const { for (const auto& label : labels_) { *command_line_options->add_labels() = label; } + 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 84f5b6f55..8fdfe0f1f 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 simple_warmup_{false}; }; } // namespace Client diff --git a/source/client/process_impl.cc b/source/client/process_impl.cc index f547ee5ac..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_)); + first_worker_start + worker_delay, http_tracer_, + options_.simpleWarmup() ? ClientWorkerImpl::HardCodedWarmupStyle::ON + : ClientWorkerImpl::HardCodedWarmupStyle::OFF)); worker_number++; } return workers_; 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/client_worker_test.cc b/test/client_worker_test.cc index 01c33f651..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_); + time_system_.monotonicTime() + 10ms, http_tracer_, + ClientWorkerImpl::HardCodedWarmupStyle::ON); worker->start(); worker->waitForCompletion(); 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() ]) diff --git a/test/integration/test_integration_basics.py b/test/integration/test_integration_basics.py index 2ab0cd8d2..8a0153714 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) @@ -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) @@ -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..22961ab3d 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(simpleWarmup, bool()); }; class MockBenchmarkClientFactory : public Client::BenchmarkClientFactory { diff --git a/test/options_test.cc b/test/options_test.cc index e5c396a71..d0c55faa7 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 {} " + "--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->simpleWarmup()); // 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->simple_warmup().value(), options->simpleWarmup()); OptionsImpl options_from_proto(*cmd); std::string s1 = Envoy::MessageUtil::getYamlStringFromMessage(