From 694f87fcd3b0ac335a8cb6e9930180b33efafd73 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Mon, 14 Sep 2020 15:30:16 +0200 Subject: [PATCH 1/5] Add experimental "pedantic" fortio output formatter. Adds a post-processing step to massage the fortio output formatting to more exactly reproduce Fortio's output, while leaving the original output formatter intact for backwards compatibility purposes. Fixes known issues #422 and fixes #514. Experimental until we have some conformation that this can be marked as final / all issues have been resolved. Signed-off-by: Otto van der Schaaf --- README.md | 19 +- api/client/options.proto | 1 + source/client/output_formatter_impl.cc | 21 + source/client/output_formatter_impl.h | 5 + test/BUILD | 1 + test/output_formatter_test.cc | 10 +- ...tput_formatter.medium.fortio-noquirks.gold | 483 ++++++++++++++++++ 7 files changed, 532 insertions(+), 8 deletions(-) create mode 100644 test/test_data/output_formatter.medium.fortio-noquirks.gold diff --git a/README.md b/README.md index 7b87c3a14..e5bdb24ba 100644 --- a/README.md +++ b/README.md @@ -70,7 +70,8 @@ format>] [--sequencer-idle-strategy ] [--address-family ] [--burst-size ] [--prefetch-connections] [--output-format -] [-v ] [-v ] [--concurrency ] [--h2] [--timeout ] [--duration ] @@ -220,9 +221,11 @@ Release requests in bursts of the specified size (default: 0). --prefetch-connections Use proactive connection prefetching (HTTP/1 only). ---output-format +--output-format Output format. Possible values: {"json", "human", "yaml", "dotted", -"fortio"}. The default output format is 'human'. +"fortio", "experimental_fortio_pedantic"}. The default output format +is 'human'. -v , --verbosity @@ -329,15 +332,17 @@ Nighthawk comes with a tool to transform its json output to its other supported USAGE: bazel-bin/nighthawk_output_transform --output-format [--] [--version] -[-h] +|dotted|fortio +|experimental_fortio_pedantic> [--] +[--version] [-h] Where: ---output-format +--output-format (required) Output format. Possible values: {"json", "human", "yaml", -"dotted", "fortio"}. +"dotted", "fortio", "experimental_fortio_pedantic"}. --, --ignore_rest Ignores the rest of the labeled arguments following this flag. diff --git a/api/client/options.proto b/api/client/options.proto index 3cf370141..6e2aa1841 100644 --- a/api/client/options.proto +++ b/api/client/options.proto @@ -58,6 +58,7 @@ message OutputFormat { YAML = 3; DOTTED = 4; FORTIO = 5; + EXPERIMENTAL_FORTIO_PEDANTIC = 6; } OutputFormatOptions value = 1; } diff --git a/source/client/output_formatter_impl.cc b/source/client/output_formatter_impl.cc index 81276ae84..b51867acc 100644 --- a/source/client/output_formatter_impl.cc +++ b/source/client/output_formatter_impl.cc @@ -3,6 +3,7 @@ #include #include +#include #include #include "nighthawk/common/exception.h" @@ -393,5 +394,25 @@ const nighthawk::client::DurationHistogram FortioOutputFormatterImpl::renderFort return fortio_histogram; } +std::string +FortioPedanticOutputFormatterImpl::formatProto(const nighthawk::client::Output& output) const { + std::string res = FortioOutputFormatterImpl::formatProto(output); + // 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. + + // clang-format off + // 1. We misdefined RequestRPS as an int, whereas Fortio outputs that as a string. + res = std::regex_replace(res, std::regex(R"EOF("RequestedQPS"\: ([0-9]*))EOF"), + "\"RequestedQPS\": \"$1\""); + // 2. Our uint64's get serialized as json strings. Fortio outputs them as json integers. + // NOTE: [0-9][0-9][0-9] looks for string fields referring to http status codes, which get + // counted. + res = std::regex_replace( + res, std::regex(R"EOF("([0-9][0-9][0-9]|Count|BytesSent|BytesReceived)"\: "([0-9]*)")EOF"), + "\"$1\": $2"); + // clang-format on + return res; +} + } // namespace Client } // namespace Nighthawk \ No newline at end of file diff --git a/source/client/output_formatter_impl.h b/source/client/output_formatter_impl.h index ff21bc44f..5ac74a307 100644 --- a/source/client/output_formatter_impl.h +++ b/source/client/output_formatter_impl.h @@ -108,5 +108,10 @@ class FortioOutputFormatterImpl : public OutputFormatterImpl { double durationToSeconds(const Envoy::ProtobufWkt::Duration& duration) const; }; +class FortioPedanticOutputFormatterImpl : public FortioOutputFormatterImpl { +public: + std::string formatProto(const nighthawk::client::Output& output) const override; +}; + } // namespace Client } // namespace Nighthawk \ No newline at end of file diff --git a/test/BUILD b/test/BUILD index f88f59e3a..ecb8d5039 100644 --- a/test/BUILD +++ b/test/BUILD @@ -118,6 +118,7 @@ envoy_cc_test( "test_data/output_formatter.dotted.gold", "test_data/output_formatter.json.gold", "test_data/output_formatter.medium.fortio.gold", + "test_data/output_formatter.medium.fortio-noquirks.gold", "test_data/output_formatter.medium.proto.gold", "test_data/output_formatter.txt.gold", "test_data/output_formatter.yaml.gold", diff --git a/test/output_formatter_test.cc b/test/output_formatter_test.cc index d734bddb4..07518f557 100644 --- a/test/output_formatter_test.cc +++ b/test/output_formatter_test.cc @@ -126,7 +126,8 @@ TEST_F(OutputCollectorTest, GetLowerCaseOutputFormats) { auto output_formats = OutputFormatterImpl::getLowerCaseOutputFormats(); // When you're looking at this code you probably just added an output format. // This is to point out that you might want to update the list below and add a test above. - ASSERT_THAT(output_formats, ElementsAre("json", "human", "yaml", "dotted", "fortio")); + ASSERT_THAT(output_formats, ElementsAre("json", "human", "yaml", "dotted", "fortio", + "experimental_fortio_pedantic")); } class FortioOutputCollectorTest : public OutputCollectorTest { @@ -224,5 +225,12 @@ TEST_F(StatidToNameTest, TestTranslations) { } } +TEST_F(MediumOutputCollectorTest, FortioPedanticFormatter) { + const auto input_proto = loadProtoFromFile("test/test_data/output_formatter.medium.proto.gold"); + FortioPedanticOutputFormatterImpl formatter; + expectEqualToGoldFile(formatter.formatProto(input_proto), + "test/test_data/output_formatter.medium.fortio-noquirks.gold"); +} + } // namespace Client } // namespace Nighthawk diff --git a/test/test_data/output_formatter.medium.fortio-noquirks.gold b/test/test_data/output_formatter.medium.fortio-noquirks.gold new file mode 100644 index 000000000..ed8b4c799 --- /dev/null +++ b/test/test_data/output_formatter.medium.fortio-noquirks.gold @@ -0,0 +1,483 @@ +{ + "Labels": "label-a label-b Nighthawk", + "RequestedQPS": "30", + "ActualQPS": 27.99999399400129, + "ActualDuration": 2000000429, + "NumThreads": 300, + "DurationHistogram": { + "Count": 53, + "Data": [ + { + "Start": 0.053798911, + "End": 0.053798911, + "Percent": 0, + "Count": 1 + }, + { + "Start": 0.053798911, + "End": 0.056340479, + "Percent": 10, + "Count": 5 + }, + { + "Start": 0.056340479, + "End": 0.057706495, + "Percent": 20, + "Count": 5 + }, + { + "Start": 0.057706495, + "End": 0.060055551, + "Percent": 30, + "Count": 5 + }, + { + "Start": 0.060055551, + "End": 0.062949375, + "Percent": 40, + "Count": 6 + }, + { + "Start": 0.062949375, + "End": 0.065077247, + "Percent": 50, + "Count": 5 + }, + { + "Start": 0.065077247, + "End": 0.066666495, + "Percent": 55.000000000000007, + "Count": 3 + }, + { + "Start": 0.066666495, + "End": 0.067948543, + "Percent": 60, + "Count": 2 + }, + { + "Start": 0.067948543, + "End": 0.068751359, + "Percent": 65, + "Count": 3 + }, + { + "Start": 0.068751359, + "End": 0.072949759, + "Percent": 70, + "Count": 3 + }, + { + "Start": 0.072949759, + "End": 0.074465279, + "Percent": 75, + "Count": 2 + }, + { + "Start": 0.074465279, + "End": 0.075792383, + "Percent": 77.5, + "Count": 2 + }, + { + "Start": 0.075792383, + "End": 0.078065663, + "Percent": 80, + "Count": 1 + }, + { + "Start": 0.078065663, + "End": 0.078168063, + "Percent": 82.5, + "Count": 1 + }, + { + "Start": 0.078168063, + "End": 0.080400383, + "Percent": 85, + "Count": 2 + }, + { + "Start": 0.080400383, + "End": 0.082427903, + "Percent": 87.5, + "Count": 1 + }, + { + "Start": 0.082427903, + "End": 0.085045247, + "Percent": 88.75, + "Count": 1 + }, + { + "Start": 0.085045247, + "End": 0.085045247, + "Percent": 90, + "Count": 0 + }, + { + "Start": 0.085045247, + "End": 0.085626879, + "Percent": 91.25, + "Count": 1 + }, + { + "Start": 0.085626879, + "End": 0.090656767, + "Percent": 92.5, + "Count": 1 + }, + { + "Start": 0.090656767, + "End": 0.090656767, + "Percent": 93.75, + "Count": 0 + }, + { + "Start": 0.090656767, + "End": 0.247513087, + "Percent": 94.375, + "Count": 1 + }, + { + "Start": 0.247513087, + "End": 0.247513087, + "Percent": 95, + "Count": 0 + }, + { + "Start": 0.247513087, + "End": 0.247513087, + "Percent": 95.625, + "Count": 0 + }, + { + "Start": 0.247513087, + "End": 0.281362431, + "Percent": 96.25, + "Count": 1 + }, + { + "Start": 0.281362431, + "End": 0.281362431, + "Percent": 96.875, + "Count": 0 + }, + { + "Start": 0.281362431, + "End": 0.281362431, + "Percent": 97.1875, + "Count": 0 + }, + { + "Start": 0.281362431, + "End": 0.281362431, + "Percent": 97.5, + "Count": 0 + }, + { + "Start": 0.281362431, + "End": 0.281362431, + "Percent": 97.8125, + "Count": 0 + }, + { + "Start": 0.281362431, + "End": 0.310886399, + "Percent": 98.125, + "Count": 1 + }, + { + "Start": 0.310886399, + "End": 0.310886399, + "Percent": 100, + "Count": 0 + } + ], + "Min": 0.053798911, + "Max": 0.310886399, + "Sum": 4.156954618, + "Avg": 0.078433106, + "StdDev": 0.05052302, + "Percentiles": [ + { + "Percentile": 50, + "Value": 0.065077247 + }, + { + "Percentile": 75, + "Value": 0.074465279 + }, + { + "Percentile": 80, + "Value": 0.078065663 + }, + { + "Percentile": 90, + "Value": 0.085045247 + }, + { + "Percentile": 95, + "Value": 0.247513087 + } + ] + }, + "RetCodes": { + "200": 56 + }, + "URL": "https://www.google.com/", + "Version": "0.0.0", + "Jitter": true, + "RunType": "HTTP", + "Sizes": { + "Count": 56, + "Data": [ + { + "Start": 847, + "End": 847, + "Percent": 0, + "Count": 56 + }, + { + "Start": 847, + "End": 847, + "Percent": 100, + "Count": 0 + } + ], + "Min": 847, + "Max": 847, + "Sum": 47432, + "Avg": 847, + "StdDev": 0, + "Percentiles": [] + }, + "HeaderSizes": { + "Count": 56, + "Data": [ + { + "Start": 47257, + "End": 47257, + "Percent": 0, + "Count": 1 + }, + { + "Start": 47257, + "End": 47289, + "Percent": 10, + "Count": 5 + }, + { + "Start": 47289, + "End": 47299, + "Percent": 20, + "Count": 6 + }, + { + "Start": 47299, + "End": 47305, + "Percent": 30, + "Count": 5 + }, + { + "Start": 47305, + "End": 47311, + "Percent": 40, + "Count": 7 + }, + { + "Start": 47311, + "End": 47317, + "Percent": 50, + "Count": 7 + }, + { + "Start": 47317, + "End": 47317, + "Percent": 55.000000000000007, + "Count": 0 + }, + { + "Start": 47317, + "End": 47321, + "Percent": 60, + "Count": 4 + }, + { + "Start": 47321, + "End": 47323, + "Percent": 65, + "Count": 2 + }, + { + "Start": 47323, + "End": 47327, + "Percent": 70, + "Count": 5 + }, + { + "Start": 47327, + "End": 47327, + "Percent": 75, + "Count": 0 + }, + { + "Start": 47327, + "End": 47329, + "Percent": 77.5, + "Count": 2 + }, + { + "Start": 47329, + "End": 47331, + "Percent": 80, + "Count": 3 + }, + { + "Start": 47331, + "End": 47331, + "Percent": 82.5, + "Count": 0 + }, + { + "Start": 47331, + "End": 47333, + "Percent": 85, + "Count": 2 + }, + { + "Start": 47333, + "End": 47333, + "Percent": 87.5, + "Count": 0 + }, + { + "Start": 47333, + "End": 47335, + "Percent": 88.75, + "Count": 1 + }, + { + "Start": 47335, + "End": 47339, + "Percent": 90, + "Count": 1 + }, + { + "Start": 47339, + "End": 47341, + "Percent": 91.25, + "Count": 1 + }, + { + "Start": 47341, + "End": 47341, + "Percent": 92.5, + "Count": 0 + }, + { + "Start": 47341, + "End": 47347, + "Percent": 93.75, + "Count": 1 + }, + { + "Start": 47347, + "End": 47347, + "Percent": 94.375, + "Count": 0 + }, + { + "Start": 47347, + "End": 47353, + "Percent": 95, + "Count": 2 + }, + { + "Start": 47353, + "End": 47353, + "Percent": 95.625, + "Count": 0 + }, + { + "Start": 47353, + "End": 47353, + "Percent": 96.25, + "Count": 0 + }, + { + "Start": 47353, + "End": 47353, + "Percent": 96.875, + "Count": 0 + }, + { + "Start": 47353, + "End": 47353, + "Percent": 97.1875, + "Count": 0 + }, + { + "Start": 47353, + "End": 47353, + "Percent": 97.5, + "Count": 0 + }, + { + "Start": 47353, + "End": 47353, + "Percent": 97.8125, + "Count": 0 + }, + { + "Start": 47353, + "End": 47353, + "Percent": 98.125, + "Count": 0 + }, + { + "Start": 47353, + "End": 47357, + "Percent": 98.4375, + "Count": 1 + }, + { + "Start": 47357, + "End": 47357, + "Percent": 100, + "Count": 0 + } + ], + "Min": 47257, + "Max": 47357, + "Sum": 2649612, + "Avg": 47314.5, + "StdDev": 20.799210149838451, + "Percentiles": [ + { + "Percentile": 50, + "Value": 47317 + }, + { + "Percentile": 75, + "Value": 47327 + }, + { + "Percentile": 80, + "Value": 47331 + }, + { + "Percentile": 90, + "Value": 47339 + }, + { + "Percentile": 95, + "Value": 47353 + } + ] + }, + "BytesSent": 3528, + "BytesReceived": 2702142, + "StartTime": "2020-01-11T12:47:57.259885200Z", + "RequestedDuration": "2s" +} From f7ac360817b4fcd9b6b46abe956de96efd171109 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Mon, 14 Sep 2020 15:47:36 +0200 Subject: [PATCH 2/5] fix complaint about std::regex usage in a comment Signed-off-by: Otto van der Schaaf --- source/client/output_formatter_impl.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/source/client/output_formatter_impl.cc b/source/client/output_formatter_impl.cc index b51867acc..2e6e04d2e 100644 --- a/source/client/output_formatter_impl.cc +++ b/source/client/output_formatter_impl.cc @@ -397,10 +397,9 @@ const nighthawk::client::DurationHistogram FortioOutputFormatterImpl::renderFort std::string FortioPedanticOutputFormatterImpl::formatProto(const nighthawk::client::Output& output) const { std::string res = FortioOutputFormatterImpl::formatProto(output); + // 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. - - // clang-format off // 1. We misdefined RequestRPS as an int, whereas Fortio outputs that as a string. res = std::regex_replace(res, std::regex(R"EOF("RequestedQPS"\: ([0-9]*))EOF"), "\"RequestedQPS\": \"$1\""); From e0a2cbbf41eb107be8282fefaf96dc8498c4c6d3 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Mon, 14 Sep 2020 16:15:05 +0200 Subject: [PATCH 3/5] clang-tidy: modernize string literals. Signed-off-by: Otto van der Schaaf --- source/client/output_formatter_impl.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/client/output_formatter_impl.cc b/source/client/output_formatter_impl.cc index 2e6e04d2e..89877359c 100644 --- a/source/client/output_formatter_impl.cc +++ b/source/client/output_formatter_impl.cc @@ -402,13 +402,13 @@ FortioPedanticOutputFormatterImpl::formatProto(const nighthawk::client::Output& // This should be OK as the regular expression we use can be trusted. // 1. We misdefined RequestRPS as an int, whereas Fortio outputs that as a string. res = std::regex_replace(res, std::regex(R"EOF("RequestedQPS"\: ([0-9]*))EOF"), - "\"RequestedQPS\": \"$1\""); + R"EOF("RequestedQPS": "$1")EOF"); // 2. Our uint64's get serialized as json strings. Fortio outputs them as json integers. // NOTE: [0-9][0-9][0-9] looks for string fields referring to http status codes, which get // counted. res = std::regex_replace( res, std::regex(R"EOF("([0-9][0-9][0-9]|Count|BytesSent|BytesReceived)"\: "([0-9]*)")EOF"), - "\"$1\": $2"); + R"EOF("$1": $2)EOF"); // clang-format on return res; } From 5ab489008c92b4d26574daa1cac86aa2d83d7395 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Wed, 16 Sep 2020 22:05:53 +0200 Subject: [PATCH 4/5] Review feedback: fix typo Signed-off-by: Otto van der Schaaf --- source/client/output_formatter_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/client/output_formatter_impl.cc b/source/client/output_formatter_impl.cc index 89877359c..a7e3dac62 100644 --- a/source/client/output_formatter_impl.cc +++ b/source/client/output_formatter_impl.cc @@ -400,7 +400,7 @@ FortioPedanticOutputFormatterImpl::formatProto(const nighthawk::client::Output& // 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. - // 1. We misdefined RequestRPS as an int, whereas Fortio outputs that as a string. + // 1. We misdefined RequestedRPS as an int, whereas Fortio outputs that as a string. res = std::regex_replace(res, std::regex(R"EOF("RequestedQPS"\: ([0-9]*))EOF"), R"EOF("RequestedQPS": "$1")EOF"); // 2. Our uint64's get serialized as json strings. Fortio outputs them as json integers. From 2df8c2be2f56ca4baebb05684fef4eaf57e942e1 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Thu, 17 Sep 2020 10:02:41 +0200 Subject: [PATCH 5/5] Review feedback Signed-off-by: Otto van der Schaaf --- source/client/output_formatter_impl.cc | 7 ++++--- source/client/output_formatter_impl.h | 16 ++++++++++++++++ test/output_formatter_test.cc | 9 ++++++--- 3 files changed, 26 insertions(+), 6 deletions(-) diff --git a/source/client/output_formatter_impl.cc b/source/client/output_formatter_impl.cc index a7e3dac62..aa4eedd96 100644 --- a/source/client/output_formatter_impl.cc +++ b/source/client/output_formatter_impl.cc @@ -404,9 +404,10 @@ FortioPedanticOutputFormatterImpl::formatProto(const nighthawk::client::Output& res = std::regex_replace(res, std::regex(R"EOF("RequestedQPS"\: ([0-9]*))EOF"), R"EOF("RequestedQPS": "$1")EOF"); // 2. Our uint64's get serialized as json strings. Fortio outputs them as json integers. - // NOTE: [0-9][0-9][0-9] looks for string fields referring to http status codes, which get - // counted. - res = std::regex_replace( + // An example of a string that would match the regular expression below would be: + // "Count": "100", which then would be replaced to look like: "Count": 100. + // NOTE: [0-9][0-9][0-9] looks for string fields referring to http status codes, which get counted. + res = std::regex_replace( res, std::regex(R"EOF("([0-9][0-9][0-9]|Count|BytesSent|BytesReceived)"\: "([0-9]*)")EOF"), R"EOF("$1": $2)EOF"); // clang-format on diff --git a/source/client/output_formatter_impl.h b/source/client/output_formatter_impl.h index 5ac74a307..37fa96d1d 100644 --- a/source/client/output_formatter_impl.h +++ b/source/client/output_formatter_impl.h @@ -108,8 +108,24 @@ class FortioOutputFormatterImpl : public OutputFormatterImpl { double durationToSeconds(const Envoy::ProtobufWkt::Duration& duration) const; }; +/** + * Applies corrections to the output of the original FortioOutputFormatterImpl class, + * to make the output adhere better to Fortio's actual output. + * In particular, the proto json serializer outputs 64 bits integers as strings, whereas + * Fortio outputs them unquoted / as integers, trusting that consumers side can take that + * well. We also fix the RequestedQPS field which was defined as an integer, but gets + * represented as a string in Fortio's json output. + */ class FortioPedanticOutputFormatterImpl : public FortioOutputFormatterImpl { public: + /** + * Format Nighthawk's native output proto to Fortio's output format. + * This relies on the base class to provide the initial render, and applies + * post processing to make corrections afterwards. + * + * @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; }; diff --git a/test/output_formatter_test.cc b/test/output_formatter_test.cc index 07518f557..8e23185b7 100644 --- a/test/output_formatter_test.cc +++ b/test/output_formatter_test.cc @@ -187,7 +187,8 @@ class MediumOutputCollectorTest : public OutputCollectorTest { }; TEST_F(MediumOutputCollectorTest, FortioFormatter) { - const auto input_proto = loadProtoFromFile("test/test_data/output_formatter.medium.proto.gold"); + const nighthawk::client::Output input_proto = + loadProtoFromFile("test/test_data/output_formatter.medium.proto.gold"); FortioOutputFormatterImpl formatter; expectEqualToGoldFile(formatter.formatProto(input_proto), "test/test_data/output_formatter.medium.fortio.gold"); @@ -203,7 +204,8 @@ TEST_F(MediumOutputCollectorTest, FortioFormatter0sJitterUniformGetsReflected) { } TEST_F(MediumOutputCollectorTest, ConsoleOutputFormatter) { - const auto input_proto = loadProtoFromFile("test/test_data/percentile-column-overflow.json"); + const nighthawk::client::Output input_proto = + loadProtoFromFile("test/test_data/percentile-column-overflow.json"); ConsoleOutputFormatterImpl formatter; expectEqualToGoldFile(formatter.formatProto(input_proto), "test/test_data/percentile-column-overflow.txt.gold"); @@ -226,7 +228,8 @@ TEST_F(StatidToNameTest, TestTranslations) { } TEST_F(MediumOutputCollectorTest, FortioPedanticFormatter) { - const auto input_proto = loadProtoFromFile("test/test_data/output_formatter.medium.proto.gold"); + const nighthawk::client::Output input_proto = + loadProtoFromFile("test/test_data/output_formatter.medium.proto.gold"); FortioPedanticOutputFormatterImpl formatter; expectEqualToGoldFile(formatter.formatProto(input_proto), "test/test_data/output_formatter.medium.fortio-noquirks.gold");