Add tracking of response body-size and header-size by oschaaf · Pull Request #265 · envoyproxy/nighthawk

oschaaf · 2020-01-09T16:16:22Z

PR summary:

Adds tracking of response body-size and header- size using the more lightweight StreamingStatistic.
Wires these through to all output formats. (Including the Fortio output, which is driving this effort).
Adds tracking if min/max in our statistics, and wires that through to our output.
Some small refactoring / enhancements in our statistics implementations
Fix edge case in StreamingStatistic::combine
Some new test and test enhancments, including a refresher of gold files.

The new statistics will show in the CLI as:

Response body size in bytes (20 samples)
min: 847 | mean: 847 | max: 847 | pstdev: 0

Response header size in bytes (20 samples)
min: 47282 | mean: 47308.3 | max: 47372 | pstdev: 20.6186

Assuming we don't need full blown histograms for this, this fixes #271
Note: I observed that the fortio seems to be tracking different things for size/headersize in fortio, depending on if the fast http client is used or not. As this seems like a bug, I didn't try to reproduce that specific behavior in NH.

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

Waiting on:

Prerequisites to merging this

Fortio output: add missing fields & correct some of the output fields #226 to go in
Address @mum4k his comments from Fortio output: add missing fields & correct some of the output fields #226 (comment) here
Discuss fields not used in Fortio visualization and istio/tools, but that actually are emitted in Fortio's json output:
- SocketCount (only actually set for one of the two http clients of I remember correctly)
- AbortOn
- Exactly (-> target fixed amount of queries/replies, possibly could translate to NH termination
  predicates, but not shown in visualization, not used in istio/tools)
Fortio's RetCodes
Discuss that Fortios Sizes and HeaderSizes in its json output are used in istio/tools and missing in NH's output. However, they are only used to compute an error ratio (slightly different from how Fortio does that itself, and only precisely status 200 seems to count as succes), as well as compute an average response payload size for output in the csv transform.
In this PR we add simple tracking for the sizes (min/mean/max/stdev), and not full blown histogram tracking of sizes. It is worth nothing that Fortio seems to report different size values depending:

[1]

With -stdclient, Sizes reports response body size and 0-valued Header Sizes.

  "Sizes": {
    ...
    "Avg": 14,
  },
  "HeaderSizes": {
    ...
    "Avg": 0,
  },

Without -stdclient (which I think implies using the diy fastclient), Sizes reports body+header size for Sizes and body size for HeaderSizes. It's worth noting that it seems like Fortio may internally switch to stdclient when using https.

  "Sizes": {
    ...
    "Avg": 185,
  },
  "HeaderSizes": {
    ...
    "Avg": 171,
  }

- Define NH version in the code in a canonical place. - Update the fortio output formatter - add missing version field - Add RELEASE_PROCEDURE.md - Consume the new version information in various places to make --version output of executables sensible. Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

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

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

mum4k · 2020-01-15T00:13:47Z

Please mark this again as waiting-for-review once the merge conflicts are resolved.

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

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

mum4k · 2020-01-15T15:05:56Z

@eric846 please review and assign back to me when ready. Looks to be a lot of lines, but most of them are golden files.

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

eric846

LGTM

I would consider all my comments optional.

eric846 · 2020-01-20T05:53:19Z

