Add remaining System.Net.Http Telemetry #40838
Conversation
|
Tagging subscribers to this area: @dotnet/ncl |
|
Tagging subscribers to this area: @dotnet/ncl |
|
/azp run runtime-libraries outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| @@ -148,6 +157,8 @@ public Http2Connection(HttpConnectionPool pool, Connection connection) | |||
| } | |||
| } | |||
|
|
|||
| ~Http2Connection() => Dispose(); | |||
There was a problem hiding this comment.
Q: Do we really need this?
Http2Connection is internal. Do we have complete control over its life-cycle or not? If yes, we should be Disposing properly already.
There was a problem hiding this comment.
We have complete ownership of it at all times, but it's not guaranteed that Dispose will ever be called if the HttpClient/Handler is never disposed of as there is no finalizer higher up in the chain.
We could instead introduce a finalizer higher in the chain (for example on ConnectionPoolManager) to avoid having one per Http2Connection, but such a finalizer isn't needed for any other use-case / for HTTP/1 at the moment afaik.
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs
Outdated
Show resolved
Hide resolved
| _totalHttp20StreamsCounter ??= new PollingCounter("http20-streams-current-total", this, () => Interlocked.Read(ref _openedHttp20Streams)) | ||
| { | ||
| DisplayName = "Current Http 2.0 Streams" | ||
| }; |
There was a problem hiding this comment.
An HTTP/2 stream is the same as a request. Do we need this counter? And if so, what's the 1.1 equivalent?
There was a problem hiding this comment.
I see this counter isn't actually listed in David's tracking issue (#37428), removed.
I added it coming from discussions around the usefulness of "single-pool-max" counters in #38686.
I realize such counters ("http11-current", "http20-current") can be implemented in a much simpler manner than was done in this PR, as we already have guarantees around Start/Stop being called on an HttpRequestMessage exactly once and we have the version info available there.
There was a problem hiding this comment.
I see this counter isn't actually listed in David's tracking issue
Are there others you added that weren't previously discussed and should be removed, too?
|
/azp run runtime-libraries outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Regex and X509Cert test failures unrelated. |
ghost
locked as resolved and limited conversation to collaborators
Adds the following events:
Http11ConnectionEstablishedHttp11ConnectionClosedHttp20ConnectionEstablishedHttp20ConnectionClosedHttp11RequestLeftQueueResponseHeadersBeginAdds the following counters:
http11-connections-current-totalhttp20-connections-current-totalhttp11-requests-queue-durationContributes to #37428