-
Notifications
You must be signed in to change notification settings - Fork 5.3k
stream_info: adding more upstream timing metrics. #18976
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
66df259
40ffafb
d077b55
d859499
bc2eb23
6f08e50
ba85b5f
a01e02a
3abfc37
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -671,6 +671,7 @@ void ConnectionImpl::onWriteReady() { | |
| if (error == 0) { | ||
| ENVOY_CONN_LOG(debug, "connected", *this); | ||
| connecting_ = false; | ||
| onConnected(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only client connection will hit this branch. Although the server connection's onConnected() is no-op for now, I suggest adding the onConnected() here
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This interval tracks the time it took from the |
||
| transport_socket_->onConnected(); | ||
| // It's possible that we closed during the connect callback. | ||
| if (state() != State::Open) { | ||
|
|
@@ -845,7 +846,7 @@ ClientConnectionImpl::ClientConnectionImpl( | |
| const Network::ConnectionSocket::OptionsSharedPtr& options) | ||
| : ConnectionImpl(dispatcher, std::move(socket), std::move(transport_socket), stream_info_, | ||
| false), | ||
| stream_info_(dispatcher.timeSource(), socket_->connectionInfoProviderSharedPtr()) { | ||
| stream_info_(dispatcher_.timeSource(), socket_->connectionInfoProviderSharedPtr()) { | ||
|
|
||
| // There are no meaningful socket options or source address semantics for | ||
| // non-IP sockets, so skip. | ||
|
|
@@ -890,6 +891,7 @@ void ClientConnectionImpl::connect() { | |
| socket_->connectionInfoProvider().remoteAddress()->asString()); | ||
| const Api::SysCallIntResult result = | ||
| socket_->connect(socket_->connectionInfoProvider().remoteAddress()); | ||
| stream_info_.upstreamTiming().onUpstreamConnectStart(dispatcher_.timeSource()); | ||
| if (result.return_value_ == 0) { | ||
| // write will become ready. | ||
| ASSERT(connecting_); | ||
|
|
@@ -918,5 +920,10 @@ void ClientConnectionImpl::connect() { | |
| } | ||
| } | ||
|
|
||
| void ClientConnectionImpl::onConnected() { | ||
| stream_info_.upstreamTiming().onUpstreamConnectComplete(dispatcher_.timeSource()); | ||
| ConnectionImpl::onConnected(); | ||
| } | ||
|
|
||
| } // namespace Network | ||
| } // namespace Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,8 +24,31 @@ class StreamInfoToHeadersFilter : public Http::PassThroughFilter { | |
| headers.addCopy(Http::LowerCaseString("alpn"), | ||
| decoder_callbacks_->streamInfo().upstreamSslConnection()->alpn()); | ||
| } | ||
|
|
||
| return Http::FilterHeadersStatus::Continue; | ||
| } | ||
| 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. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add this into the comment on the |
||
| if (upstream_timing.upstream_connect_start_.has_value()) { | ||
| trailers.addCopy( | ||
| Http::LowerCaseString("upstream_connect_start"), | ||
| absl::StrCat(upstream_timing.upstream_connect_start_.value().time_since_epoch().count())); | ||
| } | ||
| if (upstream_timing.upstream_connect_complete_.has_value()) { | ||
| trailers.addCopy( | ||
| Http::LowerCaseString("upstream_connect_complete"), | ||
| absl::StrCat( | ||
| upstream_timing.upstream_connect_complete_.value().time_since_epoch().count())); | ||
| } | ||
| if (upstream_timing.upstream_handshake_complete_.has_value()) { | ||
| trailers.addCopy( | ||
| Http::LowerCaseString("upstream_handshake_complete"), | ||
| absl::StrCat( | ||
| upstream_timing.upstream_handshake_complete_.value().time_since_epoch().count())); | ||
| } | ||
| return Http::FilterTrailersStatus::Continue; | ||
| } | ||
| }; | ||
|
|
||
| constexpr char StreamInfoToHeadersFilter::name[]; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.