Convert proto interface strings to enums (#98)

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

Author: oschaaf

api/client/options.proto

@@ -14,6 +14,52 @@ message RequestOptions {
   google.protobuf.UInt32Value request_body_size = 3 [(validate.rules).uint32 = {lte: 4194304}];
 }
 
+// We wrap all values so we can have a unified way of option handling with respect to
+// defaults, merging, etc. As there's no stock concept for enumerations, we manually
+// define custom wrappers for them. These used to be strings, which did provide the
+// wrapped type.
+message AddressFamily {
+  enum AddressFamilyOptions {
+    AUTO = 0;
+    V4 = 1;
+    V6 = 2;
+  }
+  AddressFamilyOptions value = 1;
+}
+
+message Verbosity {
+  enum VerbosityOptions {
+    DEFAULT = 0;
+    INFO = 1;
+    TRACE = 2;
+    DEBUG = 3;
+    WARN = 4;
+    ERROR = 5;
+    CRITICAL = 6;
+  }
+  VerbosityOptions value = 1;
+}
+
+message OutputFormat {
+  enum OutputFormatOptions {
+    DEFAULT = 0;
+    JSON = 1;
+    HUMAN = 2;
+    YAML = 3;
+  }
+  OutputFormatOptions value = 1;
+}
+
+message SequencerIdleStrategy {
+  enum SequencerIdleStrategyOptions {
+    DEFAULT = 0;
+    SPIN = 1;
+    POLL = 2;
+    SLEEP = 3;
+  }
+  SequencerIdleStrategyOptions value = 1;
+}
+
 // TODO(oschaaf): Ultimately this will be a load test specification. The fact that it
 // can arrive via CLI is just a concrete detail. Change this to reflect that.
 message CommandLineOptions {
@@ -32,18 +78,16 @@ message CommandLineOptions {
   google.protobuf.StringValue concurrency =
       6; // [(validate.rules).string = {pattern: "^([0-9]*|auto)$"}];
   // See :option:`--verbosity` for details.
-  google.protobuf.StringValue verbosity = 7
-      [(validate.rules).string = {in: ["trace", "debug", "info", "warn", "error", "critical"]}];
+  Verbosity verbosity = 7;
   // See :option:`--output-format` for details.
-  google.protobuf.StringValue output_format = 8
-      [(validate.rules).string = {in: ["human", "yaml", "json"]}];
+  OutputFormat output_format = 8;
   // See :option:`--prefetch-connections` for details.
   google.protobuf.BoolValue prefetch_connections = 9;
   // See :option:`--burst-size` for details.
   google.protobuf.UInt32Value burst_size = 10 [(validate.rules).uint32 = {lte: 1000000}];
+
   // See :option:`--address-family` for details.
-  google.protobuf.StringValue address_family = 11
-      [(validate.rules).string = {in: ["v4", "v6", "auto"]}];
+  AddressFamily address_family = 11;
   oneof oneof_request_options {
     // See :option:`--request_options` for details.
     RequestOptions request_options = 12;
@@ -57,8 +101,7 @@ message CommandLineOptions {
   // See :option:`--max-requests-per-connection` for details.
   google.protobuf.UInt32Value max_requests_per_connection = 16 [(validate.rules).uint32 = {gte: 1}];
   // See :option:`--sequencer-idle-strategy` for details.
-  google.protobuf.StringValue sequencer_idle_strategy = 17
-      [(validate.rules).string = {in: ["spin", "poll", "sleep"]}];
+  SequencerIdleStrategy sequencer_idle_strategy = 17;
   // See :option:`--uri` for details.
   google.protobuf.StringValue uri = 18; // [(validate.rules).string.uri = true];
 }

ci/run_coverage.sh

@@ -32,7 +32,7 @@ if [ "$VALIDATE_COVERAGE" == "true" ]
 then
     COVERAGE_VALUE=$(grep -Po '.*lines[.]*: \K(\d|\.)*' "${COVERAGE_SUMMARY}")
     # TODO(oschaaf): The target is 97.5%, so up this whenever possible in follow ups.
-    COVERAGE_THRESHOLD=97.0
+    COVERAGE_THRESHOLD=96.9
     COVERAGE_FAILED=$(echo "${COVERAGE_VALUE}<${COVERAGE_THRESHOLD}" | bc)
 
     echo "HTML coverage report is in ${COVERAGE_DIR}/coverage.html"

include/nighthawk/client/benchmark_client.h

@@ -70,7 +70,7 @@ class BenchmarkClient {
    * Sets the request method to use when sending request.
    * @param request_method to set.
    */
-  virtual void setRequestMethod(absl::string_view request_method) PURE;
+  virtual void setRequestMethod(envoy::api::v2::core::RequestMethod request_method) PURE;
 
   /**
    * Sets request header named 'key' to the specified value.

include/nighthawk/client/options.h

@@ -28,19 +28,20 @@ class Options {
   virtual std::string uri() const PURE;
   virtual bool h2() const PURE;
   virtual std::string concurrency() const PURE;
-  virtual std::string verbosity() const PURE;
-  virtual std::string outputFormat() const PURE;
+  virtual nighthawk::client::Verbosity::VerbosityOptions verbosity() const PURE;
+  virtual nighthawk::client::OutputFormat::OutputFormatOptions outputFormat() const PURE;
   virtual bool prefetchConnections() const PURE;
   virtual uint32_t burstSize() const PURE;
-  virtual std::string addressFamily() const PURE;
-  virtual std::string requestMethod() const PURE;
+  virtual nighthawk::client::AddressFamily::AddressFamilyOptions addressFamily() const PURE;
+  virtual envoy::api::v2::core::RequestMethod requestMethod() const PURE;
   virtual std::vector<std::string> requestHeaders() const PURE;
   virtual uint32_t requestBodySize() const PURE;
   virtual const envoy::api::v2::auth::UpstreamTlsContext& tlsContext() const PURE;
   virtual uint32_t maxPendingRequests() const PURE;
   virtual uint32_t maxActiveRequests() const PURE;
   virtual uint32_t maxRequestsPerConnection() const PURE;
-  virtual std::string sequencerIdleStrategy() const PURE;
+  virtual nighthawk::client::SequencerIdleStrategy::SequencerIdleStrategyOptions
+  sequencerIdleStrategy() const PURE;
 
   /**
    * Converts an Options instance to an equivalent CommandLineOptions instance in terms of option

source/client/benchmark_client_impl.h

@@ -81,8 +81,8 @@ class BenchmarkClientHttpImpl : public BenchmarkClient,
   bool tryStartOne(std::function<void()> caller_completion_callback) override;
   Envoy::Stats::Store& store() const override { return store_; }
 
-  void setRequestMethod(absl::string_view request_method) override {
-    request_headers_.insertMethod().value(request_method);
+  void setRequestMethod(envoy::api::v2::core::RequestMethod request_method) override {
+    request_headers_.insertMethod().value(envoy::api::v2::core::RequestMethod_Name(request_method));
   };
   void setRequestHeader(absl::string_view key, absl::string_view value) override;
   void setRequestBodySize(uint32_t request_body_size) override {

source/client/client.cc

@@ -42,8 +42,11 @@ Main::Main(Client::OptionsPtr&& options) : options_(std::move(options)) {}
 
 bool Main::run() {
   Envoy::Thread::MutexBasicLockable log_lock;
+
   auto logging_context = std::make_unique<Envoy::Logger::Context>(
-      spdlog::level::from_str(options_->verbosity()), "[%T.%f][%t][%L] %v", log_lock);
+      spdlog::level::from_str(
+          nighthawk::client::Verbosity::VerbosityOptions_Name(options_->verbosity())),
+      "[%T.%f][%t][%L] %v", log_lock);
   Envoy::Event::RealTimeSystem time_system; // NO_CHECK_FORMAT(real_time)
   ProcessImpl process(*options_, time_system);
   OutputCollectorFactoryImpl output_format_factory(time_system, *options_);

source/client/factories_impl.cc

@@ -60,22 +60,10 @@ SequencerPtr SequencerFactoryImpl::create(Envoy::TimeSource& time_source,
   SequencerTarget sequencer_target = [&benchmark_client](std::function<void()> f) -> bool {
     return benchmark_client.tryStartOne(std::move(f));
   };
-  IdleStrategy idle_strategy;
-  std::string lowered_idle_strategy = options_.sequencerIdleStrategy();
-  absl::AsciiStrToLower(&lowered_idle_strategy);
-  if (lowered_idle_strategy == "spin") {
-    idle_strategy = IdleStrategy::Spin;
-  } else if (lowered_idle_strategy == "sleep") {
-    idle_strategy = IdleStrategy::Sleep;
-  } else if (lowered_idle_strategy == "poll") {
-    idle_strategy = IdleStrategy::Poll;
-  } else {
-    NOT_REACHED_GCOVR_EXCL_LINE;
-  }
-  return std::make_unique<SequencerImpl>(platform_util_, dispatcher, time_source, start_time,
-                                         std::move(rate_limiter), sequencer_target,
-                                         statistic_factory.create(), statistic_factory.create(),
-                                         options_.duration(), options_.timeout(), idle_strategy);
+  return std::make_unique<SequencerImpl>(
+      platform_util_, dispatcher, time_source, start_time, std::move(rate_limiter),
+      sequencer_target, statistic_factory.create(), statistic_factory.create(), options_.duration(),
+      options_.timeout(), options_.sequencerIdleStrategy());
 }
 
 StoreFactoryImpl::StoreFactoryImpl(const Options& options) : OptionBasedFactoryImpl(options) {}
@@ -94,15 +82,16 @@ OutputCollectorFactoryImpl::OutputCollectorFactoryImpl(Envoy::TimeSource& time_s
     : OptionBasedFactoryImpl(options), time_source_(time_source) {}
 
 OutputCollectorPtr OutputCollectorFactoryImpl::create() const {
-  const std::string format = options_.outputFormat();
-  if (format == "human") {
+  switch (options_.outputFormat()) {
+  case nighthawk::client::OutputFormat::HUMAN:
     return std::make_unique<Client::ConsoleOutputCollectorImpl>(time_source_, options_);
-  } else if (format == "json") {
+  case nighthawk::client::OutputFormat::JSON:
     return std::make_unique<Client::JsonOutputCollectorImpl>(time_source_, options_);
-  } else if (format == "yaml") {
+  case nighthawk::client::OutputFormat::YAML:
     return std::make_unique<Client::YamlOutputCollectorImpl>(time_source_, options_);
+  default:
+    NOT_REACHED_GCOVR_EXCL_LINE;
   }
-  NOT_REACHED_GCOVR_EXCL_LINE;
 }
 
 } // namespace Client

source/client/options_impl.cc

@@ -18,8 +18,8 @@ namespace Client {
 OptionsImpl::OptionsImpl(int argc, const char* const* argv) {
   setNonTrivialDefaults();
   // Override some defaults, we are in CLI-mode.
-  verbosity_ = "info";
-  output_format_ = "human";
+  verbosity_ = nighthawk::client::Verbosity::INFO;
+  output_format_ = nighthawk::client::OutputFormat::HUMAN;
 
   // TODO(oschaaf): Purge the validation we perform here. Most of it should have become
   // redundant now that we also perform validation of the resulting proto.
@@ -99,7 +99,7 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv) {
       "", "address-family",
       fmt::format("Network addres family preference. Possible values: [auto, v4, v6]. The "
                   "default output format is '{}'.",
-                  address_family_),
+                  nighthawk::client::AddressFamily::AddressFamilyOptions_Name(address_family_)),
       false, "", &address_families_allowed, cmd);
 
   std::vector<std::string> request_methods = {"GET",    "HEAD",    "POST",    "PUT",
@@ -166,18 +166,47 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv) {
   uri_ = uri.getValue();
   TCLAP_SET_IF_SPECIFIED(h2, h2_);
   TCLAP_SET_IF_SPECIFIED(concurrency, concurrency_);
-  TCLAP_SET_IF_SPECIFIED(verbosity, verbosity_);
-  TCLAP_SET_IF_SPECIFIED(output_format, output_format_);
+  // TODO(oschaaf): is there a generic way to set these enum values?
+  if (verbosity.isSet()) {
+    std::string upper_cased = verbosity.getValue();
+    absl::AsciiStrToUpper(&upper_cased);
+    RELEASE_ASSERT(nighthawk::client::Verbosity::VerbosityOptions_Parse(upper_cased, &verbosity_),
+                   "Failed to parse verbosity");
+  }
+  if (output_format.isSet()) {
+    std::string upper_cased = output_format.getValue();
+    absl::AsciiStrToUpper(&upper_cased);
+    RELEASE_ASSERT(
+        nighthawk::client::OutputFormat::OutputFormatOptions_Parse(upper_cased, &output_format_),
+        "Failed to parse output format");
+  }
   TCLAP_SET_IF_SPECIFIED(prefetch_connections, prefetch_connections_);
   TCLAP_SET_IF_SPECIFIED(burst_size, burst_size_);
-  TCLAP_SET_IF_SPECIFIED(address_family, address_family_);
-  TCLAP_SET_IF_SPECIFIED(request_method, request_method_);
+  if (address_family.isSet()) {
+    std::string upper_cased = address_family.getValue();
+    absl::AsciiStrToUpper(&upper_cased);
+    RELEASE_ASSERT(
+        nighthawk::client::AddressFamily::AddressFamilyOptions_Parse(upper_cased, &address_family_),
+        "Failed to parse address family");
+  }
+  if (request_method.isSet()) {
+    std::string upper_cased = request_method.getValue();
+    absl::AsciiStrToUpper(&upper_cased);
+    RELEASE_ASSERT(envoy::api::v2::core::RequestMethod_Parse(upper_cased, &request_method_),
+                   "Failed to parse request method");
+  }
   TCLAP_SET_IF_SPECIFIED(request_headers, request_headers_);
   TCLAP_SET_IF_SPECIFIED(request_body_size, request_body_size_);
   TCLAP_SET_IF_SPECIFIED(max_pending_requests, max_pending_requests_);
   TCLAP_SET_IF_SPECIFIED(max_active_requests, max_active_requests_);
   TCLAP_SET_IF_SPECIFIED(max_requests_per_connection, max_requests_per_connection_);
-  TCLAP_SET_IF_SPECIFIED(sequencer_idle_strategy, sequencer_idle_strategy_);
+  if (sequencer_idle_strategy.isSet()) {
+    std::string upper_cased = sequencer_idle_strategy.getValue();
+    absl::AsciiStrToUpper(&upper_cased);
+    RELEASE_ASSERT(nighthawk::client::SequencerIdleStrategy::SequencerIdleStrategyOptions_Parse(
+                       upper_cased, &sequencer_idle_strategy_),
+                   "Failed to parse sequencer idle strategy");
+  }
 
   // CLI-specific tests.
   // TODO(oschaaf): as per mergconflicts's remark, it would be nice to aggregate
@@ -247,10 +276,11 @@ OptionsImpl::OptionsImpl(const nighthawk::client::CommandLineOptions& options) {
       PROTOBUF_GET_WRAPPED_OR_DEFAULT(options, prefetch_connections, prefetch_connections_);
   burst_size_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT(options, burst_size, burst_size_);
   address_family_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT(options, address_family, address_family_);
+
   const auto& request_options = options.request_options();
   if (request_options.request_method() !=
       ::envoy::api::v2::core::RequestMethod::METHOD_UNSPECIFIED) {
-    request_method_ = ::envoy::api::v2::core::RequestMethod_Name(request_options.request_method());
+    request_method_ = request_options.request_method();
   }
   request_body_size_ =
       PROTOBUF_GET_WRAPPED_OR_DEFAULT(request_options, request_body_size, request_body_size_);
@@ -268,14 +298,7 @@ OptionsImpl::OptionsImpl(const nighthawk::client::CommandLineOptions& options) {
   validate();
 }
 
-void OptionsImpl::setNonTrivialDefaults() {
-  concurrency_ = "1";
-  verbosity_ = "warn";
-  output_format_ = "json";
-  address_family_ = "v4";
-  request_method_ = "GET";
-  sequencer_idle_strategy_ = "spin";
-}
+void OptionsImpl::setNonTrivialDefaults() { concurrency_ = "1"; }
 
 void OptionsImpl::validate() const {
   // concurrency must be either 'auto' or a positive integer.
@@ -319,12 +342,10 @@ CommandLineOptionsPtr OptionsImpl::toCommandLineOptions() const {
   command_line_options->mutable_output_format()->set_value(outputFormat());
   command_line_options->mutable_prefetch_connections()->set_value(prefetchConnections());
   command_line_options->mutable_burst_size()->set_value(burstSize());
-  command_line_options->mutable_address_family()->set_value(addressFamily());
+  command_line_options->mutable_address_family()->set_value(
+      static_cast<nighthawk::client::AddressFamily_AddressFamilyOptions>(addressFamily()));
   auto request_options = command_line_options->mutable_request_options();
-  envoy::api::v2::core::RequestMethod method =
-      envoy::api::v2::core::RequestMethod::METHOD_UNSPECIFIED;
-  envoy::api::v2::core::RequestMethod_Parse(requestMethod(), &method);
-  request_options->set_request_method(method);
+  request_options->set_request_method(requestMethod());
   for (const auto& header : requestHeaders()) {
     auto header_value_option = request_options->add_request_headers();
     // TODO(oschaaf): expose append option in CLI? For now we just set.

source/client/options_impl.h

@@ -28,12 +28,16 @@ class OptionsImpl : public Options {
   std::string uri() const override { return uri_; }
   bool h2() const override { return h2_; }
   std::string concurrency() const override { return concurrency_; }
-  std::string verbosity() const override { return verbosity_; };
-  std::string outputFormat() const override { return output_format_; };
+  nighthawk::client::Verbosity::VerbosityOptions verbosity() const override { return verbosity_; };
+  nighthawk::client::OutputFormat::OutputFormatOptions outputFormat() const override {
+    return output_format_;
+  };
   bool prefetchConnections() const override { return prefetch_connections_; }
   uint32_t burstSize() const override { return burst_size_; }
-  std::string addressFamily() const override { return address_family_; };
-  std::string requestMethod() const override { return request_method_; };
+  nighthawk::client::AddressFamily::AddressFamilyOptions addressFamily() const override {
+    return address_family_;
+  };
+  envoy::api::v2::core::RequestMethod requestMethod() const override { return request_method_; };
   std::vector<std::string> requestHeaders() const override { return request_headers_; };
   uint32_t requestBodySize() const override { return request_body_size_; };
   const envoy::api::v2::auth::UpstreamTlsContext& tlsContext() const override {
@@ -42,7 +46,10 @@ class OptionsImpl : public Options {
   uint32_t maxPendingRequests() const override { return max_pending_requests_; }
   uint32_t maxActiveRequests() const override { return max_active_requests_; }
   uint32_t maxRequestsPerConnection() const override { return max_requests_per_connection_; }
-  std::string sequencerIdleStrategy() const override { return sequencer_idle_strategy_; }
+  nighthawk::client::SequencerIdleStrategy::SequencerIdleStrategyOptions
+  sequencerIdleStrategy() const override {
+    return sequencer_idle_strategy_;
+  }
 
 private:
   void setNonTrivialDefaults();
@@ -55,19 +62,22 @@ class OptionsImpl : public Options {
   std::string uri_;
   bool h2_{false};
   std::string concurrency_;
-  std::string verbosity_;
-  std::string output_format_;
+  nighthawk::client::Verbosity::VerbosityOptions verbosity_{nighthawk::client::Verbosity::WARN};
+  nighthawk::client::OutputFormat::OutputFormatOptions output_format_{
+      nighthawk::client::OutputFormat::JSON};
   bool prefetch_connections_{false};
   uint32_t burst_size_{0};
-  std::string address_family_;
-  std::string request_method_;
+  nighthawk::client::AddressFamily::AddressFamilyOptions address_family_{
+      nighthawk::client::AddressFamily::AUTO};
+  envoy::api::v2::core::RequestMethod request_method_{envoy::api::v2::core::RequestMethod::GET};
   std::vector<std::string> request_headers_;
   uint32_t request_body_size_{0};
   envoy::api::v2::auth::UpstreamTlsContext tls_context_;
   uint32_t max_pending_requests_{1};
   uint32_t max_active_requests_{largest_acceptable_uint32_option_value};
   uint32_t max_requests_per_connection_{largest_acceptable_uint32_option_value};
-  std::string sequencer_idle_strategy_;
+  nighthawk::client::SequencerIdleStrategy::SequencerIdleStrategyOptions sequencer_idle_strategy_{
+      nighthawk::client::SequencerIdleStrategy::SPIN};
 };
 
 } // namespace Client