Switch to enableHRTimer -- improve request-release timing accuracy by oschaaf · Pull Request #217 · envoyproxy/nighthawk

oschaaf · 2019-11-27T10:35:18Z

Improves accuracy of request-release timings when the sequencer cannot use spinning.

Requirement on the Envoy side needs to go in first:

Add Timer::enableHRTimer() envoy#9155
Add TimerImpl::enableHRTimer - take two envoy#9229
update the Envoy dep to a revision that has ^^: update the revision over at Update Envoy dep for access to enableHRTimer() #222
merge master after ^^, which hopefully makes CI happy regarding this PR

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

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

oschaaf · 2019-11-27T10:37:02Z

For best results, libevent needs to be made aware too that we'd like to use precise timers.
One way to do that is by setting an environment variable called EVENT_PRECISE_TIMER.

EVENT_PRECISE_TIMER=1 bazel-bin/nighthawk_client ...

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

Prerequisite to envoyproxy#217 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

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

oschaaf · 2019-12-13T00:23:25Z

readying this up for review, as we are closing in on landing all the prerequisites to this.
Tests should green-light once that happens.

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

mum4k

LGTM, however I don't know enough about the background and motivation so will leave merging this to @htuch

mum4k · 2019-12-20T01:49:38Z

/assign htuch

htuch · 2019-12-20T18:20:51Z

source/client/process_impl.cc

+  // The upside of the approach below is that we are very loosely coupled and have a one-liner.
+  // Getting to libevent for the other approach is going to introduce more code as we would need to
+  // derive our own customized versions of certain Envoy concepts.
+  putenv(const_cast<char*>("EVENT_PRECISE_TIMER=1"));


Is it safe to change this at runtime? It seems to be relying on the fact that libevent isn't caching this value or using it for its own setup. Should we at least push it to prior to wherever we initialize libevent?

So libevent applies this here.

So I think this is safe / in time for the workers. But I do agree earlier is less risky going forward. Let me see what I can do there.

(doing this early makes this a little more resilient to changes, and would also make this apply to the dispatcher we use above)

Yeah, this is better. I'm still a bit sketched by using env vars here, but let's see how it goes.

htuch

Generally looks good, just one question..
/wait

htuch · 2019-12-20T20:14:15Z

source/common/sequencer_impl.cc

-  const auto now = last_event_time_ = time_source_.monotonicTime();
+  const auto now = time_source_.monotonicTime();
  const auto running_duration = now - start_time_;
+  last_event_time_ = now;


What's the reason for this change?

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

oschaaf · 2019-12-20T21:07:53Z

@htuch backed out the no-op change, and moved the setupForHRTimers() call + added a comment about the timing of the call.

htuch

Thanks!

htuch · 2019-12-23T04:16:21Z

source/client/process_impl.cc

+  // The upside of the approach below is that we are very loosely coupled and have a one-liner.
+  // Getting to libevent for the other approach is going to introduce more code as we would need to
+  // derive our own customized versions of certain Envoy concepts.
+  putenv(const_cast<char*>("EVENT_PRECISE_TIMER=1"));


Yeah, this is better. I'm still a bit sketched by using env vars here, but let's see how it goes.

* save state on ramping rate limiter Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com> * Add tests, clean up Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com> * Refactor + LinearlyOpeningRateLimiterFilter Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com> * save state Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com> * save state Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com> * Back out change to constness in Frequency Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com> * Save state, wire in zipf / foo ZipfRateLimiter Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com> * Some comments & tidying up Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com> * small cleanup Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com> * Update Envoy dep for access to enableHRTimer() Prerequisite to #217 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com> * Whoops, amend bad copying Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com> * Also, sync .bazelrc while at it Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com> * Review feedback Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com> * Clean up the diff Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com> * Update Envoy to the latest Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com> * Move to the sha that has our target Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com> * Unbreak it Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com> * Fix a TODO Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com> * Fix accidental comment Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com> * Fix clang-tidy issue Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com> * Zipf: expose q and v arguments of the distribution Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com> * Zipf: expose q and v arguments of the distribution Also, add a test, and doc comments Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com> * Remove zipf stuff Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com> * Review feedback Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com> * Review: 1ns adjustment - Back out 1 ns adjustment - Update test to use microsecond step resolution now that we can - Add comment with explanation Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com> * Review: partially address comments - Check expectations on the distribution argument to GraduallyOpeningRateLimiterFilterTest - Clean up the computation in GraduallyOpeningRateLimiterFilter - Add a bunch of explanatory comments What's left is getting easy to grok numbers in test expectations. Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com> * Review-feedback: use second law of motion Instead of matching hard-coded expectations for release timings, use the second law of motion as a control method for verifying the acquisition timings the rate limiter. Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com> * clang-tidy: fix complaint Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com> * Review feedback Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Switch to enableHRTimer

