Add SocketsHttpHandler requests-queue-duration metrics #88981
Conversation
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsContributes to #88384 Following #88893, this PR adds the
|
| tags.Add("protocol", versionMajor switch | ||
| { | ||
| 1 => "HTTP/1.1", | ||
| 2 => "HTTP/2", | ||
| _ => "HTTP/3" | ||
| }); |
There was a problem hiding this comment.
I assume it makes more sense to conform to other connection metrics and existing event counters, but note that request metrics log HTTP/1.0 if HttpRequestMessage.Version is 1.0. Is it worth a comment?
There was a problem hiding this comment.
I think it makes sense for requests to log 1.0, but for connections, we only really have HTTP/1.1 that we share between 1.0 and 1.1 requests.
Added a small comment.
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs
Outdated
Show resolved
Hide resolved
|
|
||
| ValueTask<Http3Connection> connectionTask = GetHttp3ConnectionAsync(request, authority, cancellationToken); | ||
|
|
||
| if (HttpTelemetry.Log.IsEnabled() && connectionTask.IsCompleted) | ||
| if (requestsQueueDurationEnabled && connectionTask.IsCompleted) | ||
| { | ||
| // We avoid logging RequestLeftQueue if a stream was available immediately (synchronously) |
There was a problem hiding this comment.
I'm confused by this comment. It states that We avoid logging RequestLeftQueue, but then Http3Connection.SendAsync requests a new timestamp before OpenOutboundStreamAsync and actually logs it.
(And is it actually good that OpenOutboundStreamAsync is part of the "request queue duration" measurement?)
There was a problem hiding this comment.
It used to be that we were able to synchronously open an output stream, and if that happened we would skip logging this event.
This changed in #67859, such that OpenOutboundStreamAsync will always yield, so we would always log the event.
With that, I agree this comment here no longer makes sense. I've removed it and the few lines of logic around it.
.../System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Metrics/SocketsHttpHandlerMetrics.cs
Outdated
Show resolved
Hide resolved
|
Test failure is #87477 |
ghost
locked as resolved and limited conversation to collaborators
Contributes to #88384
Following #88893, this PR adds the
http-client-requests-queue-durationcounter (final name TBD).Includes the same tags as other connection counters:
protocol,scheme,host, andport.