Add experimental "pedantic" fortio output formatter. by oschaaf · Pull Request #534 · envoyproxy/nighthawk

oschaaf · 2020-09-14T13:34:44Z

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 #514. Experimental until we have
some confirmation that this can be marked as final / all issues have
been resolved.

Signed-off-by: Otto van der Schaaf oschaaf@we-amp.com

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 envoyproxy#422 and fixes envoyproxy#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 <oschaaf@we-amp.com>

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf · 2020-09-14T13:49:32Z

/cc @kushthedude @howardjohn

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

dubious90 · 2020-09-14T15:39:15Z

@eric846 Please review and assign back to me once done.

kushthedude

This works for us!
/cc @leecalcote

oschaaf · 2020-09-15T20:20:14Z

Also /cc @carolynhu
this is pulled of in a backwards compatible manner, but when we upgrade, there'll
be a way to get rid of the workarounds for the fortio output more quirks in NH once this lands.

source/client/output_formatter_impl.cc

carolynhu · 2020-09-16T19:09:48Z

test/test_data/output_formatter.medium.fortio-noquirks.gold

+  "Min": 0.053798911,
+  "Max": 0.310886399,
+  "Sum": 4.156954618,
+  "Avg": 0.078433106,
+  "StdDev": 0.05052302,


Could we move these fields (line 197 to line 201) up to under line 8 "Count" field as what fortio has?

That is quite hard to pull off in the current approach. Looking at it, the proto json serialiser we use here alphabetically sorts this. I think it can do that, because after parsing the difference usually doesn't matter? Do you see a way that the re-ordering could cause trouble?

Not yet. Just noticed the order is mismatch with fortio. Don't think it will cause any trouble.

carolynhu · 2020-09-16T19:09:54Z

test/test_data/output_formatter.medium.fortio-noquirks.gold

+   },
+   {
+    "Percentile": 95,
+    "Value": 47353


are these value fake or real? as this is in seconds...

I didn't come up with this test data, it's a copy of a pre-existing one modified to reflect the changes in here. If I interpret the file correctly, we're not looking at seconds, but rather a histogram of byte counts for the header?

leecalcote · 2020-09-16T19:19:46Z

@oschaaf thank you for today's call. Very much enjoyed it.

We can assist in verifying compatibility. Are the build artifacts of this PR (the container image) available?

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf · 2020-09-16T20:08:51Z

@oschaaf thank you for today's call. Very much enjoyed it.

We can assist in verifying compatibility. Are the build artifacts of this PR (the container image) available?

@leecalcote
Unfortunately, I think fresh images will only be pushed post-merge of this.
So to try this out one needs to manually build the docker image.
Having said that, it is ok to iterate on this while we have the feature marked as --experimental, as we don't
need to be backwards compatible while that is the case. So waiting until after the merge is viable too.

eric846

LGTM

mum4k · 2020-09-17T03:34:47Z

source/client/output_formatter_impl.cc

+  // 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"),


(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.

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

mum4k · 2020-09-17T03:37:27Z

source/client/output_formatter_impl.h

  double durationToSeconds(const Envoy::ProtobufWkt::Duration& duration) const;
 };

+class FortioPedanticOutputFormatterImpl : public FortioOutputFormatterImpl {


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

Done in 2df8c2b

mum4k · 2020-09-17T03:38:30Z

test/output_formatter_test.cc

 }

+TEST_F(MediumOutputCollectorTest, FortioPedanticFormatter) {
+  const auto input_proto = loadProtoFromFile("test/test_data/output_formatter.medium.proto.gold");


The "auto" here doesn't seem to hide redundant type information, so it might be better for the reader to spell it out.

Done here and in other spots in 2df8c2b

mum4k · 2020-09-17T03:39:33Z

test/output_formatter_test.cc

+TEST_F(MediumOutputCollectorTest, FortioPedanticFormatter) {
+  const auto input_proto = loadProtoFromFile("test/test_data/output_formatter.medium.proto.gold");
+  FortioPedanticOutputFormatterImpl formatter;
+  expectEqualToGoldFile(formatter.formatProto(input_proto),


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?

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?

That works, thank you.

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

mum4k · 2020-09-17T19:09:23Z

test/output_formatter_test.cc

+TEST_F(MediumOutputCollectorTest, FortioPedanticFormatter) {
+  const auto input_proto = loadProtoFromFile("test/test_data/output_formatter.medium.proto.gold");
+  FortioPedanticOutputFormatterImpl formatter;
+  expectEqualToGoldFile(formatter.formatProto(input_proto),


That works, thank you.

oschaaf added 2 commits September 14, 2020 15:30

fix complaint about std::regex usage in a comment

f7ac360

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf added the waiting-for-review A PR waiting for a review. label Sep 14, 2020

clang-tidy: modernize string literals.

e0a2cbb

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

dubious90 requested a review from eric846 September 14, 2020 15:39

This was linked to issues Sep 14, 2020

Fortio output slightly different than the real fortio #422

Closed

Nighthawk fortio's output is discrepant then real fortio's output #514

Closed

kushthedude previously approved these changes Sep 15, 2020

View reviewed changes

carolynhu reviewed Sep 16, 2020

View reviewed changes

oschaaf added 2 commits September 16, 2020 22:04

Merge remote-tracking branch 'upstream/master' into fortio-pdentantic

82e735c

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Review feedback: fix typo

5ab4890

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf dismissed kushthedude’s stale review via 5ab4890 September 16, 2020 20:06

eric846 previously approved these changes Sep 17, 2020

View reviewed changes

mum4k self-assigned this Sep 17, 2020

mum4k requested changes Sep 17, 2020

View reviewed changes

mum4k added waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. and removed waiting-for-review A PR waiting for a review. labels Sep 17, 2020

Merge remote-tracking branch 'upstream/master' into fortio-pdentantic

ab74535

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf mentioned this pull request Sep 17, 2020

Fortio output transform test: minimize gold files #542

Open

Review feedback

2df8c2b

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf dismissed eric846’s stale review via 2df8c2b September 17, 2020 08:02

oschaaf added waiting-for-review A PR waiting for a review. and removed waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. labels Sep 17, 2020

mum4k approved these changes Sep 17, 2020

View reviewed changes

mum4k merged commit 8b16dc1 into envoyproxy:master Sep 17, 2020

Conversation

oschaaf commented Sep 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oschaaf commented Sep 14, 2020

Uh oh!

dubious90 commented Sep 14, 2020

Uh oh!

kushthedude left a comment

Choose a reason for hiding this comment

Uh oh!

oschaaf commented Sep 15, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leecalcote commented Sep 16, 2020

Uh oh!

oschaaf commented Sep 16, 2020

Uh oh!

eric846 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oschaaf Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

oschaaf commented Sep 14, 2020 •

edited

Loading

oschaaf Sep 17, 2020 •

edited

Loading