Skip to content

Rename ASP.NET Core metrics#48375

Merged
JamesNK merged 5 commits intomainfrom
jamesnk/counter-rename
Jun 13, 2023
Merged

Rename ASP.NET Core metrics#48375
JamesNK merged 5 commits intomainfrom
jamesnk/counter-rename

Conversation

@JamesNK
Copy link
Member

@JamesNK JamesNK commented May 23, 2023 •
edited
Loading

Fixes #48309
Fixes #48536

Renaming counters needs discussion in API review.

Summary of changes:

  • Microsoft.AspNetCore.Hosting counters are prefixed with http-server
  • Microsoft.AspNetCore.Http.Connections counters are prefixed with http-server
  • Microsoft.AspNetCore.Servers.Kestrel counters are prefixed with kestrel
  • Microsoft.AspNetCore.RateLimiting counters are prefixed with rate-limiting

Also, I'm unsure about Microsoft.AspNetCore.Http.Connections counters being prefixed by http-server. Are these counters used by anything other than SignalR? Should they include SignalR in the name instead of being very generic? @BrennanConroy

If the Microsoft.AspNetCore.Http.Connections counters are changed to not use the http-server prefix then we could consider making Microsoft.AspNetCore.Servers.Kestrel counters prefixed with http-server.

@JamesNK JamesNK added area-runtime area-signalr Includes: SignalR clients and servers labels May 23, 2023
@JamesNK JamesNK marked this pull request as ready for review May 23, 2023 02:14
@JamesNK JamesNK added the blocked The work on this issue is blocked due to some dependency label May 23, 2023
@JamesNK
Copy link
Member Author

JamesNK commented May 23, 2023

Blocked on API review.

@BrennanConroy
Copy link
Member

BrennanConroy commented May 30, 2023

Are these counters used by anything other than SignalR? Should they include SignalR in the name instead of being very generic?

They are for the ConnectionHandler layer. Not directly related to SignalR.
See https://github.com/dotnet/aspnetcore/tree/main/src/SignalR/samples/SocialWeather for an example, SignalR is not referenced at all.

Additionally, SignalR does not have a direct dependency on Http.Connections, it depends on the common abstractions from Connections.Abstractions that Http.Connections also relies on.

@JamesNK
Copy link
Member Author

JamesNK commented May 31, 2023

Alright. Do you have a better suggestion than http-server? It seems like this is about connection handling. The prefix could be connection-handler, but connection-handler-connection-duration feels a bit messy.

@JamesNK JamesNK force-pushed the jamesnk/counter-rename branch from e9f061d to 34b0166 Compare May 31, 2023 07:13
@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
@JamesNK JamesNK removed the blocked The work on this issue is blocked due to some dependency label Jun 9, 2023
@JamesNK JamesNK force-pushed the jamesnk/counter-rename branch from 34b0166 to f5dd136 Compare June 9, 2023 04:57
@JamesNK
Copy link
Member Author

JamesNK commented Jun 9, 2023

API review: #48536

BrennanConroy
BrennanConroy reviewed Jun 9, 2023
View reviewed changes
src/SignalR/common/Http.Connections/src/Internal/HttpConnectionsMetrics.cs Outdated
Copy link
Member

@BrennanConroy BrennanConroy Jun 9, 2023

Choose a reason for hiding this comment

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

I thought this was named signalr-http-transport-current-connections in API review.

Copy link
Member Author

@JamesNK JamesNK Jun 10, 2023

Choose a reason for hiding this comment

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

Yes.

I just sent an email to API review to recommend current-negotiated-connections as the name:

When making this change, I found the new name has a couple of problems:

  1. There's an existing current-connections event counter. The new proposed new signalr-http-transport-current-connections counter will start at a different time. That means the current-connections event counter and current-connections metric can have inconsistent values.
  2. The proposed signalr-http-transport-current-connections is incremented when the connection has negotiated the transport and ends when the connection ends. That's a different time interval measured by signalr-http-transport-connection-duration. Someone could be confused why the times don't match.

I propose to rename what is currently "current-connections":

  • current-connections -> current-negotiated-connections

Copy link
Member Author

@JamesNK JamesNK Jun 13, 2023

Choose a reason for hiding this comment

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

I'm renaming it back to signalr-http-transport-current-connections. I'll make changes to get this name to "work" in a follow-up PR.

@JamesNK JamesNK force-pushed the jamesnk/counter-rename branch from d661122 to 96d46e9 Compare June 10, 2023 09:44
@JamesNK JamesNK force-pushed the jamesnk/counter-rename branch from 5c58944 to c6fd3d3 Compare June 13, 2023 08:20
@JamesNK JamesNK mentioned this pull request Jun 13, 2023
Merged
@JamesNK JamesNK merged commit 9d34e67 into main Jun 13, 2023
@JamesNK JamesNK deleted the jamesnk/counter-rename branch June 13, 2023 14:24
@ghost ghost added this to the 8.0-preview6 milestone Jun 13, 2023
@vishweshbankwar vishweshbankwar mentioned this pull request May 13, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BrennanConroy BrennanConroy BrennanConroy left review comments

@Tratcher Tratcher Tratcher approved these changes

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

@mgravell mgravell Awaiting requested review from mgravell

Assignees

No one assigned

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 area-signalr Includes: SignalR clients and servers

Projects

None yet

Milestone

8.0-preview6

Development

Successfully merging this pull request may close these issues.

Rename metrics counter names The names of the metrics produced by the new meters do not seem clear

4 participants

@JamesNK @BrennanConroy @Tratcher @amcasey