source/client/output_collector_impl.cc

  result->set_name(name.data(), name.size());
  for (auto& statistic : statistics) {
-    *(result->add_statistics()) = statistic->toProto();
+    // TODO(#XXX): Looking at if the statistic id ends with "_size" to determine how it should be


Insert issue number here

eric846 · 2020-01-20T06:07:21Z

source/common/statistic_impl.cc

  const auto& b = dynamic_cast<const SimpleStatistic&>(statistic);
  auto combined = std::make_unique<SimpleStatistic>();
-
+  combined->min_ = a.min() > b.min() ? b.min() : a.min();


Any reason not to use std::min() and std::max()?

eric846 · 2020-01-21T00:25:56Z

source/client/factories_impl.cc

+  // StreamingStatistic for the stats that track response sizes. Ideally we would have options
+  // for this to route the right stat to the right backend (HdrStatistic, SimpleStatistic,
+  // NullStatistic).
+  // TODO(#XXX): Create options and have the StatisticFactory consider those when instantiating


Insert issue number here

eric846 · 2020-01-21T00:50:40Z

source/common/statistic_impl.cc

+  if (domain == Statistic::SerializationDomain::DURATION) {
+    int64_t nanos;
+    nanos = count() == 0 ? 0 : static_cast<int64_t>(std::round(mean()));
+    statistic.mutable_mean()->set_seconds(nanos / 1000000000);


Can we change 1000000000 to (1000 * 1000 * 1000) to avoid accidents?

Also since this pattern with /1000000000 and %1000000000, maybe there should be a helper method for setting a proto duration, taking mutable Duration* and int64_t nanos

eric846 · 2020-01-21T01:09:33Z

source/common/statistic_impl.cc

+uint64_t HdrStatistic::min() const {
+  return count() == 0 ? UINT64_MAX : hdr_value_at_percentile(histogram_, 0);
+}
+uint64_t HdrStatistic::max() const { return hdr_value_at_percentile(histogram_, 100); }


Will max() blow up if count()==0?

We should be ok with respect to that (see TypedStatisticTest::Empty)

eric846 · 2020-01-21T01:20:03Z

source/common/statistic_impl.h

    return streaming_stats_->resistsCatastrophicCancellation();
  }
  uint64_t significantDigits() const override { return streaming_stats_->significantDigits(); }
+  StatisticPtr createNewInstance() const override { return std::make_unique<InMemoryStatistic>(); };


+1 on the clever elimination of the factory.

When I saw this being called, I assumed createNewInstance() would create a copy of the existing object -- the most obvious explanation for why it would be an instance method on an existing object. What about createNewInstanceOfSameType() to clarify the meaning?

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

oschaaf · 2020-01-21T10:08:00Z

@eric846 thanks for the review, comments addressed in 9aa9e98

oschaaf added 30 commits December 11, 2019 13:20

Fix ambiguous include path

7c267eb

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

Swap ot fmt and use absl::StrCat instead

88f2e03

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

Review feedback

6f23597

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

Merge remote-tracking branch 'upstream/master' into fortio-version-field

a8e8f50

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

Merge remote-tracking branch 'upstream/master' into fortio-version-field

aa28310

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

Review feedback + iterate on release procedure

dbab3b4

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

Merge remote-tracking branch 'upstream/master' into fortio-version-field

5cc8a79

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

Consolidate version in the output

315be10

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

Back out accidentally comitted file

640ea06

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

Fix achieved qps / duration / connections / qps

8e93a74

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

Remove TODO(envoyproxy#186)

e9306fa

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

Add missing fields to the proto definition

4568133

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

Define & set min/max/sum/avg/stdev/percentiles

6076b70

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

Back out RELEASE_PROCEDURE.md

8a6bee0

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

Fix error ratio

3a1f428

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

Stub the jitter flag with a TODO

98b91e0

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

Add doc comments for durationToSeconds()

ba703cb

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

Fix start time precision, clean up the diff

28f2f38

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

Merge remote-tracking branch 'upstream/master' into fortio-version-field

2acaa34

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

Merge remote-tracking branch 'upstream/master' into fortio-version-field

a8c5c0f

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

Review: VersionString as a top-level function

7de573f

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

Merge remote-tracking branch 'upstream/master' into fortio-version-field

08bd9e5

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

Merge remote-tracking branch 'upstream/master' into fortio-version-field

abddf23

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

Merge remote-tracking branch 'upstream/master' into fortio-version-field

716e3f2

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

Merge remote-tracking branch 'upstream/master' into fortio-version-field

576b15b

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

Merge remote-tracking branch 'upstream/master' into fortio-version-field

f7bdbb1

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

Fix jitter output & add RunType, test version

a0f557b

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

Fix fortio percentile data

3abd912

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

Fix output formatter expectation

d3de072

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

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 Jan 14, 2020

oschaaf added 3 commits January 14, 2020 21:27

Merge remote-tracking branch 'upstream/master' into fortio-version-field

de3fb47

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

Sync up with earlier Fortio output conversion work

2ac3386

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

Merge remote-tracking branch 'origin/fortio-version-field' into respo…

ac675da

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

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. blocked A PR that is blocked by prerequisites. labels Jan 15, 2020

Merge remote-tracking branch 'upstream/master' into response-size-tra…

d1aee22

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

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 Jan 15, 2020

Add doc comments as promised

77a7eb2

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

oschaaf mentioned this pull request Jan 15, 2020

Fortio output: add missing fields & correct some of the output fields #226

Merged

9 tasks

mum4k requested a review from eric846 January 15, 2020 15:05

oschaaf added 3 commits January 15, 2020 20:38

Merge remote-tracking branch 'upstream/master' into response-size-tra…

6a79510

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

Merge remote-tracking branch 'upstream/master' into response-size-tra…

95fb0df

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

Merge remote-tracking branch 'upstream/master' into response-size-tra…

5082cfb

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

oschaaf requested a review from jplevyak January 20, 2020 22:49

eric846 previously approved these changes Jan 21, 2020

View reviewed changes

eric846 requested a review from mum4k January 21, 2020 01:24

oschaaf mentioned this pull request Jan 21, 2020

Create options for selecting a statistics backend per stat #292

Open

Review feedback

9aa9e98

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

oschaaf dismissed eric846’s stale review via 9aa9e98 January 21, 2020 10:07

mum4k self-assigned this Jan 21, 2020

mum4k approved these changes Jan 21, 2020

View reviewed changes

mum4k merged commit 8626464 into envoyproxy:master Jan 21, 2020

oschaaf mentioned this pull request Jun 24, 2020

Support fortio output format #155

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tracking of response body-size and header-size#265

Add tracking of response body-size and header-size#265
mum4k merged 81 commits intoenvoyproxy:masterfrom
oschaaf:response-size-tracking

oschaaf commented Jan 9, 2020 •

edited

Loading

Uh oh!

mum4k commented Jan 15, 2020

Uh oh!

mum4k commented Jan 15, 2020

Uh oh!

eric846 left a comment

Uh oh!

eric846 Jan 20, 2020

Uh oh!

eric846 Jan 20, 2020

Uh oh!

eric846 Jan 21, 2020

Uh oh!

eric846 Jan 21, 2020

Uh oh!

eric846 Jan 21, 2020

Uh oh!

eric846 Jan 21, 2020

Uh oh!

oschaaf Jan 21, 2020

Uh oh!

eric846 Jan 21, 2020

Uh oh!

oschaaf commented Jan 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

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

Uh oh!

Prerequisites to merging this

Uh oh!

mum4k commented Jan 15, 2020

Uh oh!

mum4k commented Jan 15, 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!

Choose a reason for hiding this comment

Uh oh!

oschaaf commented Jan 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

oschaaf commented Jan 9, 2020 •

edited

Loading