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

Misc diagnostic doc cleanup #42359

Merged
merged 3 commits into from
Sep 4, 2024
Merged

Misc diagnostic doc cleanup #42359

merged 3 commits into from
Sep 4, 2024

Conversation

noahfalk
Copy link
Member

@noahfalk noahfalk commented Aug 28, 2024

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.

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

📄 File 🔗 Preview link
docs/core/diagnostics/built-in-metrics-diagnostics.md docs/core/diagnostics/built-in-metrics-diagnostics
docs/core/diagnostics/built-in-metrics-runtime.md docs/core/diagnostics/built-in-metrics-runtime
docs/core/diagnostics/built-in-metrics.md docs/core/diagnostics/built-in-metrics
docs/core/diagnostics/compare-metric-apis.md docs/core/diagnostics/compare-metric-apis
docs/core/diagnostics/debug-stackoverflow.md docs/core/diagnostics/debug-stackoverflow
docs/core/diagnostics/distributed-tracing-collection-walkthroughs.md docs/core/diagnostics/distributed-tracing-collection-walkthroughs
docs/core/diagnostics/dotnet-trace.md docs/core/diagnostics/dotnet-trace
docs/core/diagnostics/event-counters.md docs/core/diagnostics/event-counters
docs/core/diagnostics/logging-tracing.md docs/core/diagnostics/logging-tracing
docs/core/diagnostics/metrics-instrumentation.md docs/core/diagnostics/metrics-instrumentation

@noahfalk noahfalk requested review from tommcdon and a team as code owners August 28, 2024 04:42
@dotnet-bot dotnet-bot added this to the August 2024 milestone Aug 28, 2024
@dotnet-policy-service dotnet-policy-service bot added the okr-quality Content-quality KR: Concerns article defects (bugs), freshness, or build warnings. label Aug 28, 2024
@noahfalk noahfalk force-pushed the doc_updates branch 2 times, most recently from 8156b69 to 8e16a88 Compare August 28, 2024 04:57
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.
docs/core/diagnostics/logging-tracing.md Outdated Show resolved Hide resolved
docs/core/diagnostics/metrics-instrumentation.md Outdated Show resolved Hide resolved
Copy link
Member

@tarekgh tarekgh left a 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.

@gewarren
Copy link
Contributor

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.

@JamesNK
Copy link
Member

JamesNK commented Aug 29, 2024

I'm not a fan of adding Meter to the titles. It makes the content feel messier. Were people confused before?

It's not a hill I'll die on. I'll acquiesce to whatever the consensus is.

@noahfalk
Copy link
Member Author

Were people confused before?

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.

Copy link

@CodeBlanch CodeBlanch left a 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>
@noahfalk
Copy link
Member Author

noahfalk commented Sep 4, 2024

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

@noahfalk noahfalk force-pushed the doc_updates branch 2 times, most recently from f670bc7 to d33922b Compare September 4, 2024 04:10
@noahfalk noahfalk merged commit e5b0850 into dotnet:main Sep 4, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dotnet-fundamentals/svc okr-quality Content-quality KR: Concerns article defects (bugs), freshness, or build warnings.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants