- 
                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.