Simple warmup: default to opting out

README.md

@@ -43,8 +43,9 @@ bazel build -c opt //:nighthawk
 ```
 USAGE:
 
-bazel-bin/nighthawk_client  [--request-source <uri format>] [--label
-<string>] ... [--multi-target-use-https]
+bazel-bin/nighthawk_client  [--simple-warmup] [--request-source <uri
+format>] [--label <string>] ...
+[--multi-target-use-https]
 [--multi-target-path <string>]
 [--multi-target-endpoint <string>] ...
 [--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 <uri format>
 Remote gRPC source that will deliver to-be-replayed traffic. Each
 worker will separately connect to this source. For example

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;
 }

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
 

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<std::string> labels() const PURE;
+  virtual bool simpleWarmup() const PURE;
   /**
    * Converts an Options instance to an equivalent CommandLineOptions instance in terms of option
    * values.

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<CachedTimeSourceImpl>(*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();
 

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<std::string, uint64_t>& threadLocalCounterValues() override {
@@ -60,6 +63,7 @@ class ClientWorkerImpl : public WorkerImpl, virtual public ClientWorker {
   PhasePtr phase_;
   Envoy::LocalInfo::LocalInfoPtr local_info_;
   std::map<std::string, uint64_t> threadLocalCounterValues_;
+  const HardCodedWarmupStyle hardcoded_warmup_style_;
 };
 
 using ClientWorkerImplPtr = std::unique_ptr<ClientWorkerImpl>;

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;
 }
 

source/client/options_impl.h

@@ -79,6 +79,7 @@ class OptionsImpl : public Options, public Envoy::Logger::Loggable<Envoy::Logger
   }
   std::string multiTargetPath() const override { return multi_target_path_; }
   bool multiTargetUseHttps() const override { return multi_target_use_https_; }
+  bool simpleWarmup() const override { return simple_warmup_; }
 
 private:
   void parsePredicates(const TCLAP::MultiArg<std::string>& arg,
@@ -128,6 +129,7 @@ class OptionsImpl : public Options, public Envoy::Logger::Loggable<Envoy::Logger
   std::string multi_target_path_;
   bool multi_target_use_https_{false};
   std::vector<std::string> labels_;
+  bool simple_warmup_{false};
 };
 
 } // namespace Client

source/client/process_impl.cc

@@ -166,7 +166,9 @@ const std::vector<ClientWorkerPtr>& ProcessImpl::createWorkers(const uint32_t co
     workers_.push_back(std::make_unique<ClientWorkerImpl>(
         *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_;

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());

test/client_worker_test.cc

@@ -118,7 +118,8 @@ TEST_F(ClientWorkerTest, BasicTest) {
   auto worker = std::make_unique<ClientWorkerImpl>(
       *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();

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()
   ])
 

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))

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<std::string>());
+  MOCK_CONST_METHOD0(simpleWarmup, bool());
 };
 
 class MockBenchmarkClientFactory : public Client::BenchmarkClientFactory {

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<std::string> 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(