Finalize emission and tracking of latencies in response headers by oschaaf · Pull Request #500 · envoyproxy/nighthawk

oschaaf · 2020-08-31T11:35:47Z

Adds a client option and wires it through in TCLAP. Amends tests and code to work with that
instead of the hard coded value. Adds end to end test, and enables test-server extension build.

Follow up to #477

Fixes #360: with this the feature is ready to use.

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

TODO:

Land Echo back and track origin request-receipt timing deltas #477 and merge it in here
Wire up TCLAP
Tests for the new option parsing/handling
Enable build of the new time-tracking extension into nighthawk_test_server
End-to-end tests to prove the new client-side feature can be used to track latencies delivered by response header emitted by the test-server's new feature.
CLI & proto description / comments, regen docs.
Replace Stopwatch shared_ptr with unique_ptr

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

Will still fail the test because of mismatched expectations, but this will help merging master periodically in here to verify (almost) all is well. Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

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

dubious90 · 2020-09-01T19:51:03Z

@qqustc please review and assign to me once done. Thanks!

qqustc · 2020-09-03T21:38:19Z

Thanks Otto!
LGTM. Assign back to @dubious90 .

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

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

dubious90

This feature looks cool, Otto. Thanks for your efforts.

dubious90 · 2020-09-08T21:49:05Z

README.md


-bazel-bin/nighthawk_client  [--stats-flush-interval <uint32_t>]
-[--stats-sinks <string>] ...
+bazel-bin/nighthawk_client  [--response-header-with-latency-input


No changes here requested, and I think we're in agreement here, but eventually I think we need to move away from TCLAP automating this, because there are now so many flags that it's become difficult to find what you need (e.g. rps, duration).

Yeah, I agree.

dubious90 · 2020-09-08T22:04:22Z

README.md


 Where:

+--response-header-with-latency-input <string>


I think the meaning here is a little hard to parse from the name / comment combo.

How do you feel about response_latency_header_name?

dubious90 · 2020-09-08T22:10:40Z

README.md

 Where:

+--response-header-with-latency-input <string>
+Set an optional response header name, whose values will be tracked in


For the description, can I suggest this edit to the first sentence:
Set an optional header name that will be returned in responses, whose values will be tracked in a latency histogram if set.

dubious90 · 2020-09-08T22:14:18Z

README.md


+--response-header-with-latency-input <string>
+Set an optional response header name, whose values will be tracked in
+a latency histogram if set. Can be used in tandem with the test server


For the second sentence, can we edit to something like:
"... if set. e.g. Can be used in tandem with the test server's response option "emit_previous_request_delta_in_response_header" to record elapsed time between request arrivals."

(Adding in the word response option clarifies where the reader should look for the definition of that)

dubious90 · 2020-09-08T22:19:28Z

source/client/stream_decoder.h

                Envoy::Random::RandomGenerator& random_generator,
-                Envoy::Tracing::HttpTracerSharedPtr& http_tracer)
+                Envoy::Tracing::HttpTracerSharedPtr& http_tracer,
+                std::string response_header_with_latency_input)


I'm a little surprised by your use of a std::string copy with std::move here, as opposed to having this be an absl::string_view (or const std::string&, whicever is more common within our code).

And then having doing response_header_with_latency_input_(response_header_with_latency_input), which I'm pretty sure just works.

This suggestion is more in line with the style I'm familiar with. Is there a strong reason not to do that?

Good catch. I'm surprised too, I must have grown tired of the tried and proven path in this repo :-) Using absl::string_view now.

dubious90 · 2020-09-08T22:22:50Z

test/benchmark_http_client_test.cc

    client_ = std::make_unique<Client::BenchmarkClientHttpImpl>(
        *api_, *dispatcher_, store_, statistic_, false, cluster_manager_, http_tracer_, "benchmark",
-        request_generator, true);
+        request_generator, true, "");


This is a lot of arguments here. Would it be okay if we added comments helping users track the corresponding parameter under use? For the new one, it'd be
/*response_header_with_latency_input=*/""
And then, the two boolean parameters could also benefit from this.

dubious90 · 2020-09-08T22:29:51Z

test/integration/configurations/nighthawk_track_timings.yaml

@@ -0,0 +1,34 @@
+admin:


Can we add a comment at the top of this, explaining its purpose in just a sentence, and draw attention to the relevant/unique part of the configuration (which I think is the time-tracking filter)

dubious90 · 2020-09-08T22:39:54Z

test/integration/test_integration_basics.py

+  """Test emission and tracking of response header latencies.
+
+  Run the CLI configured to track latencies delivered by response header from the test-server
+  which is set up emit those. Ensure the expected histogram is observed.


nit: set up to emit those

dubious90 · 2020-09-08T22:40:48Z

test/integration/test_integration_basics.py

+  """Test emission and tracking of response header latencies.
+
+  Run the CLI configured to track latencies delivered by response header from the test-server
+  which is set up emit those. Ensure the expected histogram is observed.


nit: Can we instead say ensure the histogram receives the correct number of inputs or something?

dubious90 · 2020-09-08T22:45:18Z

test/integration/test_integration_basics.py

+  ])
+  global_histograms = http_test_server_fixture.getNighthawkGlobalHistogramsbyIdFromJson(parsed_json)
+  asserts.assertEqual(
+      int(global_histograms["benchmark_http_client.origin_latency_statistic"]["count"]), 99)


Can we add a second integration test that works as a crash test of sorts, for adding in the header but receiving no responses that include that header?

