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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a reference for new .NET 8 metrics #37213

Merged
merged 6 commits into from
Oct 16, 2023
Merged

Conversation

noahfalk
Copy link
Member

@noahfalk noahfalk commented Sep 22, 2023

Part of the work for #36748

This is part of new metrics reference material. If the patterns look good I can add the rest, but also happy to shuffle things around. For now I put all the metrics into files within the diagnostics folder, but I can move them anywhere area owners would like them to be. In the future I imagine adding more pages for runtime metrics, Microsoft.Extensions metrics, EF metrics, and potentially any other area of the runtime that adds metrics. The overview admittedly feels a little spartan for now.

@JamesNK @antonfirsov @tarekgh @samsp-msft


Internal previews

馃搫 File 馃敆 Preview link
docs/core/diagnostics/available-counters.md Well-known EventCounters in .NET
docs/core/diagnostics/built-in-metrics-aspnetcore.md ASP.NET Core Metrics
docs/core/diagnostics/built-in-metrics-system-net.md System.Net Metrics
docs/core/diagnostics/built-in-metrics.md Built-in Metrics in .NET
docs/core/diagnostics/compare-metric-apis.md docs/core/diagnostics/compare-metric-apis
docs/core/diagnostics/metrics-collection.md Collect metrics
docs/core/diagnostics/metrics.md Metrics Overview

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.

The doc structure and template look good to me. I left a couple of questions.

Copy link
Member

@JamesNK JamesNK left a comment

Choose a reason for hiding this comment

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

Overall, looks good. Needs a thorough edit pass.

docs/core/diagnostics/built-in-metrics-system-net.md Outdated Show resolved Hide resolved
docs/core/diagnostics/built-in-metrics-system-net.md Outdated Show resolved Hide resolved
docs/core/diagnostics/built-in-metrics-system-net.md Outdated Show resolved Hide resolved
docs/core/diagnostics/built-in-metrics-system-net.md Outdated Show resolved Hide resolved
docs/core/diagnostics/built-in-metrics-system-net.md Outdated Show resolved Hide resolved
@noahfalk noahfalk marked this pull request as ready for review October 9, 2023 07:44
@noahfalk noahfalk requested review from tommcdon and a team as code owners October 9, 2023 07:44
@noahfalk
Copy link
Member Author

noahfalk commented Oct 9, 2023

Thanks for feedback so far! I added the remaining ASP.NET metrics, updated for feedback, and hopefully caught some of the editing issues.

Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

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

Apologies for the late review. Blocking merge since there is one major inaccuracy.

docs/core/diagnostics/built-in-metrics-system-net.md Outdated Show resolved Hide resolved
docs/core/diagnostics/built-in-metrics-system-net.md Outdated Show resolved Hide resolved
Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

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

We need to sync this with the coming System.Net changes, probably easier to do it right in the PR.

@Rick-Anderson
Copy link
Contributor

Overall, looks good. Needs a thorough edit pass.

If the dotnet doc folks can't do this, @tdykstra and I can help. See open-telemetry/semantic-conventions#283 where @tdykstra and I have made massive edits.

@Rick-Anderson
Copy link
Contributor

@noahfalk throwing out this idea. It would be much more productive for you, me, and @tdykstra if you merged this PR and we had 24 hours to do an edit PR.

I'd rather PR this PR but I don't have permission.

@noahfalk noahfalk merged commit e91f03b into dotnet:main Oct 16, 2023
8 checks passed
@noahfalk
Copy link
Member Author

It would be much more productive for you, me, and @tdykstra if you merged this PR and we had 24 hours to do an edit PR.

Done :)

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.

None yet

7 participants