-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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.
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
Don't log datagrams failed to send/receive #40083
Don't log datagrams failed to send/receive #40083
Conversation
Tagging subscribers to this area: @dotnet/ncl |
There was a problem hiding this 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'm wondering why don't we have test changes in this PR?
Isn't it a requirement for the telemetry feature that both positive and negative cases are well-tested?
@@ -1180,8 +1180,7 @@ public int Send(IList<ArraySegment<byte>> buffers, SocketFlags socketFlags, out | |||
// Don't log transfered byte count in case of a failure. | |||
return 0; | |||
} | |||
|
|||
if (SocketsTelemetry.Log.IsEnabled()) | |||
else if (SocketsTelemetry.Log.IsEnabled()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgive me my stupid question, but how does the else if
instead of just if
here helps? If the previous block ends with return
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it uniformly in all cases for the sake of a higher readability. There are other places without return
inside the if (errorCode != SocketError.Success)
, but we still don't want to log anything there in case of a failure. (one example)
@antonfirsov It would be unreasonably hard to cover all possible telemetry calls with tests, and even if we did it, tests would be extremely fragile since telemetry calls can be added/moved around over time in different places. |
/azp run runtime-libraries outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
Socket's telemetry incorrectly logs datagrams as being sent or received even if the operation failed. This PR fixes it.