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

Add metrics to ASP.NET Core #46834

Merged
merged 9 commits into from Apr 12, 2023
Merged

Add metrics to ASP.NET Core #46834

merged 9 commits into from Apr 12, 2023

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Feb 23, 2023

This PR contains several changes:

  • Prototype for System.Diagnostics.Metrics integration with DI (plan is for a new Microsoft.Extensions.Metrics package)
    • This is providing value by making metrics testable but isn't publically available. The temporary workaround is to put types in ASP.NET Core, make them internal, then use InternalsVisibleTo. An alternative idea could be to put these types in a new assembly and keep it internal by not publishing ref pack, but that's more work, and this is just a temporary message.
    • The DI API surface might change, but I expect reacting to the public API won't be a big task.
  • Adding metrics to ASP.NET Core. This means adding metrics to places that have event counters today. There is an issue that specifies all the counters added in ASP.NET Core here: ASP.NET Core metrics #47536

@JamesNK JamesNK added the * NO MERGE * Do not merge this PR as long as this label is present. label Feb 23, 2023
@ghost ghost added the area-runtime label Feb 23, 2023
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.

Some hopefully useful comments inline. I was more paying attention to the overall patterns and not giving too much thought to what particular metrics/dimensions ASP.NET should have at this point.

src/Hosting/Hosting/src/Internal/HostingEventSource.cs Outdated Show resolved Hide resolved
src/Hosting/Hosting/src/Internal/HostingEventSource.cs Outdated Show resolved Hide resolved
src/Hosting/Hosting/src/Internal/HostingEventSource.cs Outdated Show resolved Hide resolved
src/Hosting/Hosting/test/HostingApplicationTests.cs Outdated Show resolved Hide resolved
src/Hosting/Hosting/test/HostingApplicationTests.cs Outdated Show resolved Hide resolved
src/Hosting/Hosting/test/HostingApplicationTests.cs Outdated Show resolved Hide resolved
src/Hosting/Hosting/src/Internal/HostingMetrics.cs Outdated Show resolved Hide resolved
@JamesNK JamesNK changed the title Metrics factory prototype Meter factory prototype Mar 13, 2023
@danmoseley
Copy link
Member

cc @tarekgh

@JamesNK
Copy link
Member Author

JamesNK commented Mar 16, 2023

cc @Tratcher @noahfalk @davidfowl

@tarekgh
Copy link
Member

tarekgh commented Mar 17, 2023

I am planning to look at this PR later. Just didn't get a chance yet :-)

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.

Coming together 👍 Probably my biggest open question at this point is what guidance/helpers we can offer to make testing as simple as possible.

@Tratcher Tratcher requested a review from MihaZupan March 17, 2023 17:51
@JamesNK JamesNK enabled auto-merge (squash) April 12, 2023 03:32
@JamesNK JamesNK disabled auto-merge April 12, 2023 05:04
@JamesNK JamesNK merged commit 7d0c273 into main Apr 12, 2023
26 checks passed
@JamesNK JamesNK deleted the jamesnk/metrics-prototype branch April 12, 2023 07:34
@ghost ghost added this to the 8.0-preview4 milestone Apr 12, 2023
@davidfowl
Copy link
Member

Did you run any tech empower benchmarks?

@JamesNK
Copy link
Member Author

JamesNK commented Apr 13, 2023

Did you run any tech empower benchmarks?

PlaintextTechEmpower microbenchmark.

@adityamandaleeka adityamandaleeka added the blog-candidate Consider mentioning this in the release blog post label May 5, 2023
@ghost
Copy link

ghost commented May 5, 2023

@JamesNK, this change will be considered for inclusion in the blog post for the release it'll ship in. Nice work!

Please ensure that the original comment in this thread contains a clear explanation of what the change does, why it's important (what problem does it solve?), and, if relevant, include things like code samples and/or performance numbers.

This content may not be exactly what goes into the blog post, but it will help the team putting together the announcement.

Thanks!

@amcasey amcasey added area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions blog-candidate Consider mentioning this in the release blog post
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet