Ensure Http Telemetry correctness #40338
Conversation
|
Tagging subscribers to this area: @dotnet/ncl |
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Instead, this PR removes an optimization for HttpConnectionPool where we avoid using a finalizer if maxConnections is int.MaxValue. With this change, we always use a finalizable HttpConnectionPool.
To my knowledge, unlimited number of HTTP 1.1 connections is the most common case, thus I'm concerned with this change will decrease performance for the most of the users.
|
It will reduce perf, but not by much. A quick benchmark of the overhead of having a suppressed finalizer yields:
We are talking about a difference in the area of ~50 ns per HttpConnection. I don't think this is a significant performance hit in the context of creating connections. For comparison, every Do you have a suggestion for an alternative approach that would avoid the finalizer overhead? |
| internal void MarkAsTrackedByTelemetry() | ||
| { | ||
| Debug.Assert(_sendStatus != MessageShouldEmitTelemetry); | ||
| _sendStatus = MessageShouldEmitTelemetry; |
There was a problem hiding this comment.
Isn't this going to overwrite the MessageAlreadySent set as part of MarkAsSent?
There was a problem hiding this comment.
Oh, I see, you consider both MessageAlreadySent and MessageShouldEmitTelemetry to mean already sent. That naming is strange to me.
There was a problem hiding this comment.
Would
private const int MessageNotYetSent = 0;
private const int MessageAlreadySent = 1;
-private const int MessageShouldEmitTelemetry = 2;
+private const int MessageAlreadySent_StopNotYetCalled = 2;be more clear?
|
|
||
| internal void OnStopped(bool aborted = false) | ||
| { | ||
| if (_sendStatus == MessageShouldEmitTelemetry && Interlocked.Exchange(ref _sendStatus, MessageAlreadySent) == MessageShouldEmitTelemetry) |
There was a problem hiding this comment.
Why is this exchange necessary?
src/libraries/System.Net.Http/src/System/Net/Http/HttpRequestMessage.cs
Outdated
Show resolved
Hide resolved
|
cc: @geoffkizer |
alnikola
dismissed
their stale review
Ok, if it's really 50 ns per connection, that's fine. Let's proceed with this PR after all other comments were resolved.
|
/azp run runtime-libraries outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-libraries outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
MihaZupan
force-pushed
the
telemetry-http-counters2
branch
from
e7f0589 to
902a13d
Compare
|
/azp run runtime-libraries outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-libraries outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-libraries outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
ghost
locked as resolved and limited conversation to collaborators
The Http Telemetry implementation from #37619 has a few problems:
This PR solves these issues by keeping the telemetry state on the HttpRequestMessage object.
This way we only call Stop if we called Start.
This PR is an alternative fix over #38876, avoiding using a new finalizable object to track each request when Telemetry is enabled.
Instead, this PR removes an optimization for
HttpConnectionwhere we avoid using a finalizer if maxConnections isint.MaxValue. With this change, we always use a finalizableHttpConnection.Contributes to #37428
Note: It might seem that the PR is removing calls to Telemetry - it is moving those calls that were always before a CompleteResponse/Dispose to those methods instead.