Add DiagnosticsAgent API

[josalem]

The DiagnosticsAgent API can be used to create a Diagnostics IPC Protocol server. A working sample is provided in the updated documentation. New connections trigger an event that provides users with a DiagnosticsClient object that can be used the same as the current implementation.

N.B.: the implementation of this API will leverage dotnet/runtime#33307. I have done local testing with a version of this API and things worked well. Check out the dotnet-reverse tool in my hack branch. Note that the other tools will be broken in that branch and only dotnet-reverse builds.

CC - @tommcdon@davidfowl

[josalem]

@sywhang and I chatted offline about whether the OnDiagnosticsConnection event needs to be public. He suggested an alternative where it is private and the handler is passed in via the ctor, e.g.,

public DiagnosticsAgent(string ipcEndpointAddress, Action<DiagnosticsConnectionEventArgs> connectionHandler);

Action<DiagnosticsConnectionEventArgs>connectionHandler=(eventArgs)=>{clientDict[eventArgs.RuntimeInstanceCookie]=(eventArgs.ProcessId,eventArgs.Client);Console.Write($"== New Connection: instanceCookie: {eventArgs.RuntimeInstanceCookie}, ProcessId: {eventArgs.ProcessId}\n> ");};using(DiagnosticsAgentagent=newDiagnosticsAgent(address,connectionHandler)){// ...}

[josalem]

CC @wiktork

[sywhang]

My suggestion was mostly based on whether the user would need to ever change it after creating the DiagnosticsAgent. It wasn't clear to me whether there would be a use case involving that, so I think it would be safer to first design it in a way that explicitly bans user from changing it.

If it is made public property, it is possible for a user to create a server socket and forget to create a handler, and never handle anything from the incoming connections. Making it an argument to the constructor makes it more clear for the user to create that handler with the server socket, which should be what they are doing with the public constructor regardless.

[jander-msft]

Will there be some mechanism by which multiple DiagnosticsClients can be established to the same process from one DiagnosticsAgent? I thinking that dotnet-monitory might need this if, for example, there is a active session for capturing logs, metrics, etc and then there is another request to capture dumps. I don't know if dotnet-monitor would be able to use the same connection concurrently for different collections of data.

[noahfalk]

I agree, I don't think a connection should include the DiagnosticClient. I'd suggest the callback arg/return value can be used to create an arbitrary number of DiagnosticClients. For example:

var agent = new DiagnosticsAgent(address);
var runtimeEndpoint = await agent.ListenForRuntimesAsync();
Console.WriteLine("Dotnet process detected with pid {runtimeEndpoint.Pid}");
// only listening to one for simplicity
while(true)
{
DotnetMonitorRequest r = await webserver.GetNextRequestAsync();
DiagnosticClient c = new DiagnosticClient(runtimeEndpoint);
Task.Run(() => HandleRequest(r,c));
}

[josalem]

I don't think there's anything preventing you from using the same DiagnosticsClient for multiple actions at once, even in the current implementation. You should already be able to start N<64 tracing sessions per process at once, and use the same DiagnosticsClient to capture dumps, gcdumps, etc. at the same time.

Is the suggestion that DiagnosticClients should be disposable, e.g., use once, constructs? Right now, DiagnosticClients abstract the connection process entirely for both traditional and reversed connections. Every time you issue a command for a traditional connection, it reconnects to the underlying transport. This new iteration, effectively does the same thing for reverse connections, but does so by caching the reverse connection each time the runtime advertises.

[noahfalk]

I don't think there's anything preventing you from using the same DiagnosticsClient for multiple actions at once, even in the current implementation

You could design it to work, but by convention .NET types are not considered thread-safe unless specifically documented otherwise. We could implement and document that DiagnosticsClient is thread-safe, but people will probably assume that it isn't by default. I'd recommend going with convention is easier than trying to explain that the API supports free-threaded use.

Is the suggestion that DiagnosticClients should be disposable, e.g., use once, constructs?

I think we should allow, but not require, that kind of usage.

[davidfowl]

Can we make this more of a listener/socket pull style API?

varagent=newDiagnosticsAgent(address);while(true){varconnection=awaitagent.AcceptAsync();_=ProcessConnectionAsync(connection);}

[josalem]

Would DiagnosticsAgent.AcceptAsync() return a connectionevery time a runtime instance connects to the agent or only the first time? The current model raises the OnDiagnosticsConnection event on only the first connection and users are expected to reuse the DiagnosticsClient (which abstracts subsequent reconnects) they get just like they use the traditional DiagnosticsClient today. if we change it to be "every time an instance connects" we may want to modify the DiagnosticsClient API for dispatching commands, or not use it entirely.

[noahfalk]

I'd suggest it is first time only. I am hoping to preserve the model that you get a DiagnosticClient and then you send an arbitrary number of commands without making the user aware of the underlying transport stream management.

[josalem]

This PR is no longer needed since #1303 went in.

[josalem]

The DiagnosticsAgent API can be used to create a Diagnostics IPC Protocol server. A working sample is provided in the updated documentation. New connections trigger an event that provides users with a DiagnosticsClient object that can be used the same as the current implementation.

N.B.: the implementation of this API will leverage dotnet/runtime#33307. I have done local testing with a version of this API and things worked well. Check out the dotnet-reverse tool in my hack branch. Note that the other tools will be broken in that branch and only dotnet-reverse builds.

CC - @tommcdon@davidfowl

[josalem]

@sywhang and I chatted offline about whether the OnDiagnosticsConnection event needs to be public. He suggested an alternative where it is private and the handler is passed in via the ctor, e.g.,

public DiagnosticsAgent(string ipcEndpointAddress, Action<DiagnosticsConnectionEventArgs> connectionHandler);

Action<DiagnosticsConnectionEventArgs>connectionHandler=(eventArgs)=>{clientDict[eventArgs.RuntimeInstanceCookie]=(eventArgs.ProcessId,eventArgs.Client);Console.Write($"== New Connection: instanceCookie: {eventArgs.RuntimeInstanceCookie}, ProcessId: {eventArgs.ProcessId}\n> ");};using(DiagnosticsAgentagent=newDiagnosticsAgent(address,connectionHandler)){// ...}

[josalem]

CC @wiktork

[sywhang]

My suggestion was mostly based on whether the user would need to ever change it after creating the DiagnosticsAgent. It wasn't clear to me whether there would be a use case involving that, so I think it would be safer to first design it in a way that explicitly bans user from changing it.

If it is made public property, it is possible for a user to create a server socket and forget to create a handler, and never handle anything from the incoming connections. Making it an argument to the constructor makes it more clear for the user to create that handler with the server socket, which should be what they are doing with the public constructor regardless.

[jander-msft]

Will there be some mechanism by which multiple DiagnosticsClients can be established to the same process from one DiagnosticsAgent? I thinking that dotnet-monitory might need this if, for example, there is a active session for capturing logs, metrics, etc and then there is another request to capture dumps. I don't know if dotnet-monitor would be able to use the same connection concurrently for different collections of data.

[noahfalk]

I agree, I don't think a connection should include the DiagnosticClient. I'd suggest the callback arg/return value can be used to create an arbitrary number of DiagnosticClients. For example:

var agent = new DiagnosticsAgent(address);
var runtimeEndpoint = await agent.ListenForRuntimesAsync();
Console.WriteLine("Dotnet process detected with pid {runtimeEndpoint.Pid}");
// only listening to one for simplicity
while(true)
{
DotnetMonitorRequest r = await webserver.GetNextRequestAsync();
DiagnosticClient c = new DiagnosticClient(runtimeEndpoint);
Task.Run(() => HandleRequest(r,c));
}

[josalem]

I don't think there's anything preventing you from using the same DiagnosticsClient for multiple actions at once, even in the current implementation. You should already be able to start N<64 tracing sessions per process at once, and use the same DiagnosticsClient to capture dumps, gcdumps, etc. at the same time.

Is the suggestion that DiagnosticClients should be disposable, e.g., use once, constructs? Right now, DiagnosticClients abstract the connection process entirely for both traditional and reversed connections. Every time you issue a command for a traditional connection, it reconnects to the underlying transport. This new iteration, effectively does the same thing for reverse connections, but does so by caching the reverse connection each time the runtime advertises.

[noahfalk]

I don't think there's anything preventing you from using the same DiagnosticsClient for multiple actions at once, even in the current implementation

You could design it to work, but by convention .NET types are not considered thread-safe unless specifically documented otherwise. We could implement and document that DiagnosticsClient is thread-safe, but people will probably assume that it isn't by default. I'd recommend going with convention is easier than trying to explain that the API supports free-threaded use.

Is the suggestion that DiagnosticClients should be disposable, e.g., use once, constructs?

I think we should allow, but not require, that kind of usage.

[davidfowl]

Can we make this more of a listener/socket pull style API?

varagent=newDiagnosticsAgent(address);while(true){varconnection=awaitagent.AcceptAsync();_=ProcessConnectionAsync(connection);}

[josalem]

Would DiagnosticsAgent.AcceptAsync() return a connectionevery time a runtime instance connects to the agent or only the first time? The current model raises the OnDiagnosticsConnection event on only the first connection and users are expected to reuse the DiagnosticsClient (which abstracts subsequent reconnects) they get just like they use the traditional DiagnosticsClient today. if we change it to be "every time an instance connects" we may want to modify the DiagnosticsClient API for dispatching commands, or not use it entirely.

[noahfalk]

I'd suggest it is first time only. I am hoping to preserve the model that you get a DiagnosticClient and then you send an arbitrary number of commands without making the user aware of the underlying transport stream management.

[josalem]

This PR is no longer needed since #1303 went in.

[josalem]

The DiagnosticsAgent API can be used to create a Diagnostics IPC Protocol server. A working sample is provided in the updated documentation. New connections trigger an event that provides users with a DiagnosticsClient object that can be used the same as the current implementation.

N.B.: the implementation of this API will leverage dotnet/runtime#33307. I have done local testing with a version of this API and things worked well. Check out the dotnet-reverse tool in my hack branch. Note that the other tools will be broken in that branch and only dotnet-reverse builds.

CC - @tommcdon@davidfowl

[josalem]

@sywhang and I chatted offline about whether the OnDiagnosticsConnection event needs to be public. He suggested an alternative where it is private and the handler is passed in via the ctor, e.g.,

public DiagnosticsAgent(string ipcEndpointAddress, Action<DiagnosticsConnectionEventArgs> connectionHandler);

Action<DiagnosticsConnectionEventArgs>connectionHandler=(eventArgs)=>{clientDict[eventArgs.RuntimeInstanceCookie]=(eventArgs.ProcessId,eventArgs.Client);Console.Write($"== New Connection: instanceCookie: {eventArgs.RuntimeInstanceCookie}, ProcessId: {eventArgs.ProcessId}\n> ");};using(DiagnosticsAgentagent=newDiagnosticsAgent(address,connectionHandler)){// ...}

[josalem]

CC @wiktork

[sywhang]

My suggestion was mostly based on whether the user would need to ever change it after creating the DiagnosticsAgent. It wasn't clear to me whether there would be a use case involving that, so I think it would be safer to first design it in a way that explicitly bans user from changing it.

If it is made public property, it is possible for a user to create a server socket and forget to create a handler, and never handle anything from the incoming connections. Making it an argument to the constructor makes it more clear for the user to create that handler with the server socket, which should be what they are doing with the public constructor regardless.

[jander-msft]

Will there be some mechanism by which multiple DiagnosticsClients can be established to the same process from one DiagnosticsAgent? I thinking that dotnet-monitory might need this if, for example, there is a active session for capturing logs, metrics, etc and then there is another request to capture dumps. I don't know if dotnet-monitor would be able to use the same connection concurrently for different collections of data.

[noahfalk]

I agree, I don't think a connection should include the DiagnosticClient. I'd suggest the callback arg/return value can be used to create an arbitrary number of DiagnosticClients. For example:

var agent = new DiagnosticsAgent(address);
var runtimeEndpoint = await agent.ListenForRuntimesAsync();
Console.WriteLine("Dotnet process detected with pid {runtimeEndpoint.Pid}");
// only listening to one for simplicity
while(true)
{
DotnetMonitorRequest r = await webserver.GetNextRequestAsync();
DiagnosticClient c = new DiagnosticClient(runtimeEndpoint);
Task.Run(() => HandleRequest(r,c));
}

[josalem]

I don't think there's anything preventing you from using the same DiagnosticsClient for multiple actions at once, even in the current implementation. You should already be able to start N<64 tracing sessions per process at once, and use the same DiagnosticsClient to capture dumps, gcdumps, etc. at the same time.

Is the suggestion that DiagnosticClients should be disposable, e.g., use once, constructs? Right now, DiagnosticClients abstract the connection process entirely for both traditional and reversed connections. Every time you issue a command for a traditional connection, it reconnects to the underlying transport. This new iteration, effectively does the same thing for reverse connections, but does so by caching the reverse connection each time the runtime advertises.

[noahfalk]

I don't think there's anything preventing you from using the same DiagnosticsClient for multiple actions at once, even in the current implementation

You could design it to work, but by convention .NET types are not considered thread-safe unless specifically documented otherwise. We could implement and document that DiagnosticsClient is thread-safe, but people will probably assume that it isn't by default. I'd recommend going with convention is easier than trying to explain that the API supports free-threaded use.

Is the suggestion that DiagnosticClients should be disposable, e.g., use once, constructs?

I think we should allow, but not require, that kind of usage.

[davidfowl]

Can we make this more of a listener/socket pull style API?

varagent=newDiagnosticsAgent(address);while(true){varconnection=awaitagent.AcceptAsync();_=ProcessConnectionAsync(connection);}

[josalem]

Would DiagnosticsAgent.AcceptAsync() return a connectionevery time a runtime instance connects to the agent or only the first time? The current model raises the OnDiagnosticsConnection event on only the first connection and users are expected to reuse the DiagnosticsClient (which abstracts subsequent reconnects) they get just like they use the traditional DiagnosticsClient today. if we change it to be "every time an instance connects" we may want to modify the DiagnosticsClient API for dispatching commands, or not use it entirely.

[noahfalk]

I'd suggest it is first time only. I am hoping to preserve the model that you get a DiagnosticClient and then you send an arbitrary number of commands without making the user aware of the underlying transport stream management.

[josalem]

This PR is no longer needed since #1303 went in.

[josalem]

The DiagnosticsAgent API can be used to create a Diagnostics IPC Protocol server. A working sample is provided in the updated documentation. New connections trigger an event that provides users with a DiagnosticsClient object that can be used the same as the current implementation.

N.B.: the implementation of this API will leverage dotnet/runtime#33307. I have done local testing with a version of this API and things worked well. Check out the dotnet-reverse tool in my hack branch. Note that the other tools will be broken in that branch and only dotnet-reverse builds.

CC - @tommcdon@davidfowl

[josalem]

@sywhang and I chatted offline about whether the OnDiagnosticsConnection event needs to be public. He suggested an alternative where it is private and the handler is passed in via the ctor, e.g.,

public DiagnosticsAgent(string ipcEndpointAddress, Action<DiagnosticsConnectionEventArgs> connectionHandler);

Action<DiagnosticsConnectionEventArgs>connectionHandler=(eventArgs)=>{clientDict[eventArgs.RuntimeInstanceCookie]=(eventArgs.ProcessId,eventArgs.Client);Console.Write($"== New Connection: instanceCookie: {eventArgs.RuntimeInstanceCookie}, ProcessId: {eventArgs.ProcessId}\n> ");};using(DiagnosticsAgentagent=newDiagnosticsAgent(address,connectionHandler)){// ...}

[josalem]

CC @wiktork

[sywhang]

My suggestion was mostly based on whether the user would need to ever change it after creating the DiagnosticsAgent. It wasn't clear to me whether there would be a use case involving that, so I think it would be safer to first design it in a way that explicitly bans user from changing it.

If it is made public property, it is possible for a user to create a server socket and forget to create a handler, and never handle anything from the incoming connections. Making it an argument to the constructor makes it more clear for the user to create that handler with the server socket, which should be what they are doing with the public constructor regardless.

[jander-msft]

Will there be some mechanism by which multiple DiagnosticsClients can be established to the same process from one DiagnosticsAgent? I thinking that dotnet-monitory might need this if, for example, there is a active session for capturing logs, metrics, etc and then there is another request to capture dumps. I don't know if dotnet-monitor would be able to use the same connection concurrently for different collections of data.

[noahfalk]

I agree, I don't think a connection should include the DiagnosticClient. I'd suggest the callback arg/return value can be used to create an arbitrary number of DiagnosticClients. For example:

var agent = new DiagnosticsAgent(address);
var runtimeEndpoint = await agent.ListenForRuntimesAsync();
Console.WriteLine("Dotnet process detected with pid {runtimeEndpoint.Pid}");
// only listening to one for simplicity
while(true)
{
DotnetMonitorRequest r = await webserver.GetNextRequestAsync();
DiagnosticClient c = new DiagnosticClient(runtimeEndpoint);
Task.Run(() => HandleRequest(r,c));
}

[josalem]

I don't think there's anything preventing you from using the same DiagnosticsClient for multiple actions at once, even in the current implementation. You should already be able to start N<64 tracing sessions per process at once, and use the same DiagnosticsClient to capture dumps, gcdumps, etc. at the same time.

Is the suggestion that DiagnosticClients should be disposable, e.g., use once, constructs? Right now, DiagnosticClients abstract the connection process entirely for both traditional and reversed connections. Every time you issue a command for a traditional connection, it reconnects to the underlying transport. This new iteration, effectively does the same thing for reverse connections, but does so by caching the reverse connection each time the runtime advertises.

[noahfalk]

I don't think there's anything preventing you from using the same DiagnosticsClient for multiple actions at once, even in the current implementation

You could design it to work, but by convention .NET types are not considered thread-safe unless specifically documented otherwise. We could implement and document that DiagnosticsClient is thread-safe, but people will probably assume that it isn't by default. I'd recommend going with convention is easier than trying to explain that the API supports free-threaded use.

Is the suggestion that DiagnosticClients should be disposable, e.g., use once, constructs?

I think we should allow, but not require, that kind of usage.

[davidfowl]

Can we make this more of a listener/socket pull style API?

varagent=newDiagnosticsAgent(address);while(true){varconnection=awaitagent.AcceptAsync();_=ProcessConnectionAsync(connection);}

[josalem]

Would DiagnosticsAgent.AcceptAsync() return a connectionevery time a runtime instance connects to the agent or only the first time? The current model raises the OnDiagnosticsConnection event on only the first connection and users are expected to reuse the DiagnosticsClient (which abstracts subsequent reconnects) they get just like they use the traditional DiagnosticsClient today. if we change it to be "every time an instance connects" we may want to modify the DiagnosticsClient API for dispatching commands, or not use it entirely.

[noahfalk]

I'd suggest it is first time only. I am hoping to preserve the model that you get a DiagnosticClient and then you send an arbitrary number of commands without making the user aware of the underlying transport stream management.

[josalem]

This PR is no longer needed since #1303 went in.

[josalem]

The DiagnosticsAgent API can be used to create a Diagnostics IPC Protocol server. A working sample is provided in the updated documentation. New connections trigger an event that provides users with a DiagnosticsClient object that can be used the same as the current implementation.

N.B.: the implementation of this API will leverage dotnet/runtime#33307. I have done local testing with a version of this API and things worked well. Check out the dotnet-reverse tool in my hack branch. Note that the other tools will be broken in that branch and only dotnet-reverse builds.

CC - @tommcdon@davidfowl

[josalem]

@sywhang and I chatted offline about whether the OnDiagnosticsConnection event needs to be public. He suggested an alternative where it is private and the handler is passed in via the ctor, e.g.,

public DiagnosticsAgent(string ipcEndpointAddress, Action<DiagnosticsConnectionEventArgs> connectionHandler);

Action<DiagnosticsConnectionEventArgs>connectionHandler=(eventArgs)=>{clientDict[eventArgs.RuntimeInstanceCookie]=(eventArgs.ProcessId,eventArgs.Client);Console.Write($"== New Connection: instanceCookie: {eventArgs.RuntimeInstanceCookie}, ProcessId: {eventArgs.ProcessId}\n> ");};using(DiagnosticsAgentagent=newDiagnosticsAgent(address,connectionHandler)){// ...}

[josalem]

CC @wiktork

[sywhang]

My suggestion was mostly based on whether the user would need to ever change it after creating the DiagnosticsAgent. It wasn't clear to me whether there would be a use case involving that, so I think it would be safer to first design it in a way that explicitly bans user from changing it.

If it is made public property, it is possible for a user to create a server socket and forget to create a handler, and never handle anything from the incoming connections. Making it an argument to the constructor makes it more clear for the user to create that handler with the server socket, which should be what they are doing with the public constructor regardless.

[jander-msft]

Will there be some mechanism by which multiple DiagnosticsClients can be established to the same process from one DiagnosticsAgent? I thinking that dotnet-monitory might need this if, for example, there is a active session for capturing logs, metrics, etc and then there is another request to capture dumps. I don't know if dotnet-monitor would be able to use the same connection concurrently for different collections of data.

[noahfalk]

I agree, I don't think a connection should include the DiagnosticClient. I'd suggest the callback arg/return value can be used to create an arbitrary number of DiagnosticClients. For example:

var agent = new DiagnosticsAgent(address);
var runtimeEndpoint = await agent.ListenForRuntimesAsync();
Console.WriteLine("Dotnet process detected with pid {runtimeEndpoint.Pid}");
// only listening to one for simplicity
while(true)
{
DotnetMonitorRequest r = await webserver.GetNextRequestAsync();
DiagnosticClient c = new DiagnosticClient(runtimeEndpoint);
Task.Run(() => HandleRequest(r,c));
}

[josalem]

I don't think there's anything preventing you from using the same DiagnosticsClient for multiple actions at once, even in the current implementation. You should already be able to start N<64 tracing sessions per process at once, and use the same DiagnosticsClient to capture dumps, gcdumps, etc. at the same time.

Is the suggestion that DiagnosticClients should be disposable, e.g., use once, constructs? Right now, DiagnosticClients abstract the connection process entirely for both traditional and reversed connections. Every time you issue a command for a traditional connection, it reconnects to the underlying transport. This new iteration, effectively does the same thing for reverse connections, but does so by caching the reverse connection each time the runtime advertises.

[noahfalk]

I don't think there's anything preventing you from using the same DiagnosticsClient for multiple actions at once, even in the current implementation

You could design it to work, but by convention .NET types are not considered thread-safe unless specifically documented otherwise. We could implement and document that DiagnosticsClient is thread-safe, but people will probably assume that it isn't by default. I'd recommend going with convention is easier than trying to explain that the API supports free-threaded use.

Is the suggestion that DiagnosticClients should be disposable, e.g., use once, constructs?

I think we should allow, but not require, that kind of usage.

[davidfowl]

Can we make this more of a listener/socket pull style API?

varagent=newDiagnosticsAgent(address);while(true){varconnection=awaitagent.AcceptAsync();_=ProcessConnectionAsync(connection);}

[josalem]

Would DiagnosticsAgent.AcceptAsync() return a connectionevery time a runtime instance connects to the agent or only the first time? The current model raises the OnDiagnosticsConnection event on only the first connection and users are expected to reuse the DiagnosticsClient (which abstracts subsequent reconnects) they get just like they use the traditional DiagnosticsClient today. if we change it to be "every time an instance connects" we may want to modify the DiagnosticsClient API for dispatching commands, or not use it entirely.

[noahfalk]

I'd suggest it is first time only. I am hoping to preserve the model that you get a DiagnosticClient and then you send an arbitrary number of commands without making the user aware of the underlying transport stream management.

[josalem]

This PR is no longer needed since #1303 went in.

[josalem]

The DiagnosticsAgent API can be used to create a Diagnostics IPC Protocol server. A working sample is provided in the updated documentation. New connections trigger an event that provides users with a DiagnosticsClient object that can be used the same as the current implementation.

N.B.: the implementation of this API will leverage dotnet/runtime#33307. I have done local testing with a version of this API and things worked well. Check out the dotnet-reverse tool in my hack branch. Note that the other tools will be broken in that branch and only dotnet-reverse builds.

CC - @tommcdon@davidfowl

[josalem]

@sywhang and I chatted offline about whether the OnDiagnosticsConnection event needs to be public. He suggested an alternative where it is private and the handler is passed in via the ctor, e.g.,

public DiagnosticsAgent(string ipcEndpointAddress, Action<DiagnosticsConnectionEventArgs> connectionHandler);

Action<DiagnosticsConnectionEventArgs>connectionHandler=(eventArgs)=>{clientDict[eventArgs.RuntimeInstanceCookie]=(eventArgs.ProcessId,eventArgs.Client);Console.Write($"== New Connection: instanceCookie: {eventArgs.RuntimeInstanceCookie}, ProcessId: {eventArgs.ProcessId}\n> ");};using(DiagnosticsAgentagent=newDiagnosticsAgent(address,connectionHandler)){// ...}

[josalem]

CC @wiktork

[sywhang]

My suggestion was mostly based on whether the user would need to ever change it after creating the DiagnosticsAgent. It wasn't clear to me whether there would be a use case involving that, so I think it would be safer to first design it in a way that explicitly bans user from changing it.

If it is made public property, it is possible for a user to create a server socket and forget to create a handler, and never handle anything from the incoming connections. Making it an argument to the constructor makes it more clear for the user to create that handler with the server socket, which should be what they are doing with the public constructor regardless.

[jander-msft]

Will there be some mechanism by which multiple DiagnosticsClients can be established to the same process from one DiagnosticsAgent? I thinking that dotnet-monitory might need this if, for example, there is a active session for capturing logs, metrics, etc and then there is another request to capture dumps. I don't know if dotnet-monitor would be able to use the same connection concurrently for different collections of data.

[noahfalk]

I agree, I don't think a connection should include the DiagnosticClient. I'd suggest the callback arg/return value can be used to create an arbitrary number of DiagnosticClients. For example:

var agent = new DiagnosticsAgent(address);
var runtimeEndpoint = await agent.ListenForRuntimesAsync();
Console.WriteLine("Dotnet process detected with pid {runtimeEndpoint.Pid}");
// only listening to one for simplicity
while(true)
{
DotnetMonitorRequest r = await webserver.GetNextRequestAsync();
DiagnosticClient c = new DiagnosticClient(runtimeEndpoint);
Task.Run(() => HandleRequest(r,c));
}

[josalem]

I don't think there's anything preventing you from using the same DiagnosticsClient for multiple actions at once, even in the current implementation. You should already be able to start N<64 tracing sessions per process at once, and use the same DiagnosticsClient to capture dumps, gcdumps, etc. at the same time.

Is the suggestion that DiagnosticClients should be disposable, e.g., use once, constructs? Right now, DiagnosticClients abstract the connection process entirely for both traditional and reversed connections. Every time you issue a command for a traditional connection, it reconnects to the underlying transport. This new iteration, effectively does the same thing for reverse connections, but does so by caching the reverse connection each time the runtime advertises.

[noahfalk]

I don't think there's anything preventing you from using the same DiagnosticsClient for multiple actions at once, even in the current implementation

You could design it to work, but by convention .NET types are not considered thread-safe unless specifically documented otherwise. We could implement and document that DiagnosticsClient is thread-safe, but people will probably assume that it isn't by default. I'd recommend going with convention is easier than trying to explain that the API supports free-threaded use.

Is the suggestion that DiagnosticClients should be disposable, e.g., use once, constructs?

I think we should allow, but not require, that kind of usage.

[davidfowl]

Can we make this more of a listener/socket pull style API?

varagent=newDiagnosticsAgent(address);while(true){varconnection=awaitagent.AcceptAsync();_=ProcessConnectionAsync(connection);}

[josalem]

Would DiagnosticsAgent.AcceptAsync() return a connectionevery time a runtime instance connects to the agent or only the first time? The current model raises the OnDiagnosticsConnection event on only the first connection and users are expected to reuse the DiagnosticsClient (which abstracts subsequent reconnects) they get just like they use the traditional DiagnosticsClient today. if we change it to be "every time an instance connects" we may want to modify the DiagnosticsClient API for dispatching commands, or not use it entirely.

[noahfalk]

I'd suggest it is first time only. I am hoping to preserve the model that you get a DiagnosticClient and then you send an arbitrary number of commands without making the user aware of the underlying transport stream management.

[josalem]

This PR is no longer needed since #1303 went in.

[josalem]

The DiagnosticsAgent API can be used to create a Diagnostics IPC Protocol server. A working sample is provided in the updated documentation. New connections trigger an event that provides users with a DiagnosticsClient object that can be used the same as the current implementation.

N.B.: the implementation of this API will leverage dotnet/runtime#33307. I have done local testing with a version of this API and things worked well. Check out the dotnet-reverse tool in my hack branch. Note that the other tools will be broken in that branch and only dotnet-reverse builds.

CC - @tommcdon@davidfowl

[josalem]

@sywhang and I chatted offline about whether the OnDiagnosticsConnection event needs to be public. He suggested an alternative where it is private and the handler is passed in via the ctor, e.g.,

public DiagnosticsAgent(string ipcEndpointAddress, Action<DiagnosticsConnectionEventArgs> connectionHandler);

Action<DiagnosticsConnectionEventArgs>connectionHandler=(eventArgs)=>{clientDict[eventArgs.RuntimeInstanceCookie]=(eventArgs.ProcessId,eventArgs.Client);Console.Write($"== New Connection: instanceCookie: {eventArgs.RuntimeInstanceCookie}, ProcessId: {eventArgs.ProcessId}\n> ");};using(DiagnosticsAgentagent=newDiagnosticsAgent(address,connectionHandler)){// ...}

[josalem]

CC @wiktork

[sywhang]

My suggestion was mostly based on whether the user would need to ever change it after creating the DiagnosticsAgent. It wasn't clear to me whether there would be a use case involving that, so I think it would be safer to first design it in a way that explicitly bans user from changing it.

If it is made public property, it is possible for a user to create a server socket and forget to create a handler, and never handle anything from the incoming connections. Making it an argument to the constructor makes it more clear for the user to create that handler with the server socket, which should be what they are doing with the public constructor regardless.

[jander-msft]

Will there be some mechanism by which multiple DiagnosticsClients can be established to the same process from one DiagnosticsAgent? I thinking that dotnet-monitory might need this if, for example, there is a active session for capturing logs, metrics, etc and then there is another request to capture dumps. I don't know if dotnet-monitor would be able to use the same connection concurrently for different collections of data.

[noahfalk]

I agree, I don't think a connection should include the DiagnosticClient. I'd suggest the callback arg/return value can be used to create an arbitrary number of DiagnosticClients. For example:

var agent = new DiagnosticsAgent(address);
var runtimeEndpoint = await agent.ListenForRuntimesAsync();
Console.WriteLine("Dotnet process detected with pid {runtimeEndpoint.Pid}");
// only listening to one for simplicity
while(true)
{
DotnetMonitorRequest r = await webserver.GetNextRequestAsync();
DiagnosticClient c = new DiagnosticClient(runtimeEndpoint);
Task.Run(() => HandleRequest(r,c));
}

[josalem]

I don't think there's anything preventing you from using the same DiagnosticsClient for multiple actions at once, even in the current implementation. You should already be able to start N<64 tracing sessions per process at once, and use the same DiagnosticsClient to capture dumps, gcdumps, etc. at the same time.

Is the suggestion that DiagnosticClients should be disposable, e.g., use once, constructs? Right now, DiagnosticClients abstract the connection process entirely for both traditional and reversed connections. Every time you issue a command for a traditional connection, it reconnects to the underlying transport. This new iteration, effectively does the same thing for reverse connections, but does so by caching the reverse connection each time the runtime advertises.

[noahfalk]

I don't think there's anything preventing you from using the same DiagnosticsClient for multiple actions at once, even in the current implementation

You could design it to work, but by convention .NET types are not considered thread-safe unless specifically documented otherwise. We could implement and document that DiagnosticsClient is thread-safe, but people will probably assume that it isn't by default. I'd recommend going with convention is easier than trying to explain that the API supports free-threaded use.

Is the suggestion that DiagnosticClients should be disposable, e.g., use once, constructs?

I think we should allow, but not require, that kind of usage.

[davidfowl]

Can we make this more of a listener/socket pull style API?

varagent=newDiagnosticsAgent(address);while(true){varconnection=awaitagent.AcceptAsync();_=ProcessConnectionAsync(connection);}

[josalem]

Would DiagnosticsAgent.AcceptAsync() return a connectionevery time a runtime instance connects to the agent or only the first time? The current model raises the OnDiagnosticsConnection event on only the first connection and users are expected to reuse the DiagnosticsClient (which abstracts subsequent reconnects) they get just like they use the traditional DiagnosticsClient today. if we change it to be "every time an instance connects" we may want to modify the DiagnosticsClient API for dispatching commands, or not use it entirely.

[noahfalk]

I'd suggest it is first time only. I am hoping to preserve the model that you get a DiagnosticClient and then you send an arbitrary number of commands without making the user aware of the underlying transport stream management.

[josalem]

This PR is no longer needed since #1303 went in.

[josalem]

The DiagnosticsAgent API can be used to create a Diagnostics IPC Protocol server. A working sample is provided in the updated documentation. New connections trigger an event that provides users with a DiagnosticsClient object that can be used the same as the current implementation.

N.B.: the implementation of this API will leverage dotnet/runtime#33307. I have done local testing with a version of this API and things worked well. Check out the dotnet-reverse tool in my hack branch. Note that the other tools will be broken in that branch and only dotnet-reverse builds.

CC - @tommcdon@davidfowl

[josalem]

@sywhang and I chatted offline about whether the OnDiagnosticsConnection event needs to be public. He suggested an alternative where it is private and the handler is passed in via the ctor, e.g.,

public DiagnosticsAgent(string ipcEndpointAddress, Action<DiagnosticsConnectionEventArgs> connectionHandler);

Action<DiagnosticsConnectionEventArgs>connectionHandler=(eventArgs)=>{clientDict[eventArgs.RuntimeInstanceCookie]=(eventArgs.ProcessId,eventArgs.Client);Console.Write($"== New Connection: instanceCookie: {eventArgs.RuntimeInstanceCookie}, ProcessId: {eventArgs.ProcessId}\n> ");};using(DiagnosticsAgentagent=newDiagnosticsAgent(address,connectionHandler)){// ...}

[josalem]

CC @wiktork

[sywhang]

My suggestion was mostly based on whether the user would need to ever change it after creating the DiagnosticsAgent. It wasn't clear to me whether there would be a use case involving that, so I think it would be safer to first design it in a way that explicitly bans user from changing it.

If it is made public property, it is possible for a user to create a server socket and forget to create a handler, and never handle anything from the incoming connections. Making it an argument to the constructor makes it more clear for the user to create that handler with the server socket, which should be what they are doing with the public constructor regardless.

[jander-msft]

Will there be some mechanism by which multiple DiagnosticsClients can be established to the same process from one DiagnosticsAgent? I thinking that dotnet-monitory might need this if, for example, there is a active session for capturing logs, metrics, etc and then there is another request to capture dumps. I don't know if dotnet-monitor would be able to use the same connection concurrently for different collections of data.

[noahfalk]

I agree, I don't think a connection should include the DiagnosticClient. I'd suggest the callback arg/return value can be used to create an arbitrary number of DiagnosticClients. For example:

var agent = new DiagnosticsAgent(address);
var runtimeEndpoint = await agent.ListenForRuntimesAsync();
Console.WriteLine("Dotnet process detected with pid {runtimeEndpoint.Pid}");
// only listening to one for simplicity
while(true)
{
DotnetMonitorRequest r = await webserver.GetNextRequestAsync();
DiagnosticClient c = new DiagnosticClient(runtimeEndpoint);
Task.Run(() => HandleRequest(r,c));
}

[josalem]

I don't think there's anything preventing you from using the same DiagnosticsClient for multiple actions at once, even in the current implementation. You should already be able to start N<64 tracing sessions per process at once, and use the same DiagnosticsClient to capture dumps, gcdumps, etc. at the same time.

Is the suggestion that DiagnosticClients should be disposable, e.g., use once, constructs? Right now, DiagnosticClients abstract the connection process entirely for both traditional and reversed connections. Every time you issue a command for a traditional connection, it reconnects to the underlying transport. This new iteration, effectively does the same thing for reverse connections, but does so by caching the reverse connection each time the runtime advertises.

[noahfalk]

I don't think there's anything preventing you from using the same DiagnosticsClient for multiple actions at once, even in the current implementation

You could design it to work, but by convention .NET types are not considered thread-safe unless specifically documented otherwise. We could implement and document that DiagnosticsClient is thread-safe, but people will probably assume that it isn't by default. I'd recommend going with convention is easier than trying to explain that the API supports free-threaded use.

Is the suggestion that DiagnosticClients should be disposable, e.g., use once, constructs?

I think we should allow, but not require, that kind of usage.

[davidfowl]

Can we make this more of a listener/socket pull style API?

varagent=newDiagnosticsAgent(address);while(true){varconnection=awaitagent.AcceptAsync();_=ProcessConnectionAsync(connection);}

[josalem]

Would DiagnosticsAgent.AcceptAsync() return a connectionevery time a runtime instance connects to the agent or only the first time? The current model raises the OnDiagnosticsConnection event on only the first connection and users are expected to reuse the DiagnosticsClient (which abstracts subsequent reconnects) they get just like they use the traditional DiagnosticsClient today. if we change it to be "every time an instance connects" we may want to modify the DiagnosticsClient API for dispatching commands, or not use it entirely.

[noahfalk]

I'd suggest it is first time only. I am hoping to preserve the model that you get a DiagnosticClient and then you send an arbitrary number of commands without making the user aware of the underlying transport stream management.

[josalem]

This PR is no longer needed since #1303 went in.