Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

System.Net.Sockets telemetry counters #39708

Merged
merged 9 commits into from
Jul 28, 2020
Merged

System.Net.Sockets telemetry counters #39708

merged 9 commits into from
Jul 28, 2020

Conversation

alnikola
Copy link
Contributor

@alnikola alnikola commented Jul 21, 2020 •
edited
Loading

Counters in SocketsTelemetry measuring

bytes-received (Bytes Received)
The cumulative total number of bytes received by all Socket objects since the process started.

bytes-sent (Bytes Sent)
The cumulative number of bytes sent by all Socket objects since the process started.

outgoing-connections-established (Outgoing Connections Established)
The cumulative total number of outgoing connections established by Socket objects for stream sockets since the process started.

incoming-connections-established (Incoming Connections Established)
The cumulative total number of incoming connections established by Socket objects for stream sockets since the process started.

datagrams-received (Datagrams Received)
The cumulative total number of datagram packets received by all Socket objects since the process started.

datagrams-sent (Datagrams Sent)
The cumulative total number of datagram packets sent by all Socket objects since the process started.

Contributes to #37428

@alnikola alnikola added this to the 5.0.0 milestone Jul 21, 2020
@alnikola alnikola requested review from noahfalk and a team July 21, 2020 15:15
@ghost
Copy link

ghost commented Jul 21, 2020

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

@jaredpar jaredpar mentioned this pull request Jul 21, 2020
Closed
@alnikola
Copy link
Contributor Author

/azp run runtime-libraries outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

MihaZupan
MihaZupan reviewed Jul 22, 2020
View reviewed changes
Copy link
Member

@MihaZupan MihaZupan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but I see the bytes received/sent telemetry is not done at SocketPal.(ReceiveAsync/ReceiveMessageFromAsync/SendAsync/SendToAsync) call-sites. Are those handeled elsewhere?

src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketsTelemetry.cs Outdated Show resolved Hide resolved
@alnikola
Copy link
Contributor Author

@MihaZupan Bytes and datagrams are counted in the Socket.cs because SocketsPal is platform dependent so I would have to duplicate code.
Do you see any issue with that approach?

@MihaZupan
Copy link
Member

I think it's good to do it in shared Sockets code.
My concern is around some methods being tracked (eg. SocketPal.Send, SocketPal.Receive) while some are not (eg. SocketPal.ReceiveAsync)

@alnikola
Copy link
Contributor Author

Async operations are tracked in the respective End*** methods. E.g. EndReceive()

@MihaZupan
Copy link
Member

Afaik, datagrams can also be sent/received with Send/Receive, not just SendTo/ReceiveFrom, as long as the endpoint is specified in a prior Connect call. Should those also check for the socket type being dgram?

@MihaZupan
Copy link
Member

SocketAsyncEventArgs overloads call into interop without going through Socket methods, I assume those are not tracked by this PR right now?

@alnikola
- SocketAsyncEventArgs log byte and datagram transfer events
d9fade2 
- Socket's Send/Recieve methods log datagram transfer
@alnikola
Copy link
Contributor Author

@MihaZupan You were right on the above two concerns. They are resolved now. Please, check again.

MihaZupan
MihaZupan approved these changes Jul 22, 2020
View reviewed changes
Copy link
Member

@MihaZupan MihaZupan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change LGTM

@alnikola alnikola requested a review from a team July 22, 2020 16:02
stephentoub
stephentoub reviewed Jul 22, 2020
View reviewed changes
src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEventArgs.cs Outdated
@@ -687,6 +692,7 @@ internal void FinishOperationSyncSuccess(int bytesTransferred, SocketFlags flags
LogBuffer(bytesTransferred);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good example of why the split across event sources is problematic. We're tracking bytes transferred with one event source but logging the contents with another.

stephentoub
stephentoub reviewed Jul 22, 2020
View reviewed changes
src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
{
SocketsTelemetry.Log.BytesReceived(bytesTransferred);
if (SocketType == SocketType.Dgram) SocketsTelemetry.Log.DatagramReceived();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about methods like Socket.SendFile?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't report the number of transferred bytes and the existing NetEventSource logs only source / destination addresses and file name, but not the size. Do you know how I can retrieve the transferred byte count here?

@alnikola
Accept telemetry added
524f077
@alnikola
- Transferred bytes logging in faliure cases removed
e016da3 
- _currentSocket?.SocketType call moved under IsEnabled() guarding check
@alnikola
Copy link
Contributor Author

/azp run runtime-libraries outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@alnikola
Copy link
Contributor Author

Accept telemetry added. Please, check.

@alnikola alnikola merged commit d319970 into dotnet:master Jul 28, 2020
@alnikola alnikola deleted the alnikola/sockets-telemetry-counters branch July 28, 2020 14:56
Jacksondr5 pushed a commit to Jacksondr5/runtime that referenced this pull request Aug 10, 2020
@alnikola @Jacksondr5
System.Net.Sockets telemetry counters (dotnet#39708)
fe8128a 
Counters in SocketsTelemetry measuring

`bytes-received` (Bytes Received)
The cumulative total number of bytes received by all Socket objects since the process started.

`bytes-sent` (Bytes Sent)
The cumulative number of bytes sent by all Socket objects  since the process started.

`outgoing-connections-established` (Outgoing Connections Established)
The cumulative total number of outgoing connections established by Socket objects for stream sockets since the process started.

`incoming-connections-established` (Incoming Connections Established)
The cumulative total number of incoming connections established by Socket objects for stream sockets since the process started.

`datagrams-received` (Datagrams Received)
The cumulative total number of datagram packets received by all Socket objects since the process started.

`datagrams-sent` (Datagrams Sent)
The cumulative total number of datagram packets sent by all Socket objects since the process started.

Contributes to dotnet#37428
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@davidfowl davidfowl davidfowl left review comments

@stephentoub stephentoub stephentoub left review comments

@noahfalk noahfalk noahfalk left review comments

@geoffkizer geoffkizer geoffkizer left review comments

@MihaZupan MihaZupan MihaZupan approved these changes

Assignees
No one assigned
Labels
area-System.Net.Sockets
Projects
None yet
Milestone
5.0.0
Development

Successfully merging this pull request may close these issues.
6 participants
@alnikola @MihaZupan @davidfowl @stephentoub @noahfalk @geoffkizer