Conversation
Introduces a concept `Phases` into the system. Phases represent pieces of the overall execution, scoping statistics, configuration, and termination predicates. For example, a warmup phase could be statically added based on a clone of the main phase with a rate limiter which smoothly ramps up to the target qps. Currently a no-op, meant to initiate discussion and have something concrete to start with. 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>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
- Make the warmup phase equivalent to what is on master - Back out changes in test expectations Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
|
This is ready for review -- but it's probably easier to do so when we've merged #222 and merged master afterwards in here. |
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
oschaaf
added
waiting-for-review
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
|
@dubious90 agreed to take another look after the recent changes. Thank you Nate. |
dubious90
left a comment
dubious90
left a comment
There was a problem hiding this comment.
Two main questions before I pass this back to mum4k
| *benchmark_client_, *termination_predicate_, | ||
| *worker_number_scope_)) {} | ||
| phase_(std::make_unique<PhaseImpl>( | ||
| "main", |
There was a problem hiding this comment.
is hardcoding this to main going to make it harder to allow arbitrary configuration of these later?
There was a problem hiding this comment.
No this shouldn’t matter for now, it will only show up in logs but otherwise is not visible externally.
There was a problem hiding this comment.
Just to clarify, does that mean this is temporary code that will be replaced later? Or that the use of main here is a secret phase that outside users won't need to know about? If it's the latter, I'm concerned about name collision, but if it's the first, then agree there's no concern here.
There was a problem hiding this comment.
It's temporary code. This will be replaced with a generic mechanism that translates phases into configuration. So the id chosen here can be an arbitrary value, perhaps I should have better called it foo or some such to avoid confusion.
| * @param should_measure_latencies Indicates if latencies should be tracked for requests issued | ||
| * during execution of this phase. | ||
| * @param time_source Time source that will be used to query the clock. | ||
| * @param start_time Optional starting time of the phase. Can be used to schedule phases ahead. |
There was a problem hiding this comment.
I don't know if start_time is a good way to schedule phases to run. Isn't NH currently being orchestrated so that processes can not run on top of themselves? And wouldn't time delays cause unexpected behavior with multiple consumers using the service?
Could we instead treat phases more like a list (maybe a linked list)? Then when one phase ends, we could start the next phase if one exists? Rather than this process holding mechanism?
There was a problem hiding this comment.
This is a re-implementation of something used today as a means to offset load test starts, to ensure request timings are evenly spaced from a global perspective. See https://github.com/envoyproxy/nighthawk/pull/219/files/c927a6ed9c006a0778813cf495c920408d1785bd#discussion_r363545616 for more context
There was a problem hiding this comment.
I'll leave this up to you and @mum4k for if that makes sense architecturally. My concern is that we're defining an ordering mechanism directly into phases that doesn't seem to fit for phases, and we're repurposing it in ClientWorkerImpl for replacing a currently existing functionality that does not seem to have much to do with phases.
For whatever it's worth, that discussion link doesn't work, so I don't know what context you're referring to.
There was a problem hiding this comment.
The right link to the earlier discussion ought to have been #219 (comment). Somehow the previous one went bad, sorry.
But point taken, somehow this doesn't fit well with me either, but so far I wasn't able to come up with something better. But reading back the linked discussion:
Maybe this should move into a different concept. I also had a concern with that this concept won't hold well with the new rate limiters we recently landed, and with non-probabilistic timing it doesn't make sense either. So maybe we should contain this functionality in a DelayStartingRateLimiter instead of here in Phase. How would you feel about that?
There was a problem hiding this comment.
I support your last suggestion @oschaaf, it makes the concept of phase more focused.
There was a problem hiding this comment.
Great; I added #281 as a prerequisite to this PR
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
|
I've left a couple questions, but as they likely do not require changes, assigning back to @mum4k for approval |
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
mum4k
added
waiting-for-changes
As per discussion [1], this is intended to be used as a replacement for the way we offset worker request release timings today. [1] envoyproxy#219 (comment) Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
As per discussion [1], this is intended to be used as a replacement for the way we offset worker request release timings today. [1] #219 (comment) Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
oschaaf
added
waiting-for-review
|
This now leverages |
This reverts commit 0dd90fd. Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
This reverts commit 870d7c8. Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
3 participants
Introduces a concept of
Phasesinto the system.Phases represent named pieces of the overall benchmark execution.
For example, a warm-up phase could perform a ramp-up to a certain qps, and then a main phase could enable latency measurement and sustain the requested qps.
Currently this adds a warmup phase as an example, which is functionally on par with what we had before. Logical follow ups to this would:
Groundwork for #31
Signed-off-by: Otto van der Schaaf oschaaf@we-amp.com
Prerequisites