Skip to content
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

Allow tools to specify a specific OS transport #770

Closed
wants to merge 15 commits into from

Conversation

josalem
Copy link
Contributor

@josalem josalem commented Jan 22, 2020

Once dotnet/runtime#1600 is merged in, the runtime will be able to create the diagnostics server transport at a specific location. This change allows the tools to consume the path to a Unix Domain Socket or a specific Named Pipe.

Example: dotnet trace collect --transport-path /diagnostics/myapp.sock

The client library is also updated to allow for creating a DiagnosticsClient using a string (for path) instead of an int (for pid).

CC - @shirhatti @tommcdon

@josalem josalem added Microsoft.Diagnostics.NETCore.Client diagnostic global tooling containers related to running/installing/configuring Diagnostics in a container labels Jan 22, 2020
@josalem josalem added this to the 5.0 milestone Jan 22, 2020
@josalem josalem self-assigned this Jan 22, 2020
@josalem josalem added this to Needs Triage in .NET Core Diagnostics via automation Jan 22, 2020
.NET Core Diagnostics automation moved this from Needs Triage to In Progress Jan 22, 2020
Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little worried about the naming, but otherwise looks good : )

src/Tools/dotnet-counters/Program.cs Outdated Show resolved Hide resolved
src/Tools/dotnet-counters/Program.cs Outdated Show resolved Hide resolved
src/Tools/dotnet-dump/Dumper.cs Outdated Show resolved Hide resolved
src/Tools/dotnet-dump/Dumper.cs Outdated Show resolved Hide resolved
@shirhatti
Copy link
Contributor

@josalem I remember us having a hallway conversation about using URIs for the address/transportPath (e.g., net.pipe://foo_pipe, unix://etc/foo/foo.sock)

Is that no longer being considered?

@josalem
Copy link
Contributor Author

josalem commented Feb 20, 2020

Is that no longer being considered?

We just chatted about it offline, but I wanted to document it here. Making this a URI is an implementation detail that we can tackle later without changing the name/functionality added here. So, for now, I'm going to leave it out, but we can certainly add it if we ever add more transports.

@noahfalk
Copy link
Member

noahfalk commented Feb 20, 2020

Making this a URI is an implementation detail

<Noah being pedantic>
Because it would affect the user interaction I'd label it a design detail, not an implementation detail. However I agree that it is not a design choice we are precluded from enabling in the future and it doesn't appear critical to enable it now.
<\Noah being pedantic>

@josalem josalem requested a review from sywhang February 22, 2020 00:42
Copy link
Contributor

@sywhang sywhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@josalem
Copy link
Contributor Author

josalem commented Mar 19, 2020

I'm closing this as the design of this feature has pivoted (see: dotnet/runtime#33307) a bit during development and this code needs to change accordingly.

@josalem josalem closed this Mar 19, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Jan 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
containers related to running/installing/configuring Diagnostics in a container diagnostic global tooling Microsoft.Diagnostics.NETCore.Client
Projects
.NET Core Diagnostics
  
In Progress
Development

Successfully merging this pull request may close these issues.

None yet

5 participants