Not sure of the exact correct assertions. We would expect it to work, but for origin_latency_statistic to equal 0, I think. Working could be defined as the responses_xxx counter increasing?

Done; we now test using two server side configurations, one with the new time-tracking extension enabled, and one that doesn't have it set up. We check expectations based on which one we're testing. Let me know if that looks good to you. (68ed58a)

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

oschaaf · 2020-09-09T09:22:03Z

@dubious90 comments addressed in 68ed58a.

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

oschaaf · 2020-09-09T09:38:52Z

@dubious90 comments addressed in 68ed58a.

Also, merged master in here and resolved conflicts. Thanks for the review!

dubious90

LGTM modulo the one naming comment. Thanks!

dubious90 · 2020-09-09T15:07:59Z

README.md

 USAGE:

-bazel-bin/nighthawk_client  [--stats-flush-interval <uint32_t>]
+bazel-bin/nighthawk_client  [--response-latency-header-name <string>]


Sorry for extra churn here. Realized I gave you a misleading name (implies this would be response latency, which, while possible to do with this, is not the only usecase).

How do you feel about --latency-response-header-name (just inverting those two words).

Sounds good, done in 594bbbe

dubious90 · 2020-09-09T15:10:52Z

test/integration/test_integration_basics.py

+  ])
+  global_histograms = http_test_server_fixture.getNighthawkGlobalHistogramsbyIdFromJson(parsed_json)
+  asserts.assertEqual(int(global_histograms["benchmark_http_client.latency_2xx"]["count"]), 100)
+  # Verify behavior is correct both with and without the timing filter enabled.


This is a very helpful comment here. Thanks

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

oschaaf added 28 commits June 15, 2020 17:31

Save state on origin timeing

c226497

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

Merge remote-tracking branch 'upstream/master' into origin-timings

c6697de

Fix clang-tidy, format, and test build

28e0d34

Will still fail the test because of mismatched expectations, but this will help merging master periodically in here to verify (almost) all is well. Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Merge remote-tracking branch 'upstream/master' into origin-timings

94ad984

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

stats naming

933c589

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

Merge remote-tracking branch 'upstream/master' into origin-timings

a1691ec

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

Track request receipt deltas. Configure response header name.

740d0ca

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

Save state before context switching

1afe2f1

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

Merge remote-tracking branch 'upstream/master' into origin-timings

ae0c08e

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

Merge remote-tracking branch 'upstream/master' into origin-timings

3487e19

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

Fix format

1d1f03a

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

Suppress clang-tidy on MUTABLE_CONSTRUCT_ON_FIRST_USE

a9d99eb

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

Proper locking, add TODOs and doc comments.

0aa8630

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

Merge remote-tracking branch 'upstream/master' into origin-timings

db01e40

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

Small fixes

5e48513

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

Add unit test for StreamDecoder

d9569cb

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

Review feedback

cf1170c

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

check_format introduced proto comment issues. Add punctuation.

421feb8

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

Merge remote-tracking branch 'upstream/master' into origin-timings

af148ea

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

Review feedback

e813e75

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

Save state on splitting out a separate extension and stopwatch

505495c

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

Merge remote-tracking branch 'upstream/master' into origin-timings

5405dc9

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

Small corrections

d858b55

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

Add threaded spam test for stopwatch. Add doc comments.

3cf32b8

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

clang-tidy: fix for loop

35aee47

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

Merge remote-tracking branch 'upstream/master' into origin-timings

027c00f

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

Review feedback: doc comments

70502e1

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

Wire up proto option

a55bbe7

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

oschaaf mentioned this pull request Aug 31, 2020

Echo back and track origin request-receipt timing deltas #477

Merged

Wire up TCLAP, add OptionImpl tests, regen CLI docs.

d199313

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

Replace stopwatch shared_ptr to unique_ptr

af60b8d

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

oschaaf marked this pull request as ready for review September 1, 2020 18:00

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

Fix a few small nits & improve option description.

3996c65

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

dubious90 requested a review from qqustc September 1, 2020 19:50

qqustc previously approved these changes Sep 3, 2020

View reviewed changes

qqustc assigned dubious90 Sep 3, 2020

oschaaf added 2 commits September 5, 2020 13:03

Merge remote-tracking branch 'upstream/master' into origin-timings-tr…

228934a

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

Fix todo

00b6861

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

oschaaf dismissed qqustc’s stale review via 00b6861 September 5, 2020 11:05

oschaaf mentioned this pull request Sep 5, 2020

Test-server extension: use shared configuration base and test facilities #512

Merged

9 tasks

dubious90 suggested changes Sep 8, 2020

View reviewed changes

dubious90 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 8, 2020

Review feedback

68ed58a

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 Sep 9, 2020

Merge remote-tracking branch 'upstream/master' into origin-timings-tr…

74f9717

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

dubious90 reviewed Sep 9, 2020

View reviewed changes

dubious90 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 9, 2020

Review feedback: option rename

594bbbe

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 Sep 9, 2020

dubious90 approved these changes Sep 9, 2020

View reviewed changes

dubious90 merged commit 365b97a into envoyproxy:master Sep 9, 2020

Conversation

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

Uh oh!

Uh oh!

dubious90 commented Sep 1, 2020

Uh oh!

qqustc commented Sep 3, 2020

Uh oh!

dubious90 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!

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 Sep 9, 2020

Uh oh!

oschaaf commented Sep 9, 2020

Uh oh!

dubious90 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

oschaaf commented Aug 31, 2020 •

edited

Loading