-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
Add metrics to ASP.NET Core #46834
Conversation
src/Hosting/Abstractions/src/MetricsPrototype/DefaultMetricsFactory.cs
Outdated
Show resolved
Hide resolved
src/Hosting/Abstractions/src/MetricsPrototype/IMetricsFactory.cs
Outdated
Show resolved
Hide resolved
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.
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/Abstractions/src/MetricsPrototype/IMetricsFactory.cs
Outdated
Show resolved
Hide resolved
src/Hosting/Abstractions/src/MetricsPrototype/IMetricsFactory.cs
Outdated
Show resolved
Hide resolved
edc69a9
to
45614d0
Compare
cc @tarekgh |
I am planning to look at this PR later. Just didn't get a chance yet :-) |
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.
Coming together 👍 Probably my biggest open question at this point is what guidance/helpers we can offer to make testing as simple as possible.
src/Hosting/Abstractions/src/MetricsPrototype/MeterServiceExtensions.cs
Outdated
Show resolved
Hide resolved
src/Hosting/Abstractions/src/MetricsPrototype/MeterServiceExtensions.cs
Outdated
Show resolved
Hide resolved
src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs
Outdated
Show resolved
Hide resolved
src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs
Outdated
Show resolved
Hide resolved
src/Hosting/Abstractions/src/MetricsPrototype/DefaultMeterFactory.cs
Outdated
Show resolved
Hide resolved
Did you run any tech empower benchmarks? |
PlaintextTechEmpower microbenchmark. |
@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! |
This PR contains several changes:
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.IHttpMetricsTagsFeature
. API issue: Add IHttpMetricsTagsFeature and IConnectionMetricsTagsFeature #47493IRouteDiagnosticsMetadata
. API issue: Add IRouteDiagnosticsMetadata #47492IConnectionMetricsTagsFeature
. API issue: Add IHttpMetricsTagsFeature and IConnectionMetricsTagsFeature #47493