Add more System.Net.Http counters #38686
Conversation
|
Tagging subscribers to this area: @dotnet/ncl |
| @@ -31,6 +31,10 @@ namespace System.Net.Http | |||
| /// <summary>Provides a set of connection pools, each for its own endpoint.</summary> | |||
| internal sealed class HttpConnectionPoolManager : IDisposable | |||
| { | |||
| // Used by HttpTelemetry to calculate statistics over all connection pools | |||
| private static readonly ConcurrentDictionary<HttpConnectionPoolManager, object?> s_allConnectionPools = | |||
| new ConcurrentDictionary<HttpConnectionPoolManager, object?>(); | |||
There was a problem hiding this comment.
Unfortunately to implement this, we have to keep track of all connection pools, even if telemetry is disabled (since we can't do it after the fact). Is there an alternative approach where we can avoid this?
There was a problem hiding this comment.
Also, since this is effectively a ConcurrentHashSet, is it more efficient to use object or something like byte for the dummy value?
There was a problem hiding this comment.
This is a potentually huge leak. Pools are now rooted until/unless they're explicitly Disposed.
There was a problem hiding this comment.
Changed to keep WeakReferences around instead, and use a finalizer to ensure the pool manager is removed from the global queue.
| } | ||
| } | ||
| return max; | ||
| } |
There was a problem hiding this comment.
Instead of maintaining a collection and looping, can you modify HttpConnectionPool to call a HttpTelemetry.SetNewMaximum(x) whenever it opens up a new connection, and Http2Connection to do similar when it opens a new stream? It should be much cheaper this way.
There was a problem hiding this comment.
I still question the value of these max counters. Wouldn't a current count be more useful? You could approximate a max on top of it, but you can't do the inverse.
There was a problem hiding this comment.
I agree. A current count seems a lot more useful.
There was a problem hiding this comment.
can you modify HttpConnectionPool to call a HttpTelemetry.SetNewMaximum(x) whenever it opens up a new connection, and Http2Connection to do similar when it opens a new stream?
The maximum reported here can fluctuate up and down, from what I understand that approach would only allow growing the max over time.
I also don't see the value of this information. @davidsh if you have the time, could you share what use case you had in mind?
By "current count", I assume you mean a sum of all opened http11 connections / all active Http2 streams? (in which case this global list mess isn't needed since we can just increment/decrement a global counter)
There was a problem hiding this comment.
By "current count", I assume you mean a sum of all opened http11 connections / all active Http2 streams?
Yup
There was a problem hiding this comment.
By "current count", I assume you mean a sum of all opened http11 connections / all active Http2 streams
Does it really make sense to count total number of streams on all connections? I think it would be better to count HTTP/2 connections only. HTTP/2 stream is a logical entity in the nutshell whereas HTTP/2 connection is a physical one consuming limited resources (in example SNAT ports).
|
Closing as we don't see value in these specific counters in favour of counters for current totals |
| @@ -1553,6 +1553,10 @@ private static bool GetIsWindows7Or2008R2() | |||
| memberName, // method name | |||
| message); // message | |||
|
|
|||
| public int Http11ConnectionCount => _associatedConnectionCount; | |||
|
|
|||
| public int Http20StreamCount => _http2Connection?.HttpStreamCount ?? 0; | |||
There was a problem hiding this comment.
Could we some how make it more friendly to upcoming multiple HTTP/2 connection API?
| } | ||
| } | ||
| return max; | ||
| } |
There was a problem hiding this comment.
By "current count", I assume you mean a sum of all opened http11 connections / all active Http2 streams
Does it really make sense to count total number of streams on all connections? I think it would be better to count HTTP/2 connections only. HTTP/2 stream is a logical entity in the nutshell whereas HTTP/2 connection is a physical one consuming limited resources (in example SNAT ports).
| { | ||
| foreach ((_, HttpConnectionPool connectionPool) in connectionPoolManager._pools) | ||
| { | ||
| max = Math.Max(max, connectionPool.Http11ConnectionCount); |
There was a problem hiding this comment.
I'd suggest to consider using an associative MaxHeap (i.e. MaxHeap combined with Dictionary to access elements for O(1)) where each node stores open connection count for a single HttpConnectionPool.
On connection open or close, the pool will increase or decrease the respective counter. Since each pool will change only their own counter, it can be easily done thread safely. When the polling counter _maxHttp11ConnectionsPerPoolCounter triggers, it will call Heapify method to rebuild the heap and get the max counter on the top. Heapify runs in O(logn) in the worst case so it seems fast enough.
There is one more issue though, that is to ensure Heapify can run concurrently with heap nodes changes. To solve that, I'd suggest to guard Heapify call with lock, but allow concurrent changes to counters on heap as explained above.
There was a problem hiding this comment.
Thank you for the idea! While having a better O() when collecting the maximum on polling intervals, I am doubtful it would be a measurable perf improvement at the end, especially considering the added complexity.
The current approach tries to minimize the overhead when telemetry is disabled. The proposed one would incur an overhead on every connection even when telemetry is disabled.
While the O(N) for this collection might seem scary, it only runs once per polling interval, and the N is effectively capped at the port limit, so I doubt it would show up as a significant overhead.
There was a problem hiding this comment.
The proposed one would incur an overhead on every connection even when telemetry is disabled.
That's not exactly so. If telemetry is disabled increase/decrease counter methods will be no-op and Heapify will never be called since timer will be disabled as well.
However, I do agree it adds more complexity.
ghost
locked as resolved and limited conversation to collaborators
Add
http11-connections-single-pool-max(HTTP 1.1 connections per pool)The maximum number (high-water mark) of TCP connections across any HTTP 1.1 connection pool in SocketsHttpHandler.
Add
http20-streams-single-connection-max(HTTP 2.0 streams per connection)The maximum number (high-water mark) of HTTP/2 streams across any HTTP 2.0 connection in SocketsHttpHandler.
Contributes to #37428