Improve Unknown Error 13 messages

include/nighthawk/client/process.h

@@ -2,6 +2,8 @@
 
 #include "nighthawk/client/output_collector.h"
 
+#include "absl/status/status.h"
+
 namespace Nighthawk {
 namespace Client {
 
@@ -15,9 +17,9 @@ class Process {
 
   /**
    * @param collector used to transform output into the desired format.
-   * @return bool true iff execution was successfull.
+   * @return OK if execution succeeded or was cancelled, otherwise error details.
    */
-  virtual bool run(OutputCollector& collector) PURE;
+  virtual absl::Status run(OutputCollector& collector) PURE;
 
   /**
    * Shuts down the worker. Mandatory call before destructing.

source/client/client.cc

@@ -77,7 +77,7 @@ bool Main::run() {
   }
   OutputFormatterFactoryImpl output_formatter_factory;
   OutputCollectorImpl output_collector(time_system, *options_);
-  bool result;
+  absl::Status result;
   {
     auto signal_handler =
         std::make_unique<SignalHandler>([&process]() { process->requestExecutionCancellation(); });
@@ -92,12 +92,12 @@ bool Main::run() {
     std::cout << *formatted_proto;
   }
   process->shutdown();
-  if (!result) {
-    ENVOY_LOG(error, "An error ocurred.");
-  } else {
-    ENVOY_LOG(info, "Done.");
+  if (!result.ok()) {
+    ENVOY_LOG(error, "An error occurred: {}.", result.message());
+    return false;
   }
-  return result;
+  ENVOY_LOG(info, "Done.");
+  return true;
 }
 
 } // namespace Client

source/client/options_impl.cc

@@ -667,11 +667,12 @@ OptionsImpl::OptionsImpl(const nighthawk::client::CommandLineOptions& options) {
 
 void OptionsImpl::setNonTrivialDefaults() {
   concurrency_ = "1";
-  // By default, we don't tolerate error status codes and connection failures, and will report
-  // upon observing those.
+  // By default, we don't tolerate error status codes, connection failures, or stream resets which
+  // could indicate a protocol mismatch, and will report upon observing those.
   failure_predicates_["benchmark.http_4xx"] = 0;
   failure_predicates_["benchmark.http_5xx"] = 0;
   failure_predicates_["benchmark.pool_connection_failure"] = 0;
+  failure_predicates_["benchmark.stream_resets"] = 0;
   // Also, fail fast when a remote request source is specified that we can't connect to or otherwise
   // fails.
   failure_predicates_["requestsource.upstream_rq_5xx"] = 0;

source/client/process_impl.cc

@@ -28,6 +28,7 @@
 #include "external/envoy/source/common/thread_local/thread_local_impl.h"
 #include "external/envoy/source/server/server.h"
 
+#include "absl/status/status.h"
 #include "absl/strings/str_replace.h"
 #include "absl/types/optional.h"
 
@@ -473,18 +474,19 @@ void ProcessImpl::setupStatsSinks(const envoy::config::bootstrap::v3::Bootstrap&
   }
 }
 
-bool ProcessImpl::runInternal(OutputCollector& collector, const std::vector<UriPtr>& uris,
-                              const UriPtr& request_source_uri, const UriPtr& tracing_uri,
-                              const absl::optional<Envoy::SystemTime>& scheduled_start) {
+absl::Status ProcessImpl::runInternal(OutputCollector& collector, const std::vector<UriPtr>& uris,
+                                      const UriPtr& request_source_uri, const UriPtr& tracing_uri,
+                                      const absl::optional<Envoy::SystemTime>& scheduled_start) {
   const Envoy::SystemTime now = time_system_.systemTime();
   if (scheduled_start.value_or(now) < now) {
     ENVOY_LOG(error, "Scheduled execution date already transpired.");
-    return false;
+    return absl::InternalError("Scheduled execution date already transpired.");
   }
   {
     auto guard = std::make_unique<Envoy::Thread::LockGuard>(workers_lock_);
     if (cancelled_) {
-      return true;
+      ENVOY_LOG(info, "Execution was cancelled before it started.");
+      return absl::OkStatus();
     }
     int number_of_workers = determineConcurrency();
     shutdown_ = false;
@@ -598,17 +600,32 @@ bool ProcessImpl::runInternal(OutputCollector& collector, const std::vector<UriP
   StatisticFactoryImpl statistic_factory(options_);
   collector.addResult("global", mergeWorkerStatistics(workers_), counters,
                       total_execution_duration / workers_.size(), first_acquisition_time);
-  return counters.find("sequencer.failed_terminations") == counters.end();
+  if (cancelled_) {
+    ENVOY_LOG(info, "Execution was cancelled.");
+    return absl::OkStatus();
+  }
+  if (counters.find("sequencer.failed_terminations") != counters.end()) {
+    return absl::InternalError(
+        "Terminated early because of a failure predicate. Check the output for problematic counter "
+        "values. The default Nighthawk failure predicates report failure if (1) Nighthawk could "
+        "not connect to the target (see 'benchmark.pool_connection_failure' counter; check the "
+        "address and port number, and try explicitly setting --address-family v4 or v6, especially "
+        "when using DNS; instead of localhost try 127.0.0.1 or ::1 explicitly), (2) the protocol "
+        "was not supported by the target (see 'benchmark.stream_resets' counter; check http/https "
+        "in the URI, --h2), (3) the target returned a 4xx or 5xx HTTP response code (see "
+        "'benchmark.http_#xx' counters; check the URI path and the server config), or (4) a custom "
+        "gRPC RequestSource failed. --failure-predicate can be used to relax expectations.");
+  }
+  return absl::OkStatus();
 }
 
-bool ProcessImpl::run(OutputCollector& collector) {
+absl::Status ProcessImpl::run(OutputCollector& collector) {
   std::vector<UriPtr> uris;
   UriPtr request_source_uri;
   UriPtr tracing_uri;
 
   try {
-    // TODO(oschaaf): See if we can rid of resolving here.
-    // We now only do it to validate.
+    // Resolving URIs validates the syntax and also detects DNS errors.
     if (options_.uri().has_value()) {
       uris.push_back(std::make_unique<UriImpl>(options_.uri().value()));
     } else {
@@ -632,16 +649,19 @@ bool ProcessImpl::run(OutputCollector& collector) {
       tracing_uri->resolve(*dispatcher_,
                            Utility::translateFamilyOptionString(options_.addressFamily()));
     }
-  } catch (const UriException&) {
-    return false;
+  } catch (const UriException& ex) {
+    return absl::InvalidArgumentError(
+        absl::StrCat("URI exception (for example, malformed URI syntax, bad "
+                     "MultiTarget path, unresolvable host DNS): ",
+                     ex.what()));
   }
 
   try {
     return runInternal(collector, uris, request_source_uri, tracing_uri,
                        options_.scheduled_start());
   } catch (Envoy::EnvoyException& ex) {
     ENVOY_LOG(error, "Fatal exception: {}", ex.what());
-    throw;
+    return absl::InternalError(absl::StrCat("Fatal exception: ", ex.what()));
   }
 }
 

source/client/process_impl.h

@@ -75,9 +75,9 @@ class ProcessImpl : public Process, public Envoy::Logger::Loggable<Envoy::Logger
    *
    * @param collector output collector implementation which will collect and hold the native output
    * format.
-   * @return true iff execution should be considered successful.
+   * @return OK if execution was successful, otherwise error details.
    */
-  bool run(OutputCollector& collector) override;
+  absl::Status run(OutputCollector& collector) override;
 
   /**
    * Should be called before destruction to cleanly shut down.
@@ -124,9 +124,9 @@ class ProcessImpl : public Process, public Envoy::Logger::Loggable<Envoy::Logger
    */
   void setupStatsSinks(const envoy::config::bootstrap::v3::Bootstrap& bootstrap,
                        std::list<std::unique_ptr<Envoy::Stats::Sink>>& stats_sinks);
-  bool runInternal(OutputCollector& collector, const std::vector<UriPtr>& uris,
-                   const UriPtr& request_source_uri, const UriPtr& tracing_uri,
-                   const absl::optional<Envoy::SystemTime>& schedule);
+  absl::Status runInternal(OutputCollector& collector, const std::vector<UriPtr>& uris,
+                           const UriPtr& request_source_uri, const UriPtr& tracing_uri,
+                           const absl::optional<Envoy::SystemTime>& schedule);
 
   /**
    * Compute the offset at which execution should start. We adhere to the scheduled start passed in

source/client/remote_process_impl.cc

@@ -14,6 +14,8 @@
 
 #include "client/options_impl.h"
 
+#include "absl/status/status.h"
+
 namespace Nighthawk {
 namespace Client {
 
@@ -22,7 +24,7 @@ RemoteProcessImpl::RemoteProcessImpl(const Options& options,
     : options_(options), service_client_(std::make_unique<NighthawkServiceClientImpl>()),
       stub_(stub) {}
 
-bool RemoteProcessImpl::run(OutputCollector& collector) {
+absl::Status RemoteProcessImpl::run(OutputCollector& collector) {
   Nighthawk::Client::CommandLineOptionsPtr options = options_.toCommandLineOptions();
   // We don't forward the option that requests remote execution. Today,
   // nighthawk_service will ignore the option, but if someone ever changes that this
@@ -33,10 +35,11 @@ bool RemoteProcessImpl::run(OutputCollector& collector) {
       service_client_->PerformNighthawkBenchmark(&stub_, *options);
   if (result.ok()) {
     collector.setOutput(result.value().output());
-    return true;
+    return absl::OkStatus();
   }
   ENVOY_LOG(error, "Remote execution failure: {}", result.status().message());
-  return false;
+  return absl::Status(static_cast<absl::StatusCode>(result.status().code()),
+                      absl::StrCat("Remote execution failure: ", result.status().message()));
 }
 
 bool RemoteProcessImpl::requestExecutionCancellation() {

source/client/remote_process_impl.h

@@ -26,10 +26,9 @@ class RemoteProcessImpl : public Process, public Envoy::Logger::Loggable<Envoy::
   RemoteProcessImpl(const Options& options, nighthawk::client::NighthawkService::Stub& stub);
   /**
    * @param collector Collects the output from the remote nighthawk service.
-   * @return true iff the remote execution should be considered successful. Unsuccessful execution
-   * will log available error details.
+   * @return OK if execution was successful, otherwise error details.
    */
-  bool run(OutputCollector& collector) override;
+  absl::Status run(OutputCollector& collector) override;
   /**
    * Shuts down the service, a no-op in this implementation.
    */

source/client/service_impl.cc

@@ -37,12 +37,10 @@ void ServiceImpl::handleExecutionRequest(const nighthawk::client::ExecutionReque
 
   ProcessImpl process(*options, time_system_, process_wide_);
   OutputCollectorImpl output_collector(time_system_, *options);
-  const bool ok = process.run(output_collector);
-  if (!ok) {
-    response.mutable_error_detail()->set_code(grpc::StatusCode::INTERNAL);
-    // TODO(https://github.com/envoyproxy/nighthawk/issues/181): wire through error descriptions, so
-    // we can do better here.
-    response.mutable_error_detail()->set_message("Unknown failure");
+  const absl::Status run_status = process.run(output_collector);
+  if (!run_status.ok()) {
+    response.mutable_error_detail()->set_code(static_cast<int>(run_status.code()));
+    response.mutable_error_detail()->set_message(std::string(run_status.message()));
   }
   *(response.mutable_output()) = output_collector.toProto();
   process.shutdown();

test/integration/BUILD

@@ -20,6 +20,7 @@ py_library(
         "//:nighthawk_output_transform",
         "//:nighthawk_service",
         "//:nighthawk_test_server",
+        "//test/request_source:testdata",
         "@envoy//test/config/integration/certs",
     ],
     deps = [

test/options_test.cc

@@ -297,6 +297,7 @@ TEST_P(RequestSourcePluginTestFixture, CreatesOptionsImplWithRequestSourceConfig
   // comparison below.
   EXPECT_EQ(1, command->mutable_failure_predicates()->erase("benchmark.http_4xx"));
   EXPECT_EQ(1, command->mutable_failure_predicates()->erase("benchmark.http_5xx"));
+  EXPECT_EQ(1, command->mutable_failure_predicates()->erase("benchmark.stream_resets"));
   EXPECT_EQ(1, command->mutable_failure_predicates()->erase("requestsource.upstream_rq_5xx"));
 
   // TODO(#433)
@@ -438,6 +439,7 @@ TEST_F(OptionsImplTest, TlsContext) {
   // comparison below.
   EXPECT_EQ(1, cmd->mutable_failure_predicates()->erase("benchmark.http_4xx"));
   EXPECT_EQ(1, cmd->mutable_failure_predicates()->erase("benchmark.http_5xx"));
+  EXPECT_EQ(1, cmd->mutable_failure_predicates()->erase("benchmark.stream_resets"));
   EXPECT_EQ(1, cmd->mutable_failure_predicates()->erase("requestsource.upstream_rq_5xx"));
   // TODO(#433)
   OptionsImpl options_from_proto(*cmd);
@@ -500,6 +502,7 @@ TEST_F(OptionsImplTest, MultiTarget) {
   // textual comparison below.
   EXPECT_EQ(1, cmd->mutable_failure_predicates()->erase("benchmark.http_4xx"));
   EXPECT_EQ(1, cmd->mutable_failure_predicates()->erase("benchmark.http_5xx"));
+  EXPECT_EQ(1, cmd->mutable_failure_predicates()->erase("benchmark.stream_resets"));
   EXPECT_EQ(1, cmd->mutable_failure_predicates()->erase("requestsource.upstream_rq_5xx"));
   // TODO(#433)
   OptionsImpl options_from_proto(*cmd);

test/process_test.cc

@@ -65,14 +65,12 @@ class FakeStatsSinkFactory : public NighthawkStatsSinkFactory {
 // expect failure.
 class ProcessTest : public TestWithParam<Envoy::Network::Address::IpVersion> {
 public:
-  enum class RunExpectation { EXPECT_SUCCESS, EXPECT_FAILURE };
-
   ProcessTest()
       : loopback_address_(Envoy::Network::Test::getLoopbackAddressUrlString(GetParam())),
         options_(TestUtility::createOptionsImpl(
             fmt::format("foo --duration 1 -v error --rps 10 https://{}/", loopback_address_))){};
 
-  void runProcess(RunExpectation expectation, bool do_cancel = false,
+  void runProcess(absl::StatusCode expected_status_code, bool do_cancel = false,
                   bool terminate_right_away = false) {
     ProcessPtr process = std::make_unique<ProcessImpl>(*options_, time_system_);
     OutputCollectorImpl collector(time_system_, *options_);
@@ -91,9 +89,8 @@ class ProcessTest : public TestWithParam<Envoy::Network::Address::IpVersion> {
         cancel_thread.join();
       }
     }
-    const auto result =
-        process->run(collector) ? RunExpectation::EXPECT_SUCCESS : RunExpectation::EXPECT_FAILURE;
-    EXPECT_EQ(result, expectation);
+    absl::Status run_status = process->run(collector);
+    EXPECT_EQ(run_status.code(), expected_status_code);
     if (do_cancel) {
       if (cancel_thread.joinable()) {
         cancel_thread.join();
@@ -126,17 +123,17 @@ INSTANTIATE_TEST_SUITE_P(IpVersions, ProcessTest,
                          Envoy::TestUtility::ipTestParamsToString);
 
 TEST_P(ProcessTest, TwoProcessInSequence) {
-  runProcess(RunExpectation::EXPECT_FAILURE);
+  runProcess(absl::StatusCode::kInternal);
   options_ = TestUtility::createOptionsImpl(
       fmt::format("foo --h2 --duration 1 --rps 10 https://{}/", loopback_address_));
-  runProcess(RunExpectation::EXPECT_FAILURE);
+  runProcess(absl::StatusCode::kInternal);
 }
 
 // TODO(oschaaf): move to python int. tests once it adds to coverage.
 TEST_P(ProcessTest, BadTracerSpec) {
   options_ = TestUtility::createOptionsImpl(
       fmt::format("foo --trace foo://localhost:79/api/v1/spans https://{}/", loopback_address_));
-  runProcess(RunExpectation::EXPECT_FAILURE);
+  runProcess(absl::StatusCode::kInvalidArgument);
 }
 
 TEST_P(ProcessTest, CancelDuringLoadTest) {
@@ -146,14 +143,14 @@ TEST_P(ProcessTest, CancelDuringLoadTest) {
   options_ = TestUtility::createOptionsImpl(
       fmt::format("foo --duration 300 --failure-predicate foo:0 --concurrency 2 https://{}/",
                   loopback_address_));
-  runProcess(RunExpectation::EXPECT_SUCCESS, true);
+  runProcess(absl::StatusCode::kOk, /*do_cancel=*/true);
 }
 
 TEST_P(ProcessTest, CancelExecutionBeforeBeginLoadTest) {
   options_ = TestUtility::createOptionsImpl(
       fmt::format("foo --duration 300 --failure-predicate foo:0 --concurrency 2 https://{}/",
                   loopback_address_));
-  runProcess(RunExpectation::EXPECT_SUCCESS, true, true);
+  runProcess(absl::StatusCode::kOk, /*do_cancel=*/true, /*terminate_right_away=*/true);
 }
 
 TEST_P(ProcessTest, RunProcessWithStatsSinkConfigured) {
@@ -164,7 +161,7 @@ TEST_P(ProcessTest, RunProcessWithStatsSinkConfigured) {
                   "--stats-sinks {} https://{}/",
                   kSinkName, loopback_address_));
   numFlushes = 0;
-  runProcess(RunExpectation::EXPECT_FAILURE);
+  runProcess(absl::StatusCode::kInternal);
   EXPECT_GT(numFlushes, 0);
 }
 
@@ -176,7 +173,7 @@ TEST_P(ProcessTest, NoFlushWhenCancelExecutionBeforeLoadTestBegin) {
                   "2 --stats-flush-interval 1 --stats-sinks {} https://{}/",
                   kSinkName, loopback_address_));
   numFlushes = 0;
-  runProcess(RunExpectation::EXPECT_SUCCESS, true, true);
+  runProcess(absl::StatusCode::kOk, /*do_cancel=*/true, /*terminate_right_away=*/true);
   EXPECT_EQ(numFlushes, 0);
 }
 
@@ -196,9 +193,9 @@ class ProcessTestWithSimTime : public Envoy::Event::TestUsingSimulatedTime,
     auto run_thread = std::thread([this, &verify_callback] {
       ProcessPtr process = std::make_unique<ProcessImpl>(*options_, simTime());
       OutputCollectorImpl collector(simTime(), *options_);
-      const bool result = process->run(collector);
+      const absl::Status result = process->run(collector);
       process->shutdown();
-      verify_callback(result, collector.toProto());
+      verify_callback(result.ok(), collector.toProto());
     });
 
     // We introduce real-world sleeps to give the executing ProcessImpl