diff --git a/ci/do_ci.sh b/ci/do_ci.sh index 11128b3c4..d34e58d4b 100755 --- a/ci/do_ci.sh +++ b/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 diff --git a/include/nighthawk/client/output_formatter.h b/include/nighthawk/client/output_formatter.h index d28fffb0b..cd0eab433 100644 --- a/include/nighthawk/client/output_formatter.h +++ b/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 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 + formatProto(const nighthawk::client::Output& output) const PURE; }; using OutputFormatterPtr = std::unique_ptr; diff --git a/source/client/BUILD b/source/client/BUILD index f783e2f45..774d42156 100644 --- a/source/client/BUILD +++ b/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", ], diff --git a/source/client/client.cc b/source/client/client.cc index cd3d0b14f..174e9250b 100644 --- a/source/client/client.cc +++ b/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 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."); diff --git a/source/client/output_formatter_impl.cc b/source/client/output_formatter_impl.cc index 45101ae54..d8df09bae 100644 --- a/source/client/output_formatter_impl.cc +++ b/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 +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 +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 +YamlOutputFormatterImpl::formatProto(const nighthawk::client::Output& output) const { return Envoy::MessageUtil::getYamlStringFromMessage(output, true, true); } -std::string +absl::StatusOr DottedStringOutputFormatterImpl::formatProto(const nighthawk::client::Output& output) const { std::stringstream ss; for (const auto& result : output.results()) { @@ -206,7 +210,7 @@ DottedStringOutputFormatterImpl::formatProto(const nighthawk::client::Output& ou return ss.str(); } -const nighthawk::client::Result& +absl::optional FortioOutputFormatterImpl::getGlobalResult(const nighthawk::client::Output& output) const { for (const auto& nh_result : output.results()) { if (nh_result.name() == "global") { @@ -214,7 +218,7 @@ FortioOutputFormatterImpl::getGlobalResult(const nighthawk::client::Output& outp } } - 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 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 +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 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 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 FortioPedanticOutputFormatterImpl::formatProto(const nighthawk::client::Output& output) const { - std::string res = FortioOutputFormatterImpl::formatProto(output); + absl::StatusOr 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. diff --git a/source/client/output_formatter_impl.h b/source/client/output_formatter_impl.h index 37fa96d1d..1ad9168c5 100644 --- a/source/client/output_formatter_impl.h +++ b/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 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 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 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 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 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 + 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 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 formatProto(const nighthawk::client::Output& output) const override; }; } // namespace Client diff --git a/source/client/output_transform_main.cc b/source/client/output_transform_main.cc index 144153f73..b5eb20b80 100644 --- a/source/client/output_transform_main.cc +++ b/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 format_status = formatter->formatProto(output); + if (!format_status.ok()) { + ENVOY_LOG(error, "error while formatting proto"); + return 1; + } + std::cout << *format_status; return 0; } diff --git a/test/output_formatter_test.cc b/test/output_formatter_test.cc index 9b334fcc7..6c720ea29 100644 --- a/test/output_formatter_test.cc +++ b/test/output_formatter_test.cc @@ -100,25 +100,27 @@ class OutputCollectorTest : public Test { TEST_F(OutputCollectorTest, CliFormatter) { ConsoleOutputFormatterImpl formatter; - expectEqualToGoldFile(formatter.formatProto(collector_->toProto()), + expectEqualToGoldFile(*(formatter.formatProto(collector_->toProto())), "test/test_data/output_formatter.txt.gold"); } TEST_F(OutputCollectorTest, JsonFormatter) { JsonOutputFormatterImpl formatter; - expectEqualToGoldFile(formatter.formatProto(collector_->toProto()), + EXPECT_EQ((formatter.formatProto(collector_->toProto())).ok(), true); + expectEqualToGoldFile((formatter.formatProto(collector_->toProto())).value(), "test/test_data/output_formatter.json.gold"); } TEST_F(OutputCollectorTest, YamlFormatter) { YamlOutputFormatterImpl formatter; - expectEqualToGoldFile(formatter.formatProto(collector_->toProto()), + EXPECT_EQ((formatter.formatProto(collector_->toProto())).ok(), true); + expectEqualToGoldFile((formatter.formatProto(collector_->toProto())).value(), "test/test_data/output_formatter.yaml.gold"); } TEST_F(OutputCollectorTest, DottedFormatter) { DottedStringOutputFormatterImpl formatter; - expectEqualToGoldFile(formatter.formatProto(collector_->toProto()), + expectEqualToGoldFile((formatter.formatProto(collector_->toProto())).value(), "test/test_data/output_formatter.dotted.gold"); } @@ -151,27 +153,38 @@ TEST_F(FortioOutputCollectorTest, MissingGlobalResult) { output_proto.clear_results(); FortioOutputFormatterImpl formatter; - ASSERT_THROW(formatter.formatProto(output_proto), NighthawkException); + EXPECT_FALSE((formatter.formatProto(output_proto)).ok()); +} + +TEST_F(FortioOutputCollectorTest, MissingGlobalResultGetGlobalResult) { + nighthawk::client::Output output_proto = collector_->toProto(); + output_proto.clear_results(); + + FortioOutputFormatterImpl formatter; + EXPECT_FALSE((formatter.getGlobalResult(output_proto)).has_value()); } TEST_F(FortioOutputCollectorTest, MissingCounter) { nighthawk::client::Output output_proto = collector_->toProto(); output_proto.mutable_results(2)->clear_counters(); FortioOutputFormatterImpl formatter; - ASSERT_NO_THROW(formatter.formatProto(output_proto)); + EXPECT_TRUE((formatter.formatProto(output_proto)).ok()); + ASSERT_NO_THROW((formatter.formatProto(output_proto)).value()); } TEST_F(FortioOutputCollectorTest, MissingStatistic) { nighthawk::client::Output output_proto = collector_->toProto(); output_proto.mutable_results(2)->clear_statistics(); FortioOutputFormatterImpl formatter; - ASSERT_NO_THROW(formatter.formatProto(output_proto)); + EXPECT_TRUE((formatter.formatProto(output_proto)).ok()); + ASSERT_NO_THROW((formatter.formatProto(output_proto)).value()); } TEST_F(FortioOutputCollectorTest, NoExceptions) { nighthawk::client::Output output_proto = collector_->toProto(); FortioOutputFormatterImpl formatter; - ASSERT_NO_THROW(formatter.formatProto(output_proto)); + EXPECT_TRUE((formatter.formatProto(output_proto)).ok()); + ASSERT_NO_THROW((formatter.formatProto(output_proto)).value()); } class MediumOutputCollectorTest : public OutputCollectorTest { @@ -190,7 +203,7 @@ TEST_F(MediumOutputCollectorTest, FortioFormatter) { const nighthawk::client::Output input_proto = loadProtoFromFile("test/test_data/output_formatter.medium.proto.gold"); FortioOutputFormatterImpl formatter; - expectEqualToGoldFile(formatter.formatProto(input_proto), + expectEqualToGoldFile((formatter.formatProto(input_proto)).value(), "test/test_data/output_formatter.medium.fortio.gold"); } @@ -200,14 +213,15 @@ TEST_F(MediumOutputCollectorTest, FortioFormatter0sJitterUniformGetsReflected) { FortioOutputFormatterImpl formatter; input_proto.mutable_options()->mutable_jitter_uniform()->set_nanos(0); input_proto.mutable_options()->mutable_jitter_uniform()->set_seconds(0); - EXPECT_NE(formatter.formatProto(input_proto).find(" \"Jitter\": false,"), std::string::npos); + EXPECT_NE((formatter.formatProto(input_proto)).value().find(" \"Jitter\": false,"), + std::string::npos); } TEST_F(MediumOutputCollectorTest, ConsoleOutputFormatter) { const nighthawk::client::Output input_proto = loadProtoFromFile("test/test_data/percentile-column-overflow.json"); ConsoleOutputFormatterImpl formatter; - expectEqualToGoldFile(formatter.formatProto(input_proto), + expectEqualToGoldFile((formatter.formatProto(input_proto)).value(), "test/test_data/percentile-column-overflow.txt.gold"); } @@ -231,9 +245,17 @@ TEST_F(MediumOutputCollectorTest, FortioPedanticFormatter) { const nighthawk::client::Output input_proto = loadProtoFromFile("test/test_data/output_formatter.medium.proto.gold"); FortioPedanticOutputFormatterImpl formatter; - expectEqualToGoldFile(formatter.formatProto(input_proto), + expectEqualToGoldFile((formatter.formatProto(input_proto)).value(), "test/test_data/output_formatter.medium.fortio-noquirks.gold"); } +TEST_F(MediumOutputCollectorTest, FortioPedanticFormatterMissingGlobalResult) { + nighthawk::client::Output output_proto = collector_->toProto(); + output_proto.clear_results(); + + FortioPedanticOutputFormatterImpl formatter; + EXPECT_FALSE((formatter.formatProto(output_proto)).ok()); +} + } // namespace Client } // namespace Nighthawk diff --git a/test/output_transform_main_test.cc b/test/output_transform_main_test.cc index 6ae792bba..14710e322 100644 --- a/test/output_transform_main_test.cc +++ b/test/output_transform_main_test.cc @@ -65,7 +65,7 @@ TEST_F(OutputTransformMainTest, HappyFlowForAllOutputFormats) { output.add_results()->set_name("global"); } output.mutable_options()->mutable_uri()->set_value("http://127.0.0.1/"); - stream_ << Envoy::MessageUtil::getJsonStringFromMessageOrDie(output, true, true); + stream_ << Envoy::MessageUtil::getJsonStringFromMessageOrError(output, true, true); OutputTransformMain main(argv.size(), argv.data(), stream_); EXPECT_EQ(main.run(), 0); } diff --git a/test/statistic_test.cc b/test/statistic_test.cc index 2378969ec..a26461608 100644 --- a/test/statistic_test.cc +++ b/test/statistic_test.cc @@ -343,9 +343,10 @@ TEST(StatisticTest, HdrStatisticPercentilesProto) { util.loadFromJson(Envoy::Filesystem::fileSystemForTest().fileReadToEnd( TestEnvironment::runfilesPath("test/test_data/hdr_proto_json.gold")), parsed_json_proto, Envoy::ProtobufMessage::getStrictValidationVisitor()); - const std::string json = util.getJsonStringFromMessageOrDie( + const std::string json = util.getJsonStringFromMessageOrError( statistic.toProto(Statistic::SerializationDomain::DURATION), true, true); - const std::string golden_json = util.getJsonStringFromMessageOrDie(parsed_json_proto, true, true); + const std::string golden_json = + util.getJsonStringFromMessageOrError(parsed_json_proto, true, true); EXPECT_THAT(statistic.toProto(Statistic::SerializationDomain::DURATION), Envoy::ProtoEq(parsed_json_proto)) << json << "\n" @@ -365,9 +366,10 @@ TEST(StatisticTest, CircllhistStatisticPercentilesProto) { util.loadFromJson(Envoy::Filesystem::fileSystemForTest().fileReadToEnd( TestEnvironment::runfilesPath("test/test_data/circllhist_proto_json.gold")), parsed_json_proto, Envoy::ProtobufMessage::getStrictValidationVisitor()); - const std::string json = util.getJsonStringFromMessageOrDie( + const std::string json = util.getJsonStringFromMessageOrError( statistic.toProto(Statistic::SerializationDomain::DURATION), true, true); - const std::string golden_json = util.getJsonStringFromMessageOrDie(parsed_json_proto, true, true); + const std::string golden_json = + util.getJsonStringFromMessageOrError(parsed_json_proto, true, true); EXPECT_THAT(statistic.toProto(Statistic::SerializationDomain::DURATION), Envoy::ProtoEq(parsed_json_proto)) << json << "\n"