Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,8 @@ format>] [--sequencer-idle-strategy <spin
|CONNECT|OPTIONS|TRACE>] [--address-family
<auto|v4|v6>] [--burst-size <uint32_t>]
[--prefetch-connections] [--output-format
<json|human|yaml|dotted|fortio>] [-v <trace
<json|human|yaml|dotted|fortio
|experimental_fortio_pedantic>] [-v <trace
|debug|info|warn|error|critical>]
[--concurrency <string>] [--h2] [--timeout
<uint32_t>] [--duration <uint32_t>]
Expand Down Expand Up @@ -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 <json|human|yaml|dotted|fortio>
--output-format <json|human|yaml|dotted|fortio
|experimental_fortio_pedantic>
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 <trace|debug|info|warn|error|critical>, --verbosity <trace|debug
|info|warn|error|critical>
Expand Down Expand Up @@ -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 <json|human|yaml
|dotted|fortio> [--] [--version]
[-h]
|dotted|fortio
|experimental_fortio_pedantic> [--]
[--version] [-h]


Where:

--output-format <json|human|yaml|dotted|fortio>
--output-format <json|human|yaml|dotted|fortio
|experimental_fortio_pedantic>
(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.
Expand Down
1 change: 1 addition & 0 deletions api/client/options.proto
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ message OutputFormat {
YAML = 3;
DOTTED = 4;
FORTIO = 5;
EXPERIMENTAL_FORTIO_PEDANTIC = 6;
}
OutputFormatOptions value = 1;
}
Expand Down
21 changes: 21 additions & 0 deletions source/client/output_formatter_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#include <google/protobuf/util/time_util.h>

#include <chrono>
#include <regex>
#include <sstream>

#include "nighthawk/common/exception.h"
Expand Down Expand Up @@ -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);
// 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 RequestedRPS as an int, whereas Fortio outputs that as a string.
res = std::regex_replace(res, std::regex(R"EOF("RequestedQPS"\: ([0-9]*))EOF"),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(optional) Would you mind including (in the comment) and example of the matched string? That usually helps when trying to understand regexes in code. This one is fairly trivial, so optional, but we could use one for the second regex.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I always felt that regular expressions are kind of a write-only concept :-)
Done in 2df8c2b

R"EOF("RequestedQPS": "$1")EOF");
// 2. Our uint64's get serialized as json strings. Fortio outputs them as json integers.
// 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
return res;
}

} // namespace Client
} // namespace Nighthawk
21 changes: 21 additions & 0 deletions source/client/output_formatter_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -108,5 +108,26 @@ 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 {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a comment on the class roughly explaining what it is for?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 2df8c2b

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;
};

} // namespace Client
} // namespace Nighthawk
1 change: 1 addition & 0 deletions test/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
17 changes: 14 additions & 3 deletions test/output_formatter_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -186,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");
Expand All @@ -202,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");
Expand All @@ -224,5 +227,13 @@ TEST_F(StatidToNameTest, TestTranslations) {
}
}

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),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This golden file is fairly long which makes it hard for the reader to figure out which portions actually are relevant to this test and which are just noise. Any way we could cut it down to the minimum required to test this new behavior?

Copy link
Copy Markdown
Member Author

@oschaaf oschaaf Sep 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense, but is kind of pre-existing; I think the initial work for this just grabbed a raw arbitrary snapshot and included it for testing the origin fortio output formatter code.
We are now more or less forced to propagate that, as our new gold file is an amended version of the original one.
I punted this to #542 to track doing this, does that work for you?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That works, thank you.

"test/test_data/output_formatter.medium.fortio-noquirks.gold");
}

} // namespace Client
} // namespace Nighthawk
Loading