-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Rename ASP.NET Core metrics #48375
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
Rename ASP.NET Core metrics #48375
Conversation
|
Blocked on API review. |
They are for the Additionally, SignalR does not have a direct dependency on Http.Connections, it depends on the common abstractions from |
|
Alright. Do you have a better suggestion than |
e9f061d to
34b0166
Compare
34b0166 to
f5dd136
Compare
|
API review: #48536 |
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.
I thought this was named signalr-http-transport-current-connections in API review.
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.
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:
- 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.
- 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
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.
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.
d661122 to
96d46e9
Compare
5c58944 to
c6fd3d3
Compare
Fixes #48309
Fixes #48536
Renaming counters needs discussion in API review.
Summary of changes:
Microsoft.AspNetCore.Hostingcounters are prefixed withhttp-serverMicrosoft.AspNetCore.Http.Connectionscounters are prefixed withhttp-serverMicrosoft.AspNetCore.Servers.Kestrelcounters are prefixed withkestrelMicrosoft.AspNetCore.RateLimitingcounters are prefixed withrate-limitingAlso, I'm unsure about
Microsoft.AspNetCore.Http.Connectionscounters being prefixed byhttp-server. Are these counters used by anything other than SignalR? Should they include SignalR in the name instead of being very generic? @BrennanConroyIf the
Microsoft.AspNetCore.Http.Connectionscounters are changed to not use thehttp-serverprefix then we could consider makingMicrosoft.AspNetCore.Servers.Kestrelcounters prefixed withhttp-server.