Add current-outgoing-connect-attempts counter to Sockets #66651
Conversation
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsFollowup from #66605 (comment). We have We also have
|
src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketsTelemetry.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketsTelemetry.cs
Show resolved
Hide resolved
| @@ -216,11 +216,11 @@ private void AfterConnectAcceptTelemetry() | |||
| switch (LastOperation) | |||
| { | |||
| case SocketAsyncOperation.Accept: | |||
| SocketsTelemetry.Log.AfterAccept(SocketError); | |||
| SocketsTelemetry.AfterAccept(SocketError); | |||
There was a problem hiding this comment.
Don't these changes mean we'll no longer be able to trim out as much as we otherwise would have if EventSource trimming is enabled?
There was a problem hiding this comment.
Yes, though all the core event logic is still guarded by IsEnabled checks.
What may now remain are the Interlocked calls mentioned above and this switch method.
If we wanted those to be trimmed out completely, I think we should expose the EventSource.IsSupported flag instead (currently internal), as that is what we actually care about here, not whether someone is listening at this exact moment.
Since IsEnabled can toggle between on/off, it can't be used the same way without complicating the calling logic.
|
@stephentoub Besides the static vs. non-static differences, is there something that should change here, or are we okay with the current approach? |
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@stephentoub can you please take a look at this one again? I've changed it back to using non-static methods/fields for consistency. |
src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEventArgs.cs
Show resolved
Hide resolved
MihaZupan
force-pushed
the
sockets-telem-current
branch
from
7f05950
to
a7d2b27
Compare
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
This test failure is related, looking into it ... |
MihaZupan
force-pushed
the
sockets-telem-current
branch
from
a7d2b27
to
dc06015
Compare
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
ghost
locked as resolved and limited conversation to collaborators
Followup from #66605 (comment).
We have
current-requestsfor HTTP,current-tls-handshakesfor Security, andcurrent-dns-lookupsfor name resolution, but we only have totals for Sockets.current-outgoing-connect-attempts(feel free to suggest better names) would be the Sockets counterpart.We also have
httpXX-connections-current-total,tlsXX-sessions-openanddns-lookups-requestedcounters.Any thoughts on adding something like that for Sockets (as in number of currently connected sockets for the process)?
Note that we don't currently distinguish between types of socket when counting
outgoing-connections-established, even UDP socket connects count.