-
Notifications
You must be signed in to change notification settings - Fork 383
Extend diagnostics to handle tcpip diagnostics client option #2833
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
Conversation
src/Microsoft.Diagnostics.NETCore.Client/DiagnosticsIpc/IpcEndpointConfig.cs
Outdated
Show resolved
Hide resolved
54dfdff to
1d3e055
Compare
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
left a comment
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 think this is fine in principle - suggested a few changes and agreed with @josalem that the constructor should be internal.
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
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
left a comment
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.
LGTM modulo a few minor comments inline
| </PropertyGroup> | ||
|
|
||
| <PropertyGroup Condition="'$(GitHubRepositoryName)' == 'runtime'"> | ||
| <DefineConstants>$(DefineConstants);DIAGNOSTICS_RUNTIME</DefineConstants> |
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.
Do we need to define NET_6 here, or does that happen somewhere else?
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.
NET_6 should be defined iff compiling against net6.0+ TFMs.
|
|
||
| // .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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI - this override will always be used at runtime given our compilation modes, regardless of the runtime version that gets used.
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