Schedule option by oschaaf · Pull Request #573 · envoyproxy/nighthawk

oschaaf · 2020-11-16T17:57:06Z

Adds a new schedule option to the proto api., which allows specifying a date/time
at which execution should start.

Should also fix #569 by reducing reliance on Envoy::SystemTime to a minimum:
Based on the value schedule option, a monotonic time will be computed
and propagated. This makes us use Envoy::MonotonicTime again in places
where sub ms resolution is a must have. (The fix will need confirmation, as I
couldn't get --config=libc++ to work for other reasons.)

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

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

dubious90 · 2020-11-23T19:57:13Z

@qqustc Please review and assign back to me once ready.

qqustc

Thanks Otto!

qqustc · 2020-11-25T03:26:38Z

include/nighthawk/client/output_collector.h

                         const std::map<std::string, uint64_t>& counters,
-                         const std::chrono::nanoseconds execution_duration) PURE;
+                         const std::chrono::nanoseconds execution_duration,
+                         const absl::optional<Envoy::SystemTime>& first_acquisition_time) PURE;


Add comment for first_acquisition_time?

qqustc · 2020-11-25T03:37:35Z

@qqustc Please review and assign back to me once ready.

LGTM, assigned back to @dubious90 for review.

api/client/options.proto

dubious90 · 2020-11-25T17:12:13Z

include/nighthawk/common/factories.h

                              TerminationPredicatePtr&& termination_predicate,
                              Envoy::Stats::Scope& scope,
-                              const Envoy::SystemTime scheduled_starting_time) const PURE;
+                              const Envoy::MonotonicTime scheduled_starting_time) const PURE;


Not actionable, just making sure I'm understanding - this is where we're moving back to MonotonicTime to prevent #569 ?

Yes, that is right.

dubious90 · 2020-11-25T17:13:10Z

include/nighthawk/client/output_collector.h

                         const std::map<std::string, uint64_t>& counters,
-                         const std::chrono::nanoseconds execution_duration) PURE;
+                         const std::chrono::nanoseconds execution_duration,
+                         const absl::optional<Envoy::SystemTime>& first_acquisition_time) PURE;


will likely be addressed when the comment is added, but I'm not sure i understand what acquisition means in this context.

source/client/process_impl.cc

dubious90 · 2020-11-25T17:33:08Z

source/client/process_impl.cc

  // track-for-future issue.
-  const auto first_worker_start = time_system_.systemTime() + kMinimalWorkerDelay;
+  const Envoy::MonotonicTime monotonic_now = time_system_.monotonicTime();
+  const std::chrono::nanoseconds offset =


I might be misunderstanding but this ternary seems off to me. kMinimalWorkerDelay has the worker offsets baked into it, correct? So, if we are using a scheduled start time, and we aren't using minimal worker delay at all, then we aren't including any worker offsets from each other.

So now, batching behavior of nighthawk acts differently depending on whether or not we are using a schedule which doesn't seem right.

This computes the start of the first worker, we will still be injecting an offset between each worker below. I refactored this for clarity, hopefully it's easier to read now.

dubious90 · 2020-11-25T17:33:29Z

source/client/process_impl.cc

  // global status quo, which we do not have right now. This has been noted in the
  // track-for-future issue.
-  const auto first_worker_start = time_system_.systemTime() + kMinimalWorkerDelay;
+  const Envoy::MonotonicTime monotonic_now = time_system_.monotonicTime();


Three comments here:

This has gotten complex enough (especially with the long comments above to explain what's going on), that I think it might warrant its own privately scoped function. CalculateWorkerStartTime_ or something?

dubious90 · 2020-11-25T17:37:38Z

source/client/process_impl.cc

-  const auto first_worker_start = time_system_.systemTime() + kMinimalWorkerDelay;
+  const Envoy::MonotonicTime monotonic_now = time_system_.monotonicTime();
+  const std::chrono::nanoseconds offset =
+      schedule.has_value() ? schedule.value() - time_system_.systemTime() : kMinimalWorkerDelay;


We are heavily explaining the workerdelay behavior here in the comment above. Can we add a one-line comment for how the schedule behavior fits in? For instance, if I am wrong about point 2 above, we should say why the scheduled workers don't need an offset delay.

dubious90 · 2020-11-25T17:41:07Z

include/nighthawk/common/rate_limiter.h

+  /**
+   * @return absl::optional<Envoy::SystemTime> Time of the first acquisition, if any.
+   */
+  virtual absl::optional<Envoy::SystemTime> firstAcquisitionTime() const PURE;


I might have just missed it in this pull request (if so, please point me to the right file), but while I'm seeing code that writes this field or passes it around, I'm not finding where it's actually used functionally. What is its intended purpose?

This gets consumed over at https://github.com/envoyproxy/nighthawk/pull/573/files#diff-e528c3973715873e2ec7fcac6a842e5d9a59d6523a77bf1219d295dd4c7285cdR539 and ends up in the output as execution_start.

test/process_test.cc

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

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

oschaaf · 2020-12-07T10:07:45Z

@dubious90 Thanks for the feedback, I think I addressed it. Extracted the worker start time & offset calculations into their own methods for clarity. Ready for another round!

dubious90

Looks good. Thanks for the readability changes! It's really easy to follow what's happening now.

oschaaf added 4 commits November 9, 2020 16:14

Add a new "schedule" option for indicating an explicit execution

bc0b643

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

Merge remote-tracking branch 'upstream/master' into schedule-date

b6af281

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

Changes to address envoyproxy#569

6657217

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

Address clang-tidy complaint / missing include

e8de757

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

oschaaf marked this pull request as ready for review November 18, 2020 12:01

oschaaf added the waiting-for-review A PR waiting for a review. label Nov 23, 2020

dubious90 requested a review from qqustc November 23, 2020 19:57

qqustc previously approved these changes Nov 25, 2020

View reviewed changes

qqustc requested a review from dubious90 November 25, 2020 03:36

qqustc assigned dubious90 Nov 25, 2020

dubious90 suggested changes Nov 25, 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 Nov 25, 2020

oschaaf added 2 commits December 4, 2020 00:20

Partially address review feedback

69410ab

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

Merge remote-tracking branch 'upstream/master' into schedule-date

9b419c0

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

oschaaf dismissed qqustc’s stale review via 9b419c0 December 3, 2020 23:21

oschaaf added 2 commits December 4, 2020 00:33

s/schedule/scheduled_start

fcc37e9

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

Review feedback: extract worker start delay into separate methods

a286055

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

oschaaf added the waiting-for-review A PR waiting for a review. label Dec 7, 2020

oschaaf removed the waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. label Dec 7, 2020

dubious90 approved these changes Dec 7, 2020

View reviewed changes

dubious90 merged commit dac3cc1 into envoyproxy:master Dec 7, 2020

oschaaf mentioned this pull request Dec 7, 2020

Unit tests for computeFirstWorkerStart() and computeInterWorkerDelay() #590

Open

Conversation

oschaaf commented Nov 16, 2020

Uh oh!

dubious90 commented Nov 23, 2020

Uh oh!

qqustc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qqustc commented Nov 25, 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!

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!

Uh oh!

oschaaf commented Dec 7, 2020

Uh oh!

dubious90 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants