Add "no-duration" to facilitate infinite load generation#366
Add "no-duration" to facilitate infinite load generation#366mum4k merged 7 commits intoenvoyproxy:masterfrom
Conversation
With this, --duration 0 will be treated as a request to infinitely execute. Other default predicates will still exist, to execution may stop when connection failures or error response codes are observed. This leaves some small tech debt: - We shouldn't need to create a foo root termination predicate - We need test coverage, but that's much to add once envoyproxy#280 or envoyproxy#344 goes in Fixes envoyproxy#333 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
|
/cc @howardjohn |
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
howardjohn
left a comment
There was a problem hiding this comment.
Not sure about the code but the user facing changes are what I pictured. Thanks!
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
once we can have good means to test this. Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
source/client/factories_impl.cc
Outdated
| time_source, options_.duration(), scheduled_starting_time); | ||
| TerminationPredicate* current_predicate = duration_predicate.get(); | ||
| TerminationPredicatePtr root_predicate = std::make_unique<NullTerminationPredicateImpl>(); | ||
| if (options_.duration() > 0s) { |
There was a problem hiding this comment.
This may be a bit surprising to unaware users. Especially if zero is the default.
Additionally what would happen in the gRPC world if we send a request to the NH server with an infinite duration and how will the client terminate such a run and get the results? The same question likely applies to local runs - how do we terminate and get results?
Do we want to do some or all of the following?
a) print out on the screen + log saying that this is a run with an infinite duration and an instruction of how to terminate it.
b) should we use -1 instead of zero to avoid the "runs infinitely by default" situation? It would also help to make this change backwards compatible.
c) should we improve the documentation of the option indicating how does one terminate an infinite run and get results?
There was a problem hiding this comment.
This may be a bit surprising to unaware users. Especially if zero is the default.
Our default duration actually is 5 seconds, but that gets defined elsewhere in the options implementation header. That part doesn't change in this PR.
Additionally what would happen in the gRPC world if we send a request to the NH server with an infinite duration and how will the client terminate such a run and get the results? The same question likely applies to local runs - how do we terminate and get results?
For local runs, there's no way to terminate (gracefully) today. But this PR purposely didn't worry about that, as the feature was requested in a context where results are irrelevant. So this PR helps by proving a way to just generate infinite load.
The gRPC service would behave identical.
However, I did have similar thoughts before starting on this, so I also added a follow up to that teaches the CLI about handling signals and wires through a way for ProcessImpl to quickly and gracefully exit and yield results. RemoteProcessImpl still has a TODO for that, but the path I had in mind there would send a gRPC message to request cancellation of the currently active ProcessImpl over there, which would then be propagated to the active ProcessImpl running within this service -- if any (we can only have a single one active at a time today).
Do we want to do some or all of the following?
a) print out on the screen + log saying that this is a run with an infinite duration and an instruction of how to terminate it.
Right now the CLI will print that execution will target 0 seconds via an informational log line. I will make that print out "infinite" instead.
b) should we use -1 instead of zero to avoid the "runs infinitely by default" situation? It would also help to make this change backwards compatible.
Alternatively, we could also try to add an explicit --no-duration flag, and make this value an internal detail. If we back out the current change, zero would be suitable for that, as we guard against that value being set by the user. I can go either way.
c) should we improve the documentation of the option indicating how does one terminate an infinite run and get results?
That probably belongs in the follow up I mentioned above?
| TerminationPredicate* current_predicate = duration_predicate.get(); | ||
| TerminationPredicatePtr root_predicate = std::make_unique<NullTerminationPredicateImpl>(); | ||
| if (options_.duration() > 0s) { | ||
| root_predicate = std::make_unique<DurationTerminationPredicateImpl>( |
There was a problem hiding this comment.
Readability wise - it may be better to have an explicit if<->else rather then set a value and modify it after. The latter may confuse the reader if they don't notice the modification under the if statement.
Consider (pseudo code):
root_predicate = NULL
if (finite_run) {
root_predicate = TerminateAt...
} else {
root_predicate = NeverTerminate
}
| }; | ||
|
|
||
| /** | ||
| * Predicate which always returns the passed in termination status. |
There was a problem hiding this comment.
Doesn't seem to accept any parameters, does the doc comment need an update?
| @@ -41,7 +41,7 @@ COVERAGE_VALUE=${COVERAGE_VALUE%?} | |||
|
|
|||
There was a problem hiding this comment.
Can we (for now) add a TODO pointing to an issue about the missing test coverage? Not sure if this is the right file, so feel free to add it in the appropriate location.
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
In envoyproxy#366 we had to lower the coverage threshold as that PR didn't come with the means to easily cover testing with infinite excecution durations. Now that we have signal handling in place, we can easily pull this off. Fixes envoyproxy#370 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
…nvoyproxy#384) In envoyproxy#366 we had to lower the coverage threshold as that PR didn't come with the means to easily cover testing with infinite excecution durations. Now that we have signal handling in place, we can easily pull this off. Fixes envoyproxy#370 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Adds
--no-duration, which will be treated as a request to infinitelycontinue load generation. Other default predicates will still exist, so
execution may still stop when connection failures or error response codes
are observed.
This leaves some small tech debt:
nighthawk_client: handle termination signal #280 or Add functionality to flush metrics periodically and a few metrics (latency, total requests sent, ...) #344 goes in
Fixes #333
Signed-off-by: Otto van der Schaaf oschaaf@we-amp.com