-
Notifications
You must be signed in to change notification settings - Fork 93
Add experimental "pedantic" fortio output formatter. #534
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
694f87f
f7ac360
e0a2cbb
82e735c
5ab4890
ab74535
2df8c2b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |
| #include <google/protobuf/util/time_util.h> | ||
|
|
||
| #include <chrono> | ||
| #include <regex> | ||
| #include <sstream> | ||
|
|
||
| #include "nighthawk/common/exception.h" | ||
|
|
@@ -393,5 +394,24 @@ 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 RequestRPS as an int, whereas Fortio outputs that as a string. | ||
| res = std::regex_replace(res, std::regex(R"EOF("RequestedQPS"\: ([0-9]*))EOF"), | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 :-) |
||
| 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"), | ||
| R"EOF("$1": $2)EOF"); | ||
| // clang-format on | ||
| return res; | ||
| } | ||
|
|
||
| } // namespace Client | ||
| } // namespace Nighthawk | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -108,5 +108,10 @@ class FortioOutputFormatterImpl : public OutputFormatterImpl { | |
| double durationToSeconds(const Envoy::ProtobufWkt::Duration& duration) const; | ||
| }; | ||
|
|
||
| class FortioPedanticOutputFormatterImpl : public FortioOutputFormatterImpl { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in 2df8c2b |
||
| public: | ||
| std::string formatProto(const nighthawk::client::Output& output) const override; | ||
| }; | ||
|
|
||
| } // namespace Client | ||
| } // namespace Nighthawk | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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"); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The "auto" here doesn't seem to hide redundant type information, so it might be better for the reader to spell it out.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done here and in other spots in 2df8c2b |
||
| FortioPedanticOutputFormatterImpl formatter; | ||
| expectEqualToGoldFile(formatter.formatProto(input_proto), | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
Uh oh!
There was an error while loading. Please reload this page.