Better error handling using absl::StatusOr

ci/do_ci.sh

@@ -41,7 +41,7 @@ function do_clang_tidy() {
 
 function do_unit_test_coverage() {
     export TEST_TARGETS="//test/... -//test:python_test"
-    export COVERAGE_THRESHOLD=94.2
+    export COVERAGE_THRESHOLD=94.1
     echo "bazel coverage build with tests ${TEST_TARGETS}"
     test/run_nighthawk_bazel_coverage.sh ${TEST_TARGETS}
     exit 0

include/nighthawk/client/output_formatter.h

@@ -8,6 +8,9 @@
 
 #include "api/client/output.pb.validate.h"
 
+#include "absl/status/status.h"
+#include "absl/status/statusor.h"
+
 namespace Nighthawk {
 namespace Client {
 
@@ -19,10 +22,11 @@ class OutputFormatter {
   virtual ~OutputFormatter() = default;
 
   /**
-   * @return std::string serialized representation of output. The specific format depends
-   * on the derived class, for example human-readable or json.
+   * @return absl::StatusOr<std::string> serialized representation of output, or an error.
+   * The specific format depends on the derived class, for example human-readable or json.
    */
-  virtual std::string formatProto(const nighthawk::client::Output& output) const PURE;
+  virtual absl::StatusOr<std::string>
+  formatProto(const nighthawk::client::Output& output) const PURE;
 };
 
 using OutputFormatterPtr = std::unique_ptr<OutputFormatter>;

source/client/BUILD

@@ -152,6 +152,7 @@ envoy_cc_library(
         "//api/client/transform:transform_cc_proto",
         "//include/nighthawk/client:client_includes",
         "//source/common:nighthawk_common_lib",
+        "@com_google_googletest//:gtest",
         "@envoy//source/common/network:utility_lib_with_external_headers",
         "@envoy//source/common/protobuf:utility_lib_with_external_headers",
     ],

source/client/client.cc

@@ -34,6 +34,9 @@
 #include "client/process_impl.h"
 #include "client/remote_process_impl.h"
 
+#include "absl/status/status.h"
+#include "absl/status/statusor.h"
+
 using namespace std::chrono_literals;
 
 namespace Nighthawk {
@@ -81,7 +84,13 @@ bool Main::run() {
     result = process->run(output_collector);
   }
   auto formatter = output_formatter_factory.create(options_->outputFormat());
-  std::cout << formatter->formatProto(output_collector.toProto());
+  absl::StatusOr<std::string> formatted_proto = formatter->formatProto(output_collector.toProto());
+  if (!formatted_proto.ok()) {
+    ENVOY_LOG(error, "An error occured while formatting proto");
+    result = false;
+  } else {
+    std::cout << *formatted_proto;
+  }
   process->shutdown();
   if (!result) {
     ENVOY_LOG(error, "An error ocurred.");

source/client/output_formatter_impl.cc

@@ -14,6 +14,7 @@
 
 #include "common/version_info.h"
 
+#include "absl/status/statusor.h"
 #include "absl/strings/str_cat.h"
 #include "absl/strings/strip.h"
 
@@ -51,7 +52,8 @@ void OutputFormatterImpl::iteratePercentiles(
   }
 }
 
-std::string ConsoleOutputFormatterImpl::formatProto(const nighthawk::client::Output& output) const {
+absl::StatusOr<std::string>
+ConsoleOutputFormatterImpl::formatProto(const nighthawk::client::Output& output) const {
   std::stringstream ss;
   ss << "Nighthawk - A layer 7 protocol benchmarking tool." << std::endl << std::endl;
   for (const auto& result : output.results()) {
@@ -142,15 +144,17 @@ std::string ConsoleOutputFormatterImpl::statIdtoFriendlyStatName(absl::string_vi
   return std::string(stat_id);
 }
 
-std::string JsonOutputFormatterImpl::formatProto(const nighthawk::client::Output& output) const {
-  return Envoy::MessageUtil::getJsonStringFromMessageOrDie(output, true, true);
+absl::StatusOr<std::string>
+JsonOutputFormatterImpl::formatProto(const nighthawk::client::Output& output) const {
+  return Envoy::MessageUtil::getJsonStringFromMessage(output, true, true).value();
 }
 
-std::string YamlOutputFormatterImpl::formatProto(const nighthawk::client::Output& output) const {
+absl::StatusOr<std::string>
+YamlOutputFormatterImpl::formatProto(const nighthawk::client::Output& output) const {
   return Envoy::MessageUtil::getYamlStringFromMessage(output, true, true);
 }
 
-std::string
+absl::StatusOr<std::string>
 DottedStringOutputFormatterImpl::formatProto(const nighthawk::client::Output& output) const {
   std::stringstream ss;
   for (const auto& result : output.results()) {
@@ -206,15 +210,15 @@ DottedStringOutputFormatterImpl::formatProto(const nighthawk::client::Output& ou
   return ss.str();
 }
 
-const nighthawk::client::Result&
+absl::optional<const nighthawk::client::Result>
 FortioOutputFormatterImpl::getGlobalResult(const nighthawk::client::Output& output) const {
   for (const auto& nh_result : output.results()) {
     if (nh_result.name() == "global") {
       return nh_result;
     }
   }
 
-  throw NighthawkException("Nighthawk output was malformed, contains no 'global' results.");
+  return absl::nullopt;
 }
 
 uint64_t FortioOutputFormatterImpl::getCounterValue(const nighthawk::client::Result& result,
@@ -239,10 +243,11 @@ FortioOutputFormatterImpl::findStatistic(const nighthawk::client::Result& result
   return nullptr;
 }
 
-std::chrono::nanoseconds FortioOutputFormatterImpl::getAverageExecutionDuration(
+absl::StatusOr<std::chrono::nanoseconds> FortioOutputFormatterImpl::getAverageExecutionDuration(
     const nighthawk::client::Output& output) const {
   if (!output.results_size()) {
-    throw NighthawkException("No results in output");
+    return absl::Status(absl::StatusCode::kNotFound,
+                        "getAverageExecutionDuration found no results in output");
   }
   const auto& r = output.results().at(output.results_size() - 1);
   ASSERT(r.name() == "global");
@@ -255,7 +260,8 @@ FortioOutputFormatterImpl::durationToSeconds(const Envoy::ProtobufWkt::Duration&
   return Envoy::Protobuf::util::TimeUtil::DurationToNanoseconds(duration) / 1e9;
 }
 
-std::string FortioOutputFormatterImpl::formatProto(const nighthawk::client::Output& output) const {
+absl::StatusOr<std::string>
+FortioOutputFormatterImpl::formatProto(const nighthawk::client::Output& output) const {
   nighthawk::client::FortioResult fortio_output;
   // Iff there's only a single worker we will have only a single result. Otherwise the number of
   // workers can be derived by substracting one from the number of results (for the
@@ -272,7 +278,13 @@ std::string FortioOutputFormatterImpl::formatProto(const nighthawk::client::Outp
                                  output.options().requests_per_second().value());
   fortio_output.set_url(output.options().uri().value());
   *fortio_output.mutable_requestedduration() = output.options().duration();
-  auto actual_duration = getAverageExecutionDuration(output);
+  absl::StatusOr<std::chrono::nanoseconds> actual_duration_status =
+      getAverageExecutionDuration(output);
+  if (!actual_duration_status.ok()) {
+    return absl::Status(actual_duration_status.status().code(),
+                        actual_duration_status.status().message());
+  }
+  std::chrono::nanoseconds actual_duration = *actual_duration_status;
   fortio_output.set_actualduration(actual_duration.count());
   fortio_output.set_jitter(output.options().has_jitter_uniform() &&
                            (output.options().jitter_uniform().nanos() > 0 ||
@@ -292,7 +304,12 @@ std::string FortioOutputFormatterImpl::formatProto(const nighthawk::client::Outp
   fortio_output.set_numthreads(number_of_connections);
 
   // Get the result that represents all workers (global)
-  const auto& nh_global_result = getGlobalResult(output);
+  absl::optional<const nighthawk::client::Result> nh_global_result_optional =
+      getGlobalResult(output);
+  if (!nh_global_result_optional.has_value()) {
+    return absl::Status(absl::StatusCode::kNotFound, "formatProto global result not found");
+  }
+  const auto& nh_global_result = nh_global_result_optional.value();
 
   // Fill in the actual QPS based on the counters
   const double actual_qps =
@@ -320,7 +337,7 @@ std::string FortioOutputFormatterImpl::formatProto(const nighthawk::client::Outp
   if (statistic != nullptr) {
     fortio_output.mutable_headersizes()->CopyFrom(renderFortioDurationHistogram(*statistic));
   }
-  return Envoy::MessageUtil::getJsonStringFromMessageOrDie(fortio_output, true, true);
+  return Envoy::MessageUtil::getJsonStringFromMessage(fortio_output, true, true).value();
 }
 
 const nighthawk::client::DurationHistogram FortioOutputFormatterImpl::renderFortioDurationHistogram(
@@ -394,9 +411,13 @@ const nighthawk::client::DurationHistogram FortioOutputFormatterImpl::renderFort
   return fortio_histogram;
 }
 
-std::string
+absl::StatusOr<std::string>
 FortioPedanticOutputFormatterImpl::formatProto(const nighthawk::client::Output& output) const {
-  std::string res = FortioOutputFormatterImpl::formatProto(output);
+  absl::StatusOr<std::string> res_status = FortioOutputFormatterImpl::formatProto(output);
+  if (!res_status.ok()) {
+    return res_status;
+  }
+  std::string res = *res_status;
   // clang-format off
   // Fix two types of quirks. We disable linting because we use std::regex directly.
   // This should be OK as the regular expression we use can be trusted.

source/client/output_formatter_impl.h

@@ -6,12 +6,16 @@
 
 #include "nighthawk/client/output_formatter.h"
 
+#include "external/com_google_googletest/googletest/include/gtest/gtest_prod.h"
 #include "external/envoy/source/common/protobuf/protobuf.h"
 
 #include "api/client/output.pb.h"
 #include "api/client/transform/fortio.pb.h"
 
+#include "absl/status/status.h"
+#include "absl/status/statusor.h"
 #include "absl/strings/string_view.h"
+#include "absl/types/optional.h"
 
 namespace Nighthawk {
 namespace Client {
@@ -28,7 +32,7 @@ class OutputFormatterImpl : public OutputFormatter {
 
 class ConsoleOutputFormatterImpl : public OutputFormatterImpl {
 public:
-  std::string formatProto(const nighthawk::client::Output& output) const override;
+  absl::StatusOr<std::string> formatProto(const nighthawk::client::Output& output) const override;
   static std::string statIdtoFriendlyStatName(absl::string_view stat_id);
 
 private:
@@ -37,32 +41,34 @@ class ConsoleOutputFormatterImpl : public OutputFormatterImpl {
 
 class JsonOutputFormatterImpl : public OutputFormatterImpl {
 public:
-  std::string formatProto(const nighthawk::client::Output& output) const override;
+  absl::StatusOr<std::string> formatProto(const nighthawk::client::Output& output) const override;
 };
 
 class YamlOutputFormatterImpl : public OutputFormatterImpl {
 public:
-  std::string formatProto(const nighthawk::client::Output& output) const override;
+  absl::StatusOr<std::string> formatProto(const nighthawk::client::Output& output) const override;
 };
 
 class DottedStringOutputFormatterImpl : public OutputFormatterImpl {
 public:
-  std::string formatProto(const nighthawk::client::Output& output) const override;
+  absl::StatusOr<std::string> formatProto(const nighthawk::client::Output& output) const override;
 };
 
 class FortioOutputFormatterImpl : public OutputFormatterImpl {
+  FRIEND_TEST(FortioOutputCollectorTest, MissingGlobalResultGetGlobalResult);
+
 public:
-  std::string formatProto(const nighthawk::client::Output& output) const override;
+  absl::StatusOr<std::string> formatProto(const nighthawk::client::Output& output) const override;
 
 protected:
   /**
    * Return the result that represents all workers (the one with the "global" name).
    *
    * @param output the Nighthawk output proto
-   * @return the corresponding global result
-   * @throws NighthawkException if global result is not found
+   * @return the corresponding global result, or absl::Status if failed
    */
-  const nighthawk::client::Result& getGlobalResult(const nighthawk::client::Output& output) const;
+  absl::optional<const nighthawk::client::Result>
+  getGlobalResult(const nighthawk::client::Output& output) const;
 
   /**
    * Return the counter with the specified name.
@@ -97,7 +103,7 @@ class FortioOutputFormatterImpl : public OutputFormatterImpl {
    * @param output the Nighthawk output proto
    * @return the corresponding average execution duration in nanoseconds
    */
-  std::chrono::nanoseconds
+  absl::StatusOr<std::chrono::nanoseconds>
   getAverageExecutionDuration(const nighthawk::client::Output& output) const;
 
   /**
@@ -126,7 +132,7 @@ class FortioPedanticOutputFormatterImpl : public FortioOutputFormatterImpl {
    * @param output Nighthawk's native output proto that will be transformed.
    * @return std::string Fortio formatted json string.
    */
-  std::string formatProto(const nighthawk::client::Output& output) const override;
+  absl::StatusOr<std::string> formatProto(const nighthawk::client::Output& output) const override;
 };
 
 } // namespace Client

source/client/output_transform_main.cc

@@ -58,7 +58,12 @@ uint32_t OutputTransformMain::run() {
   }
   OutputFormatterFactoryImpl factory;
   OutputFormatterPtr formatter = factory.create(translated_format);
-  std::cout << formatter->formatProto(output);
+  absl::StatusOr<std::string> format_status = formatter->formatProto(output);
+  if (!format_status.ok()) {
+    ENVOY_LOG(error, "error while formatting proto");
+    return 1;
+  }
+  std::cout << *format_status;
   return 0;
 }