Review feedback: option rename

oschaaf · oschaaf · commit 594bbbecf3a3 · 2020-09-09T18:07:52.000+02:00
Signed-off-by: Otto van der Schaaf &lt;oschaaf@we-amp.com&gt;
diff --git a/README.md b/README.md
@@ -43,7 +43,7 @@ bazel build -c opt //:nighthawk
 ```
 USAGE:
 
-bazel-bin/nighthawk_client  [--response-latency-header-name <string>]
+bazel-bin/nighthawk_client  [--latency-response-header-name <string>]
 [--stats-flush-interval <uint32_t>]
 [--stats-sinks <string>] ...
 [--no-duration] [--simple-warmup]
@@ -81,7 +81,7 @@ format>
 
 Where:
 
---response-latency-header-name <string>
+--latency-response-header-name <string>
 Set an optional header name that will be returned in responses, whose
 values will be tracked in a latency histogram if set. Can be used in
 tandem with the test server's response option
diff --git a/api/client/options.proto b/api/client/options.proto
@@ -206,5 +206,5 @@ message CommandLineOptions {
   // a latency histogram if set. Can be used in tandem with the test server's response option
   // "emit_previous_request_delta_in_response_header" to record elapsed time between request
   // arrivals.
-  google.protobuf.StringValue response_latency_header_name = 36;
+  google.protobuf.StringValue latency_response_header_name = 36;
 }
diff --git a/source/client/benchmark_client_impl.cc b/source/client/benchmark_client_impl.cc
@@ -84,14 +84,14 @@ BenchmarkClientHttpImpl::BenchmarkClientHttpImpl(
     Envoy::Upstream::ClusterManagerPtr& cluster_manager,
     Envoy::Tracing::HttpTracerSharedPtr& http_tracer, absl::string_view cluster_name,
     RequestGenerator request_generator, const bool provide_resource_backpressure,
-    absl::string_view response_latency_header_name)
+    absl::string_view latency_response_header_name)
     : api_(api), dispatcher_(dispatcher), scope_(scope.createScope("benchmark.")),
       statistic_(std::move(statistic)), use_h2_(use_h2),
       benchmark_client_counters_({ALL_BENCHMARK_CLIENT_COUNTERS(POOL_COUNTER(*scope_))}),
       cluster_manager_(cluster_manager), http_tracer_(http_tracer),
       cluster_name_(std::string(cluster_name)), request_generator_(std::move(request_generator)),
       provide_resource_backpressure_(provide_resource_backpressure),
-      response_latency_header_name_(response_latency_header_name) {
+      latency_response_header_name_(latency_response_header_name) {
   statistic_.connect_statistic->setId("benchmark_http_client.queue_to_connect");
   statistic_.response_statistic->setId("benchmark_http_client.request_to_response");
   statistic_.response_header_size_statistic->setId("benchmark_http_client.response_header_size");
@@ -168,7 +168,7 @@ bool BenchmarkClientHttpImpl::tryStartRequest(CompletionCallback caller_completi
       *statistic_.connect_statistic, *statistic_.response_statistic,
       *statistic_.response_header_size_statistic, *statistic_.response_body_size_statistic,
       *statistic_.origin_latency_statistic, request->header(), shouldMeasureLatencies(),
-      content_length, generator_, http_tracer_, response_latency_header_name_);
+      content_length, generator_, http_tracer_, latency_response_header_name_);
   requests_initiated_++;
   pool_ptr->newStream(*stream_decoder, *stream_decoder);
   return true;
diff --git a/source/client/benchmark_client_impl.h b/source/client/benchmark_client_impl.h
@@ -107,7 +107,7 @@ class BenchmarkClientHttpImpl : public BenchmarkClient,
                           Envoy::Tracing::HttpTracerSharedPtr& http_tracer,
                           absl::string_view cluster_name, RequestGenerator request_generator,
                           const bool provide_resource_backpressure,
-                          absl::string_view response_latency_header_name);
+                          absl::string_view latency_response_header_name);
   void setConnectionLimit(uint32_t connection_limit) { connection_limit_ = connection_limit; }
   void setMaxPendingRequests(uint32_t max_pending_requests) {
     max_pending_requests_ = max_pending_requests;
@@ -163,7 +163,7 @@ class BenchmarkClientHttpImpl : public BenchmarkClient,
   std::string cluster_name_;
   const RequestGenerator request_generator_;
   const bool provide_resource_backpressure_;
-  const std::string response_latency_header_name_;
+  const std::string latency_response_header_name_;
 };
 
 } // namespace Client
diff --git a/source/client/options_impl.cc b/source/client/options_impl.cc
@@ -294,8 +294,8 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv) {
                   stats_flush_interval_),
       false, 5, "uint32_t", cmd);
 
-  TCLAP::ValueArg<std::string> response_latency_header_name(
-      "", "response-latency-header-name",
+  TCLAP::ValueArg<std::string> latency_response_header_name(
+      "", "latency-response-header-name",
       "Set an optional header name that will be returned in responses, whose values will be "
       "tracked in a latency histogram if set. "
       "Can be used in tandem with the test server's response option "
@@ -435,7 +435,7 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv) {
     }
   }
   TCLAP_SET_IF_SPECIFIED(stats_flush_interval, stats_flush_interval_);
-  TCLAP_SET_IF_SPECIFIED(response_latency_header_name, response_latency_header_name_);
+  TCLAP_SET_IF_SPECIFIED(latency_response_header_name, latency_response_header_name_);
 
   // CLI-specific tests.
   // TODO(oschaaf): as per mergconflicts's remark, it would be nice to aggregate
@@ -621,8 +621,8 @@ OptionsImpl::OptionsImpl(const nighthawk::client::CommandLineOptions& options) {
     no_duration_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT(options, no_duration, no_duration_);
   }
   std::copy(options.labels().begin(), options.labels().end(), std::back_inserter(labels_));
-  response_latency_header_name_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT(
-      options, response_latency_header_name, response_latency_header_name_);
+  latency_response_header_name_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT(
+      options, latency_response_header_name, latency_response_header_name_);
 
   validate();
 }
@@ -795,8 +795,8 @@ CommandLineOptionsPtr OptionsImpl::toCommandLineOptionsInternal() const {
     *command_line_options->add_stats_sinks() = stats_sink;
   }
   command_line_options->mutable_stats_flush_interval()->set_value(stats_flush_interval_);
-  command_line_options->mutable_response_latency_header_name()->set_value(
-      response_latency_header_name_);
+  command_line_options->mutable_latency_response_header_name()->set_value(
+      latency_response_header_name_);
   return command_line_options;
 }
 
diff --git a/source/client/options_impl.h b/source/client/options_impl.h
@@ -86,7 +86,7 @@ class OptionsImpl : public Options, public Envoy::Logger::Loggable<Envoy::Logger
   }
   uint32_t statsFlushInterval() const override { return stats_flush_interval_; }
   std::string responseHeaderWithLatencyInput() const override {
-    return response_latency_header_name_;
+    return latency_response_header_name_;
   };
 
 private:
@@ -141,7 +141,7 @@ class OptionsImpl : public Options, public Envoy::Logger::Loggable<Envoy::Logger
   bool no_duration_{false};
   std::vector<envoy::config::metrics::v3::StatsSink> stats_sinks_;
   uint32_t stats_flush_interval_{5};
-  std::string response_latency_header_name_;
+  std::string latency_response_header_name_;
 };
 
 } // namespace Client
diff --git a/source/client/stream_decoder.cc b/source/client/stream_decoder.cc
@@ -19,8 +19,8 @@ void StreamDecoder::decodeHeaders(Envoy::Http::ResponseHeaderMapPtr&& headers, b
   response_header_sizes_statistic_.addValue(response_headers_->byteSize());
   const uint64_t response_code = Envoy::Http::Utility::getResponseStatus(*response_headers_);
   stream_info_.response_code_ = static_cast<uint32_t>(response_code);
-  if (!response_latency_header_name_.empty()) {
-    const auto timing_header_name = Envoy::Http::LowerCaseString(response_latency_header_name_);
+  if (!latency_response_header_name_.empty()) {
+    const auto timing_header_name = Envoy::Http::LowerCaseString(latency_response_header_name_);
     const Envoy::Http::HeaderEntry* timing_header = response_headers_->get(timing_header_name);
     if (timing_header != nullptr) {
       absl::string_view timing_value = timing_header->value().getStringView();
diff --git a/source/client/stream_decoder.h b/source/client/stream_decoder.h
@@ -47,7 +47,7 @@ class StreamDecoder : public Envoy::Http::ResponseDecoder,
                 HeaderMapPtr request_headers, bool measure_latencies, uint32_t request_body_size,
                 Envoy::Random::RandomGenerator& random_generator,
                 Envoy::Tracing::HttpTracerSharedPtr& http_tracer,
-                absl::string_view response_latency_header_name)
+                absl::string_view latency_response_header_name)
       : dispatcher_(dispatcher), time_source_(time_source),
         decoder_completion_callback_(decoder_completion_callback),
         caller_completion_callback_(std::move(caller_completion_callback)),
@@ -59,7 +59,7 @@ class StreamDecoder : public Envoy::Http::ResponseDecoder,
         complete_(false), measure_latencies_(measure_latencies),
         request_body_size_(request_body_size), stream_info_(time_source_),
         random_generator_(random_generator), http_tracer_(http_tracer),
-        response_latency_header_name_(response_latency_header_name) {
+        latency_response_header_name_(latency_response_header_name) {
     if (measure_latencies_ && http_tracer_ != nullptr) {
       setupForTracing();
     }
@@ -121,7 +121,7 @@ class StreamDecoder : public Envoy::Http::ResponseDecoder,
   Envoy::Tracing::HttpTracerSharedPtr& http_tracer_;
   Envoy::Tracing::SpanPtr active_span_;
   Envoy::StreamInfo::UpstreamTiming upstream_timing_;
-  const std::string response_latency_header_name_;
+  const std::string latency_response_header_name_;
 };
 
 } // namespace Client
diff --git a/test/integration/test_integration_basics.py b/test/integration/test_integration_basics.py
@@ -713,7 +713,7 @@ def test_http_h1_response_header_latency_tracking(http_test_server_fixture, serv
   parsed_json, _ = http_test_server_fixture.runNighthawkClient([
       http_test_server_fixture.getTestServerRootUri(), "--connections", "1", "--rps", "100",
       "--duration", "100", "--termination-predicate", "benchmark.http_2xx:99",
-      "--response-latency-header-name", "x-origin-request-receipt-delta"
+      "--latency-response-header-name", "x-origin-request-receipt-delta"
   ])
   global_histograms = http_test_server_fixture.getNighthawkGlobalHistogramsbyIdFromJson(parsed_json)
   asserts.assertEqual(int(global_histograms["benchmark_http_client.latency_2xx"]["count"]), 100)
diff --git a/test/options_test.cc b/test/options_test.cc
@@ -118,7 +118,7 @@ TEST_F(OptionsImplTest, AlmostAll) {
       "--experimental-h2-use-multiple-connections "
       "--experimental-h1-connection-reuse-strategy lru --label label1 --label label2 {} "
       "--simple-warmup --stats-sinks {} --stats-sinks {} --stats-flush-interval 10 "
-      "--response-latency-header-name zz",
+      "--latency-response-header-name zz",
       client_name_,
       "{name:\"envoy.transport_sockets.tls\","
       "typed_config:{\"@type\":\"type.googleapis.com/envoy.api.v2.auth.UpstreamTlsContext\","
@@ -248,7 +248,7 @@ TEST_F(OptionsImplTest, AlmostAll) {
   ASSERT_EQ(cmd->stats_sinks_size(), options->statsSinks().size());
   EXPECT_TRUE(util(cmd->stats_sinks(0), options->statsSinks()[0]));
   EXPECT_TRUE(util(cmd->stats_sinks(1), options->statsSinks()[1]));
-  EXPECT_EQ(cmd->response_latency_header_name().value(), options->responseHeaderWithLatencyInput());
+  EXPECT_EQ(cmd->latency_response_header_name().value(), options->responseHeaderWithLatencyInput());
 
   OptionsImpl options_from_proto(*cmd);
   std::string s1 = Envoy::MessageUtil::getYamlStringFromMessage(

Original file line number	Diff line number	Diff line change
`@@ -206,5 +206,5 @@ message CommandLineOptions {`
`206`	`206`	`// a latency histogram if set. Can be used in tandem with the test server's response option`
`207`	`207`	`// "emit_previous_request_delta_in_response_header" to record elapsed time between request`
`208`	`208`	`// arrivals.`
`209`		`- google.protobuf.StringValue response_latency_header_name = 36;`
	`209`	`+ google.protobuf.StringValue latency_response_header_name = 36;`
`210`	`210`	`}`