Adaptive Load migration to absl::StatusOr

api/adaptive_load/adaptive_load.proto

@@ -33,16 +33,22 @@ message AdaptiveLoadSessionSpec {
   // A proto describing Nighthawk Service traffic. See
   // https://github.com/envoyproxy/nighthawk/blob/master/api/client/options.proto
   //
-  // The adaptive load controller will return an error if the |duration| or
-  // |open_loop| fields are set within |nighthawk_traffic_options|. The
-  // controller will also be configured to overwrite at least one of the
-  // numerical fields during the search, such as requests_per_second, so any
-  // value of those fields specified here will be ignored.
+  // The adaptive load controller will return an error if the |duration| field is set within
+  // |nighthawk_traffic_template|.
   //
-  // All other fields in |nighthawk_traffic_options| are passed through to the
+  // If |open_loop| is unset, it will be overridden to true by the adaptive load controller. This is
+  // to support the typical case where the controller needs full control over attempted requests per
+  // second, which could conflict with the backpressure mechanism of closed-loop mode. Note that in
+  // standalone Nighthawk clients, closed-loop mode is the default.
+  //
+  // The controller will override at least one field in this proto to vary the load, such as the
+  // requests_per_second field or headers. Any existing value for such a field in the template will
+  // be ignored.
+  //
+  // All other fields in |nighthawk_traffic_template| are passed through to the
   // Nighthawk Service.
   //
-  // Note that |concurrency| in |nighthawk_traffic_options| multiplies the total
+  // Note that |concurrency| in |nighthawk_traffic_template| multiplies the total
   // RPS sent.
   //
   // Required.
@@ -65,10 +71,6 @@ message AdaptiveLoadSessionSpec {
 // Complete description of an adaptive load session, including metric scores
 // for every degree of load attempted during the adjusting stage.
 message AdaptiveLoadSessionOutput {
-  // Overall status of the session with error detail. INVALID_ARGUMENT if the input spec contained
-  // errors, DEADLINE_EXCEEDED if convergence did not occur before the deadline, ABORTED if the step
-  // controller determined it can never converge.
-  google.rpc.Status session_status = 1;
   // Results of each short benchmark performed during the adjusting stage.
   repeated BenchmarkResult adjusting_stage_results = 2;
   // Result of the single benchmark of the testing stage.

api/adaptive_load/benchmark_result.proto

@@ -30,9 +30,6 @@ message BenchmarkResult {
   // Raw Nighthawk Service output. Includes start/end times and full Nighthawk
   // Service input spec.
   nighthawk.client.Output nighthawk_service_output = 1;
-  // Execution status of this call to the Nighthawk Service. This will record errors connecting to
-  // the Nighthawk Service and internal errors returned from the Nighthawk Service.
-  google.rpc.Status status = 2;
   // Status of all declared metrics during this benchmark session. Not present
   // in the event of Nighthawk Service errors.
   repeated MetricEvaluation metric_evaluations = 3;

include/nighthawk/adaptive_load/BUILD

@@ -21,6 +21,7 @@ envoy_basic_cc_library(
         "@envoy//include/envoy/common:base_includes",
         "@envoy//include/envoy/common:time_interface",
         "@envoy//include/envoy/config:typed_config_interface",
+        "@envoy//source/common/common:statusor_lib_with_external_headers",
     ],
 )
 

include/nighthawk/adaptive_load/adaptive_load_controller.h

@@ -2,6 +2,8 @@
 
 #include "envoy/common/time.h"
 
+#include "external/envoy/source/common/common/statusor.h"
+
 #include "api/adaptive_load/adaptive_load.pb.h"
 #include "api/client/service.grpc.pb.h"
 
@@ -23,11 +25,10 @@ namespace Nighthawk {
  * Envoy-based process, there may be an existing TimeSource or TimeSystem to use. If calling
  * from a test, pass a fake TimeSource.
  *
- * @return AdaptiveLoadSessionOutput a proto logging the result of all traffic attempted and all
- * corresponding metric values and scores. Any errors that occur will be recorded in the
- * |session_status| field.
+ * @return StatusOr<AdaptiveLoadSessionOutput> A proto logging the result of all traffic attempted
+ * and all corresponding metric values and scores, or an overall error status if the session failed.
  */
-nighthawk::adaptive_load::AdaptiveLoadSessionOutput PerformAdaptiveLoadSession(
+absl::StatusOr<nighthawk::adaptive_load::AdaptiveLoadSessionOutput> PerformAdaptiveLoadSession(
     nighthawk::client::NighthawkService::StubInterface* nighthawk_service_stub,
     const nighthawk::adaptive_load::AdaptiveLoadSessionSpec& spec, Envoy::TimeSource& time_source);
 

source/adaptive_load/step_controller_impl.cc

@@ -116,10 +116,6 @@ bool ExponentialSearchStepController::IsDoomed(std::string& doom_reason) const {
 }
 
 void ExponentialSearchStepController::UpdateAndRecompute(const BenchmarkResult& benchmark_result) {
-  if (benchmark_result.status().code()) {
-    doom_reason_ = "Nighthawk Service returned an error.";
-    return;
-  }
   const double score = TotalScore(benchmark_result);
   if (is_range_finding_phase_) {
     IterateRangeFindingPhase(score);

test/adaptive_load/fake_plugins/fake_step_controller/fake_step_controller.cc

@@ -40,13 +40,6 @@ FakeStepController::GetCurrentCommandLineOptions() const {
 
 void FakeStepController::UpdateAndRecompute(
     const nighthawk::adaptive_load::BenchmarkResult& benchmark_result) {
-  if (benchmark_result.status().code() == ::grpc::OK) {
-    is_doomed_ = false;
-    doomed_reason_ = "";
-  } else {
-    is_doomed_ = true;
-    doomed_reason_ = benchmark_result.status().message();
-  }
   // "Convergence" is defined as the latest benchmark reporting any score > 0.0.
   is_converged_ = false;
   for (const nighthawk::adaptive_load::MetricEvaluation& metric_evaluation :

test/adaptive_load/fake_plugins/fake_step_controller/fake_step_controller_test.cc

@@ -132,38 +132,6 @@ TEST(FakeStepController, IsConvergedReturnsTrueAfterBenchmarkResultWithPositiveS
   EXPECT_TRUE(step_controller.IsConverged());
 }
 
-TEST(FakeStepController, IsDoomedReturnsFalseAfterSuccessfulBenchmarkResult) {
-  FakeStepController step_controller(FakeStepControllerConfig{}, CommandLineOptions{});
-  BenchmarkResult benchmark_result;
-  benchmark_result.mutable_status()->set_code(::grpc::OK);
-  step_controller.UpdateAndRecompute(benchmark_result);
-  std::string doomed_reason;
-  EXPECT_FALSE(step_controller.IsDoomed(doomed_reason));
-}
-
-TEST(FakeStepController,
-     IsDoomedReturnsFalseAndLeavesDoomedReasonUntouchedAfterSuccessfulBenchmarkResult) {
-  FakeStepController step_controller(FakeStepControllerConfig{}, CommandLineOptions{});
-  BenchmarkResult benchmark_result;
-  benchmark_result.mutable_status()->set_code(::grpc::OK);
-  step_controller.UpdateAndRecompute(benchmark_result);
-  std::string variable_that_should_not_be_written = "original value";
-  EXPECT_FALSE(step_controller.IsDoomed(variable_that_should_not_be_written));
-  EXPECT_EQ(variable_that_should_not_be_written, "original value");
-}
-
-TEST(FakeStepController, IsDoomedReturnsTrueAndSetsDoomedReasonAfterFailedBenchmarkResult) {
-  const std::string kErrorMessage = "error from nighthawk";
-  FakeStepController step_controller(FakeStepControllerConfig{}, CommandLineOptions{});
-  BenchmarkResult benchmark_result;
-  benchmark_result.mutable_status()->set_code(::grpc::INTERNAL);
-  benchmark_result.mutable_status()->set_message(kErrorMessage);
-  step_controller.UpdateAndRecompute(benchmark_result);
-  std::string doomed_reason;
-  EXPECT_TRUE(step_controller.IsDoomed(doomed_reason));
-  EXPECT_EQ(doomed_reason, kErrorMessage);
-}
-
 TEST(MakeFakeStepControllerPluginConfig, ActivatesFakeStepControllerPlugin) {
   absl::StatusOr<StepControllerPtr> plugin_or = LoadStepControllerPlugin(
       MakeFakeStepControllerPluginConfig(0), nighthawk::client::CommandLineOptions{});

test/adaptive_load/step_controller_test.cc

@@ -29,12 +29,6 @@ nighthawk::adaptive_load::BenchmarkResult MakeBenchmarkResultWithScore(double sc
   return result;
 }
 
-nighthawk::adaptive_load::BenchmarkResult MakeBenchmarkResultWithNighthawkError() {
-  nighthawk::adaptive_load::BenchmarkResult result;
-  result.mutable_status()->set_code(1);
-  return result;
-}
-
 TEST(ExponentialSearchStepControllerConfigFactory, GeneratesEmptyConfigProto) {
   auto& config_factory =
       Envoy::Config::Utility::getAndCheckFactoryByName<StepControllerConfigFactory>(
@@ -175,16 +169,6 @@ TEST(ExponentialSearchStepController, ReportsDoomIfOutsideThresholdsOnInitialVal
   EXPECT_THAT(doom_reason, HasSubstr("already exceed metric thresholds with the initial load"));
 }
 
-TEST(ExponentialSearchStepController, ReportsDoomAfterNighthawkServiceError) {
-  nighthawk::adaptive_load::ExponentialSearchStepControllerConfig config;
-  nighthawk::client::CommandLineOptions options_template;
-  ExponentialSearchStepController step_controller(config, options_template);
-  step_controller.UpdateAndRecompute(MakeBenchmarkResultWithNighthawkError());
-  std::string doom_reason;
-  EXPECT_TRUE(step_controller.IsDoomed(doom_reason));
-  EXPECT_EQ(doom_reason, "Nighthawk Service returned an error.");
-}
-
 TEST(ExponentialSearchStepController,
      IncreasesRpsExponentiallyIfWithinThresholdUsingDefaultExponent) {
   const double kInitialInput = 100.0;