Extend diagnostics to handle tcpip diagnostics client option #2833
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mdh1418
changed the title
mdh1418
commented
Jan 28, 2022
src/Microsoft.Diagnostics.NETCore.Client/DiagnosticsIpc/IpcEndpointConfig.cs
Outdated
Show resolved
Hide resolved
mdh1418
force-pushed
the
diagnostics_tcpclient_cleanup
branch
from
54dfdff
to
1d3e055
Compare
josalem
reviewed
Jan 28, 2022
src/Microsoft.Diagnostics.NETCore.Client/DiagnosticsClient/DiagnosticsClient.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Diagnostics.NETCore.Client/DiagnosticsIpc/IpcEndpointConfig.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Diagnostics.NETCore.Client/DiagnosticsIpc/IpcTransport.cs
Outdated
Show resolved
Hide resolved
noahfalk
reviewed
Feb 1, 2022
src/Microsoft.Diagnostics.NETCore.Client/DiagnosticsIpc/IpcEndpointConfig.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Diagnostics.NETCore.Client/DiagnosticsIpc/IpcEndpointConfig.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Diagnostics.NETCore.Client/DiagnosticsClient/DiagnosticsClient.cs
Outdated
Show resolved
Hide resolved
mdh1418
force-pushed
the
diagnostics_tcpclient_cleanup
branch
2 times, most recently
from
a9db366
to
0bfbb2e
Compare
|
@noahfalk @hoyosjs @josalem This PR captures the entirety of expected changes to Microsoft.Diagnostics.NETCore.Client source code in order to get |
noahfalk
approved these changes
Feb 15, 2022
| @@ -13,6 +13,11 @@ | |||
| <IsShipping>true</IsShipping> | |||
| </PropertyGroup> | |||
|
|
|||
| <PropertyGroup Condition="'$(GitHubRepositoryName)' == 'runtime'"> | |||
| <DefineConstants>$(DefineConstants);DIAGNOSTICS_RUNTIME</DefineConstants> | |||
There was a problem hiding this comment.
Do we need to define NET_6 here, or does that happen somewhere else?
There was a problem hiding this comment.
NET_6 should be defined iff compiling against net6.0+ TFMs.
hoyosjs
approved these changes
Feb 15, 2022
hoyosjs
reviewed
Feb 16, 2022
| @@ -23,6 +22,8 @@ public IpcSocket(AddressFamily addressFamily, SocketType socketType, ProtocolTyp | |||
| { | |||
| } | |||
|
|
|||
| // .NET 6 implements this method directly on Socket, but for earlier runtimes we need a polyfill | |||
| #if !NET6_0 | |||
| public async Task<Socket> AcceptAsync(CancellationToken token) | |||
There was a problem hiding this comment.
FYI - this override will always be used at runtime given our compilation modes, regardless of the runtime version that gets used.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In effort to enable diagnostics tests for mobile (specifically Android) in dotnet/runtime#60879, it was realized that some changes to
Microsoft.Diagnostics.NETCore.Clientare needed in order for the eventpipe runtime tests to work (bigevent, buffersize, eventsourceerror). More specifically, DiagnosticsClient needed to handle a TCPIP option. With most of the functionality brought up by @lateralusX previously, just a new transport type is needed for the tests to work (without the new transport type, exceptions are thrown because the API only expects two type of transports)The expectation for the shared code between this repository and
Dotnet/Runtimeis that any modifications tosrc/tests/tracing/eventpipe/common/Microsoft.Diagnostics.NETCore.ClientinDotnet/Runtimemust first be made here inDotnet/Diagnosticsand once passed, will then be downstreamed toDotnet/Runtime.This pr accomplishes the following:
Dotnet/Runtime-specific PropertyGroup and ItemGroup intoMicrosoft.Diagnostics.NETCore.Client.csprojto defineDIAGNOSTICS_RUNTIMEconstant, ignore compiler warnings, and compile the source code.TcpSocketfor TCP/IP case yet wraps it under aDIAGNOSTICS_RUNTIME#if directive to limit modifications to this repository. Also extendsIpcTransport.Connectwith TcpClient to handle the TCP/IP transport.AssemblyInfo.cscompletely wrapped under the above mentioned directive in order for internals to be visible ondotnet/runtime/src/tests/tracing/eventpipe/common/.IpcSocket.AcceptAsyncandIpcSocket.ConnectAsyncin a#if !NET6_0preprocessor directive because .NET 6 implements the methods directly and throws onDotnet/RuntimeIpcEndpointConfig.Address