diff --git a/api/adaptive_load/adaptive_load.proto b/api/adaptive_load/adaptive_load.proto index 8b0d2724e..43d2456e7 100644 --- a/api/adaptive_load/adaptive_load.proto +++ b/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. diff --git a/api/adaptive_load/benchmark_result.proto b/api/adaptive_load/benchmark_result.proto index 13e2c0ab5..4989a33b9 100644 --- a/api/adaptive_load/benchmark_result.proto +++ b/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; diff --git a/include/nighthawk/adaptive_load/BUILD b/include/nighthawk/adaptive_load/BUILD index 3607226d1..07e6522c7 100644 --- a/include/nighthawk/adaptive_load/BUILD +++ b/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", ], ) diff --git a/include/nighthawk/adaptive_load/adaptive_load_controller.h b/include/nighthawk/adaptive_load/adaptive_load_controller.h index 6ede4e315..512e8cd5f 100644 --- a/include/nighthawk/adaptive_load/adaptive_load_controller.h +++ b/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 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 PerformAdaptiveLoadSession( nighthawk::client::NighthawkService::StubInterface* nighthawk_service_stub, const nighthawk::adaptive_load::AdaptiveLoadSessionSpec& spec, Envoy::TimeSource& time_source); diff --git a/source/adaptive_load/step_controller_impl.cc b/source/adaptive_load/step_controller_impl.cc index 7ade8082a..94ab8bcac 100644 --- a/source/adaptive_load/step_controller_impl.cc +++ b/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); diff --git a/test/adaptive_load/fake_plugins/fake_step_controller/fake_step_controller.cc b/test/adaptive_load/fake_plugins/fake_step_controller/fake_step_controller.cc index 061727805..580258dee 100644 --- a/test/adaptive_load/fake_plugins/fake_step_controller/fake_step_controller.cc +++ b/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 : diff --git a/test/adaptive_load/fake_plugins/fake_step_controller/fake_step_controller_test.cc b/test/adaptive_load/fake_plugins/fake_step_controller/fake_step_controller_test.cc index 830189da4..bdf29987f 100644 --- a/test/adaptive_load/fake_plugins/fake_step_controller/fake_step_controller_test.cc +++ b/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 plugin_or = LoadStepControllerPlugin( MakeFakeStepControllerPluginConfig(0), nighthawk::client::CommandLineOptions{}); diff --git a/test/adaptive_load/step_controller_test.cc b/test/adaptive_load/step_controller_test.cc index d502a21f6..678846b75 100644 --- a/test/adaptive_load/step_controller_test.cc +++ b/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( @@ -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;