Skip to content

Add metrics to ASP.NET Core#46834

Merged
JamesNK merged 9 commits intomainfrom
jamesnk/metrics-prototype
Apr 12, 2023
Merged

Add metrics to ASP.NET Core#46834
JamesNK merged 9 commits intomainfrom
jamesnk/metrics-prototype

Conversation

@JamesNK
Copy link
Member

@JamesNK JamesNK commented Feb 23, 2023 •
edited
Loading

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
JamesNK
JamesNK commented Feb 23, 2023
View reviewed changes
JamesNK
JamesNK commented Feb 23, 2023
View reviewed changes
JamesNK
JamesNK commented Feb 23, 2023
View reviewed changes
JamesNK
JamesNK commented Feb 23, 2023
View reviewed changes
JamesNK
JamesNK commented Feb 23, 2023
View reviewed changes
JamesNK
JamesNK commented Feb 23, 2023
View reviewed changes
JamesNK
JamesNK commented Feb 23, 2023
View reviewed changes
samsp-msft
samsp-msft reviewed Feb 24, 2023
View reviewed changes
noahfalk
noahfalk reviewed Feb 24, 2023
View reviewed changes
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.

@JamesNK JamesNK force-pushed the jamesnk/metrics-prototype branch from edc69a9 to 45614d0 Compare March 13, 2023 02:06
@JamesNK JamesNK changed the title Metrics factory prototype Meter factory prototype Mar 13, 2023
@adityamandaleeka adityamandaleeka mentioned this pull request Mar 13, 2023
Closed
12 tasks
@danmoseley
Copy link
Member

danmoseley commented Mar 13, 2023

cc @tarekgh

davidfowl
davidfowl reviewed Mar 14, 2023
View reviewed changes
This was referenced Mar 14, 2023
Open
Closed
@JamesNK
Copy link
Member Author

JamesNK commented Mar 16, 2023

cc @Tratcher @noahfalk @davidfowl

@tarekgh
Copy link
Member

tarekgh commented Mar 17, 2023 •
edited
Loading

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

noahfalk
noahfalk reviewed Mar 17, 2023
View reviewed changes
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
Tratcher
Tratcher reviewed Mar 17, 2023
View reviewed changes
tarekgh
tarekgh reviewed Mar 21, 2023
View reviewed changes
tarekgh
tarekgh reviewed Mar 21, 2023
View reviewed changes
tarekgh
tarekgh reviewed Mar 21, 2023
View reviewed changes
JamesNK
JamesNK commented Apr 12, 2023
View reviewed changes
@JamesNK JamesNK enabled auto-merge (squash) April 12, 2023 03:32
@JamesNK JamesNK disabled auto-merge April 12, 2023 05:04
@build-analysis build-analysis bot mentioned this pull request Apr 12, 2023
Open
5 tasks
@JamesNK JamesNK merged commit 7d0c273 into main Apr 12, 2023
@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

davidfowl commented Apr 12, 2023

Did you run any tech empower benchmarks?

@amcasey amcasey mentioned this pull request Apr 12, 2023
Merged
This was referenced Apr 13, 2023
Merged
Closed
Closed
@JamesNK
Copy link
Member Author

JamesNK commented Apr 13, 2023

Did you run any tech empower benchmarks?

PlaintextTechEmpower microbenchmark.

@amcasey amcasey mentioned this pull request Apr 15, 2023
Closed
@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!

@noahfalk noahfalk mentioned this pull request May 6, 2023
Closed
@amcasey amcasey added area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
@leastprivilege leastprivilege mentioned this pull request Jun 27, 2023
Closed
xakep139
xakep139 reviewed Jul 3, 2023
View reviewed changes
@leastprivilege leastprivilege mentioned this pull request Jul 6, 2023
Closed
@justindbaur justindbaur mentioned this pull request Jul 29, 2023
Open
@vishweshbankwar vishweshbankwar mentioned this pull request Aug 23, 2023
Open
@sebastienros sebastienros mentioned this pull request Sep 19, 2023
Closed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@amcasey amcasey amcasey left review comments

@Tratcher Tratcher Tratcher approved these changes

@noahfalk noahfalk noahfalk left review comments

@BrennanConroy BrennanConroy BrennanConroy left review comments

@davidfowl davidfowl davidfowl left review comments

@tarekgh tarekgh tarekgh left review comments

@mitchdenny mitchdenny mitchdenny approved these changes

@MihaZupan MihaZupan Awaiting requested review from MihaZupan

@halter73 halter73 Awaiting requested review from halter73 halter73 is a code owner

@mgravell mgravell Awaiting requested review from mgravell

@javiercn javiercn Awaiting requested review from javiercn

@captainsafia captainsafia Awaiting requested review from captainsafia

+6 more reviewers

@xakep139 xakep139 xakep139 left review comments

@reyang reyang reyang approved these changes

@vishweshbankwar vishweshbankwar vishweshbankwar left review comments

@CodeBlanch CodeBlanch CodeBlanch left review comments

@dpk83 dpk83 dpk83 left review comments

@samsp-msft samsp-msft samsp-msft left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@JamesNK JamesNK

Labels

area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares 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

Milestone

8.0-preview4

Development

Successfully merging this pull request may close these issues.