c6b707d

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

oschaaf added 3 commits November 27, 2019 11:43

Fix test, tidy up.

9efd30a

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

Whoops

cf704a5

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

Fix sequencer test hang

e624faa

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

oschaaf mentioned this pull request Dec 3, 2019

Linear ramping and probabilistic ramping #218

Merged

2 tasks

oschaaf added 2 commits December 3, 2019 16:47

Merge remote-tracking branch 'upstream/master' into hr-timer

9709ee9

Update Envoy dep for access to enableHRTimer()

7883d59

Prerequisite to envoyproxy#217 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf mentioned this pull request Dec 3, 2019

Update Envoy dep for access to enableHRTimer() #222

Merged

oschaaf added 8 commits December 3, 2019 17:18

Whoops, amend bad copying

e458ea5

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

Also, sync .bazelrc while at it

e7b65c1

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

Merge remote-tracking branch 'upstream/master' into hr-timer

46dda28

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

Merge remote-tracking branch 'upstream/master' into hr-timer

dbc205c

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

Merge remote-tracking branch 'upstream/master' into update-envoy-dep-8

b5b5fd2

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

Update Envoy to the latest

fb85732

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

Move to the sha that has our target

8a268fe

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

Unbreak it

34f649b

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

oschaaf marked this pull request as ready for review December 13, 2019 00:22

oschaaf added 5 commits December 13, 2019 09:17

Merge remote-tracking branch 'origin/update-envoy-dep-8' into hr-timer

66d6629

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

Add setupForHRTimers() & use it

54dba60

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

Merge remote-tracking branch 'upstream/master' into hr-timer

c64d513

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

Merge remote-tracking branch 'upstream/master' into hr-timer

14ac087

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

Merge remote-tracking branch 'upstream/master' into hr-timer

cbd4c72

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

oschaaf added the P1 label Dec 17, 2019

Merge remote-tracking branch 'upstream/master' into hr-timer

621df41

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

mum4k reviewed Dec 20, 2019

View reviewed changes

repokitteh-read-only bot assigned htuch Dec 20, 2019

htuch reviewed Dec 20, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Dec 20, 2019

Review feedback

75aca4e

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

repokitteh-read-only bot removed the waiting label Dec 20, 2019

oschaaf added the waiting-for-review A PR waiting for a review. label Dec 20, 2019

htuch approved these changes Dec 23, 2019

View reviewed changes

htuch merged commit 8bfedb5 into envoyproxy:master Dec 23, 2019

Conversation

oschaaf commented Nov 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oschaaf commented Nov 27, 2019

Uh oh!

oschaaf commented Dec 13, 2019

Uh oh!

mum4k left a comment

Choose a reason for hiding this comment

Uh oh!

mum4k commented Dec 20, 2019

Uh oh!

htuch Dec 20, 2019

Choose a reason for hiding this comment

Uh oh!

oschaaf Dec 20, 2019

Choose a reason for hiding this comment

Uh oh!

oschaaf Dec 20, 2019

Choose a reason for hiding this comment

Uh oh!

htuch Dec 23, 2019

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

htuch Dec 20, 2019

Choose a reason for hiding this comment

Uh oh!

oschaaf commented Dec 20, 2019

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

htuch Dec 23, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

oschaaf commented Nov 27, 2019 •

edited

Loading