stream_info: adding more upstream timing metrics.#18976
stream_info: adding more upstream timing metrics.#18976yanavlasov merged 9 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
cc @AndresGuedez @yanavlasov as we may want to use these in house and not just E-M |
| if (error == 0) { | ||
| ENVOY_CONN_LOG(debug, "connected", *this); | ||
| connecting_ = false; | ||
| onConnected(); |
There was a problem hiding this comment.
Only client connection will hit this branch.
Although the server connection's onConnected() is no-op for now, I suggest adding the onConnected() here
There was a problem hiding this comment.
This interval tracks the time it took from the connect call to connection being connected. This can only be applicable to upstream/client connections.
yanavlasov
left a comment
yanavlasov
left a comment
There was a problem hiding this comment.
LGTM modulo comment.
/wait
| } | ||
| Http::FilterTrailersStatus encodeTrailers(Http::ResponseTrailerMap& trailers) override { | ||
| StreamInfo::UpstreamTiming& upstream_timing = decoder_callbacks_->streamInfo().upstreamTiming(); | ||
| // Upstream metrics aren't available until the response is complete. |
There was a problem hiding this comment.
Can we add this into the comment on the StreamInfo::upstreamTiming() API, please?
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
| } | ||
|
|
||
| void onUpstreamHandshakeComplete(TimeSource& time_source) { | ||
| upstream_handshake_complete_ = time_source.monotonicTime(); |
There was a problem hiding this comment.
Naive question, I notice the other events assert the event time was not prior set. What's the reason for UpstreamHandshakeComplete to diverge?
There was a problem hiding this comment.
largely laziness - I didn't want to rewrite a bunch of unit tests to land this PR and the unit tests break the (integration test passing) invariant.
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
This is great, thanks Alyssa! |
6 participants
Risk Level: low
Testing: integration
Docs Changes: n/a
Release Notes: inline