dotnet / dotnet/diagnostics

API Proposal: Refactor ReversedDiagnosticsServer and *EndpointInfoSource classes

Open
#2,131 0 comments 0 reactions 0 assignees View on GitHub
enhancement Microsoft.Diagnostics.NETCore.Client
Dominant language
C++
Stars
1.3k
Forks
404
Avg merge
2d 5h
Merged PRs (30d)
35

Description

## Background and Motivation

The current `*EndpointInfoSource` classes are rigid about how new diagnostic connections are introduced into the connection cache (if there is a cache at all) and how those connections are configured (e.g. `ServerEndpointInfoSource` always calls ResumeRuntime for each new connection; there is no opportunity for additional configuration before adding connections to the cache).

I believe that separating the production of new connections from the `*EndpointInfoSource` classes and standardizing on a means of providing new connections to the `*EndpointInfoSource` classes provides opportunities for better caching of connections and configuring connections before they are added to the caches. This would enable scenarios such as setting up event pipe sessions on new connections, for triggering capabilities, before the connections are made more broadly available via the `*EndpointInfoSource`.

This proposal largely describes the "public" surface area changes and leaves out other private implementations.

## Proposed API Changes

```diff
namespace Microsoft.Diagnostics.NETCore.Client
{
internal struct IpcEndpointInfo
{
public IpcEndpoint Endpoint { get; }

public int ProcessId { get; }

- public Guid RuntimeInstanceCookie { get; }
+ public Guid InstanceCookie { get; }
}

- internal sealed class ReversedDiagnosticsServer : IAsyncDisposable
+ internal sealed class DiagnosticPortListener : IDiagnosticPortProvider, IAsyncDisposable
{
- public ReversedDiagnosticsServer(string transportPath);
+ public DiagnosticPortListener(string transportPath);

public ValueTask DisposeAsync();

public void Start();
public void Start(int maxConnections);

public IpcEndpointInfo Accept(TimeSpan timeout);
public Task AcceptAsync(CancellationToken token);

- public bool RemoveConnection(Guid runtimeCookie);
+ public bool Discard(IpcEndpointInfo endpointInfo);

public static int MaxAllowedConnections;
}

+ internal sealed class DiagnosticPortConnector : IDiagnosticPortProvider, IAsyncDisposable
+ {
+ public DiagnosticPortConnector();

+ public async ValueTask DisposeAsync();

+ public void Start();

+ public IpcEndpointInfo Accept(TimeSpan timeout);
+ public Task AcceptAsync(CancellationToken token);

+ public bool Discard(IpcEndpointInfo endpointInfo);
+ }

+ internal interface IDiagnosticPortProvider
+ {
+ IpcEndpointInfo Accept(TimeSpan timeout);
+ Task AcceptAsync(CancellationToken token);

+ bool Discard(IpcEndpointInfo endpointInfo);
+ }
}

namespace Microsoft.Diagnostics.Monitoring
{
- internal sealed class ClientEndpointInfoSource : IEndpointInfoSourceInternal
- {
- public Task> GetEndpointInfoAsync(CancellationToken token);
- }

- internal class ServerEndpointInfoSource : IEndpointInfoSourceInternal, IAsyncDisposable
+ internal sealed class EndpointInfoSource : IAsyncDisposable
{
- public ServerEndpointInfoSource(string transportPath);
+ public EndpointInfoSource(IDiagnosticPortProvider provider);

public ValueTask DisposeAsync();

- public void Start();
- public void Start(int maxConnections);

public Task> GetEndpointInfoAsync(CancellationToken token);
}
}
```

## Usage Examples

The follow pseudo-code example shows how dotnet-monitor might configure each port provider, add a new intermediate port provider in the "pipeline" for setting up event pipe sessions and resuming runtimes, and finally feed into the `EndpointInfoSource`.

```cs
IDiagnosticPortProvider provider;
if (Listen == connectionMode)
{
DiagnosticPortListener listener = new(transportPath: "diagport");
listener.Start(maxConnections: 16);
provider = listener;
}
else
{
DiagnosticPortConnector connector = new();
connector.Start();
provider = connector;
}

// These two classes could be merged if EndpointInfoSource becomes an
// implementation detail of dotnet-monitor.
WrapperPortProvider wrapperProvider = new(listener);
EndpointInfoSource source = new(wrapperProvider);

...

IEnumerable endpointInfos = await source.GetEndpointInfoAsync(token);
foreach (IEndpointInfo endpointInfo in endpointInfos)
{
Console.WriteLine(endpointInfo.Pid);
}

...

// Wraps an IDiagnosticPortProvider, sets up triggers on each endpoint,
// and resumes the runtime for each endpoint.
internal sealed class WrapperPortProvider : IDiagnosticPortProvider
{
private readonly IDiagnosticPortProvider _provider;

public WrapperPortProvider(IDiagnosticPortProvider provider)
{
_provider = provider;
}

public IpcEndpointInfo Accept(TimeSpan timeout)
{
IpcEndpointInfo endpointInfo = _provder.Accept(timeout);
SetupTriggersAndResume(endpointInfo);
return endpointInfo;
}

public Task AcceptAsync(CancellationToken token)
{
IpcEndpointInfo endpointInfo = await _provder.AcceptAsync(timeout);
SetupTriggersAndResume(endpointInfo);
return endpointInfo;
}

public bool Discard(IpcEndpointInfo endpointInfo)
{
_provider.Discard(endpointInfo);
}

private void SetupTriggersAndResume(IpcEndpointInfo info)
{
var client = new DiagnosticsClient(info.Endpoint);

// Setup event pipe sessions to monitor for triggers.

// Resume runtime
try
{
client.ResumeRuntime();
}
catch (ServerErrorException)
{
// The runtime likely doesn't understand the ResumeRuntime command e.g. .NET Core 3.1
}
}
}
```

## Details

- The `DiagnosticPortListener` (formerly `ReversedDiagnosticsServer`) largely remains unchanged except for some renames.
- The `DiangosticPortConnector` is very similar to `DiagnosticPortListener` except that it periodically scans for the listening diagnostic ports of applications found in the default diagnostic port location (e.g. /tmp/* for Unix or \\.\pipe\* for Windows). It will only report newly discovered instances. Two or more diagnostic ports with the same process ID but different disambiguators in their port name will be reported separately as two or more distinct connections (this allows for communication with multiple Linux containers in the same pod because they will likely all have process ID of 1).
- The `IpcEndpointInfo.InstanceCookie` will have a value of:
- the runtime instance cookie value when provided by `DiagnosticPortListener`
- a Guid generated from bit splicing or hashing the (1) process ID, and (2) either the diagnostic port discriminator (for Unix) or the process start time (for Windows)
- `EndpointInfoSource` is largely a merger of `ServerEndpointInfoSource` and `ClientEndpointInfoSource`, but with the distinction that it does not need to understand the direction of the diagnostic port connection. It still tests the viability of each cached connection and calls `IDiagnosticPortProvider.Discard(IpcEndpointInfo)` when a connection is no longer viable, to aid in cleaning up stale connection state in the port providers.

cc @josalem, @sywhang, @wiktork

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.