-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
Misc diagnostic doc cleanup #42359
Misc diagnostic doc cleanup #42359
Conversation
8156b69
to
8e16a88
Compare
I was recently reviewing the diagnostic docs and accumulated a variety of small fixes, improvements, and updates: - Added 'Meter' to the headings in built-in metrics references to clarify the names are Meter names and not just arbitrary descriptive names for some related metrics. - Added links to using the Meters to the built-in metrics pages that didn't have one. - Moved links for EventCounters and PerformanceCounters to the bottom of built-in-metrics.md to make it clearer these are different from the System.Diagnostics.Metrics API and to avoid being a distraction. - On the comparison pages for both metrics and logging I significantly reduced content about terminology and principles that is fairly generalized across all languages that have these features. I am assuming that most readers are either familiar with the basics already or they can learn it elsewhere. This reduces abstraction, allows the reader to get to the .NET specific APIs much quicker, and ensures that the top-most recommended API should be visible without needing to scroll down. - Updated references to out-of-date OpenTelemetry packages and removed usage of the obsolete StartWithHost() OTel API. - Updated some example snippets to remove nullability build warnings - Fixed a compilation error where a metrics snippet refered to a non-existant overload of Random.GetNext() - Updated the output of several examples to account for changes in OTel console exporter and dotnet-counters output - Updated some of the old examples to use newer SDK versions - Adjusted warnings about dotnet-trace bitness matching that appear to no longer be accurate. In the past there apparently was an issue calling Process.GetProcessById() and potentially other System.Diagnostic.Process functions when the application bitness running the code didn't match the bitness of the process being inspected. Testing now I didn't detect any issues running x86 dotnet-trace against a 64 bit target process or vice versa with one exception - x86 dotnet-trace doesn't correctly show the command lines of 64 bit apps in the ps command. - Removed some spurious warnings about using dotnet-trace with apps older than .NET 5. The doc already mentions .NET 5+ is supported so there seemed no need to keep repeating it. - Removed the dotnet-trace EventCounter collection example. The functionality should still work but this seems like an obscure scenario that would rarely be useful. I didn't want to give the impression this was a common technique we expect users to need or encourage them to do. Using dotnet-counters or dotnet-monitor would be a more natural and convenient approach to collect that data ad-hoc. - Added a note to EventCounters about the newer Metrics API to ensure readers are aware this older API is not the preferred one in most scenarios. - Added information about the Gauge instrument that was added during .NET 8. - Removed/adjusted some discussion about the dotnet-counters output format that is no longer accurate given recent changes to the tool's output behavior. - Adjusted the stack overflow debugging example to account for the stacktrace that is shown in all currently supported versions of the runtime.
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.
Modulo the comments from others, LGTM.
One generic issue I want to check for docs, does the term .NET
imply/include .NET Framework
too? I am asking if we have term that include .NET and .NET Framework same time.
.NET can mean different things depending on the context. See https://learn.microsoft.com/en-us/dotnet/standard/glossary#net. |
I'm not a fan of adding It's not a hill I'll die on. I'll acquiesce to whatever the consensus is. |
I had at least one person tell me they were and it did seem ambiguous when I thought about it. So I certainly wouldn't say I have a lot of feedback here, just a faint signal. Let me try to solicit more feedback about it. |
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
Co-authored-by: Genevieve Warren <24882762+gewarren@users.noreply.github.com>
Thanks for the feedback all! Based on what you said: @JamesNK - I haven't gotten any more feedback in either direction on the "Meter" title text so I backed that part out. If more feedback emerges in the future I may re-introduce it. @mikelle-rogers @tarekgh - I re-added the logging terminology at the bottom of the logging trace page and added links inline where the terms appear. I think it should work well. @gewarren - Applied all your suggestions as-is |
f670bc7
to
d33922b
Compare
d33922b
to
6b27b54
Compare
I was recently reviewing the diagnostic docs and accumulated a variety of small fixes, improvements, and updates:
PTAL - I tried to note areas different reviewers might care about the most but feedback on any portion is welcome.
@samsp-msft (everything)
@tarekgh (logging and metrics related changes)
@JamesNK (ASP.NET built-in metrics page)
@antonfirsov @MihaZupan (System.Net built-in metrics page)
@joperezr (Microsoft.Extensions.Diagnostics built-in metrics page)
@CodeBlanch (OTel example changes)
@dotnet/dotnet-diag (dotnet-trace, EventCounters, stack overflow tutorial)
Internal previews