Simple warmup: default to opting out#295
Conversation
The current single request we issue for the simple warmup pollutes some of the counters we emit in our output. Looking forward, when landing generic phases, we might want to default to opting out. This new flag could then serve as an opt-in afterwards, for those who want to preserve old behavior. Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
api/client/options.proto
Outdated
| // TransportSocket configuration to use in every request | ||
| envoy.config.core.v3.TransportSocket transport_socket = 27; | ||
| // Do not perform the simple warmup call. | ||
| google.protobuf.BoolValue no_simple_warmup = 32; |
There was a problem hiding this comment.
We have learned internally that naming boolean flags in the negative can end up being confusing. E.g:
--no-simple-warmup=False
Can we name the flag simple-warmup instead and set its default accordingly?
api/client/options.proto
Outdated
| // TransportSocket configuration to use in every request | ||
| envoy.config.core.v3.TransportSocket transport_socket = 27; | ||
| // Do not perform the simple warmup call. | ||
| google.protobuf.BoolValue no_simple_warmup = 32; |
There was a problem hiding this comment.
On a similar topic - why wouldn't we make it an opt-in right away? What are the use cases to have this enabled by default?
There was a problem hiding this comment.
I think that is a good idea, fixed in 5940d56 (with a callout in the changelist)
source/client/client_worker_impl.h
Outdated
| Envoy::Stats::Store& store, const int worker_number, | ||
| const Envoy::MonotonicTime starting_time, | ||
| Envoy::Tracing::HttpTracerPtr& http_tracer); | ||
| Envoy::Tracing::HttpTracerPtr& http_tracer, const bool simple_warmup); |
There was a problem hiding this comment.
How will this API evolve? This is about my usual concern around readability of boolean flags. Is this related to phases and will this be removed in favor of some phase information? Or should we introduce an enum now?
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
mum4k
left a comment
There was a problem hiding this comment.
Thank you for the updates, just one documentation nit.
README.md
Outdated
| Where: | ||
|
|
||
| --simple-warmup | ||
| Do not perform the simple warmup call. |
There was a problem hiding this comment.
We should also update the comment next to the flag. Should now say something like:
Perform the simple warmup call.
While we are here, do we also want to expand this a bit to explain what simple warmup call is?
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>
The current single request we issue for the simple warmup pollutes
some of the counters we emit in our output.
Looking forward, when landing generic phases, we might want to
default to opting out.
This new flag could then serve as an opt-in afterwards, for those
who want to preserve old behavior.
Signed-off-by: Otto van der Schaaf oschaaf@we-amp.com