Skip to content

Conversation

noahfalk
Copy link
Member

Fixes dotnet/diagnostics#2900

We added the diagnostic port in 3.1 and expanded on it in 5.0, but never
documented it aside from tangential references in some tools that used it.
This change describes it directly and adds some detail that wasn't mentioned
elsewhere. I also tried to improve docs on some related environment variables
that misleadingly claimed scenarios were only for dotnet-monitor or only for
Mono.

Fixes dotnet/diagnostics#2900

We added the diagnostic port in 3.1 and expanded on it in 5.0, but never
documented it aside from tangential references in some tools that used it.
This change describes it directly and adds some detail that wasn't mentioned
elsewhere. I also tried to improve docs on some related environment variables
that misleadingly claimed scenarios were only for dotnet-monitor or only for
Mono.
@noahfalk
Copy link
Member Author

I'm still need to clean up the lint stuff and build warnings, but it should be close enough to do a decent review
@lateralusX - Can you check if the new content is saying the right things about Mono? Feedback on anything else is welcome too of course.
@dotnet/dotnet-diag

@noahfalk
Copy link
Member Author

Thanks @lateralusX 👍 I think I had a little tunnel vision associating Mono with Android/iOS/tvOS. I pivoted the description to focus on the operating system and hopefully I've got Mono included in all the right places now.

@noahfalk noahfalk marked this pull request as ready for review April 14, 2022 07:23
@noahfalk noahfalk requested review from tdykstra, tommcdon and a team as code owners April 14, 2022 07:23
Copy link
Contributor

@tdykstra tdykstra left a comment

Choose a reason for hiding this comment

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

Looks good, just some grammar and style suggestions.

Copy link
Contributor

@josalem josalem left a comment

Choose a reason for hiding this comment

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

LGTM sans a couple nits I commented on 👍

noahfalk and others added 2 commits April 14, 2022 23:22
Co-authored-by: Tom Dykstra <tdykstra@microsoft.com>
Co-authored-by: John Salem <josalem@microsoft.com>
@noahfalk
Copy link
Member Author

Thanks all!
@josalem - two of your comments I was unsure about, otherwise I think this is ready to go.

@noahfalk
Copy link
Member Author

@josalem - got it and yeah I can see the confusion. I'm going to guess that even if people are confused and thought diagnostic port was used to implement debugging they would still conclude that debugging is effected. If you have any thoughts on a better way to say it we could always do a tiny PR that adjusts it : ) Thanks!

@noahfalk noahfalk merged commit cf7c94d into dotnet:main Apr 15, 2022
IEvangelist pushed a commit to IEvangelist/docs that referenced this pull request Apr 21, 2022
* Document Diagnostic Port

Fixes dotnet/diagnostics#2900

We added the diagnostic port in 3.1 and expanded on it in 5.0, but never
documented it aside from tangential references in some tools that used it.
This change describes it directly and adds some detail that wasn't mentioned
elsewhere. I also tried to improve docs on some related environment variables
that misleadingly claimed scenarios were only for dotnet-monitor or only for
Mono.

* Apply suggestions from code review

Co-authored-by: Tom Dykstra <tdykstra@microsoft.com>
Co-authored-by: John Salem <josalem@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Diagnostic Port and Microsoft.Diagnostics.NetCore.Client
5 participants