Skip to content
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 metrics counter names #48536

Closed
JamesNK opened this issue May 31, 2023 · 2 comments · Fixed by #48375
Closed

Rename metrics counter names #48536

JamesNK opened this issue May 31, 2023 · 2 comments · Fixed by #48375
Labels
api-approved API was approved in API review, it can be implemented area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions

Comments

@JamesNK
Copy link
Member

JamesNK commented May 31, 2023

Background and Motivation

See #48309 (comment)

Proposed API

Microsoft.AspNetCore.Hosting:

  • current-requests -> http-server-current-requests
  • request-duration -> http-server-request-duration

Microsoft.AspNetCore.Http.Connections:

  • current-connections -> http-server-current-connections
  • connection-duration -> http-server-connection-duration
  • current-transports -> http-server-current-transports

http-server is probably the wrong name here. Maybe connection-handler?

Microsoft.AspNetCore.Server.Kestrel:

  • current-connections -> kestrel-current-connections
  • connection-duration -> kestrel-connection-duration
  • rejected-connections -> kestrel-rejected-connections
  • queued-connections -> kestrel-queued-connections
  • queued-requests -> kestrel-queued-requests
  • current-upgraded-connections -> kestrel-current-upgraded-connections
  • tls-handshake-duration -> kestrel-tls-handshake-duration
  • current-tls-handshakes -> kestrel-current-tls-handshakes

Microsoft.AspNetCore.RateLimiting:

  • current-leased-requests -> rate-limiting-current-leased-requests
  • leased-request-duration -> rate-limiting-leased-request-duration
  • current-queued-requests -> rate-limiting-current-queued-requests
  • queued-request-duration -> rate-limiting-queued-request-duration
  • lease-failed-requests -> rate-limiting-lease-failed-requests

Usage Examples

Alternative Designs

Risks

@JamesNK JamesNK added area-runtime api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels May 31, 2023
@ghost
Copy link

ghost commented May 31, 2023

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@halter73
Copy link
Member

halter73 commented Jun 8, 2023

API Review Notes:

  • We like the kestrel- and rate-limiting- prefixes.
    • limiting seems better than limiter because it aligns with the namespace and assembly.
  • As for http-server-, we like those for the hosting metrics since it's server agnostic. We don't like it for the definitely-not-signalr ;) counters.
    • How about signalr-http-transport- or http-connection-handler-?
    • "Http Connection Handler" seems too broad. One might think that Kestrel connection handlers are counted.
    • We will have a counter called signalr-http-transport-current-transports this way.
      • Can we get rid of -current-transports and add -pending-connections or -negotiating-connections?
      • Could we not have -negotiating-connections at all and only start -current-connections until a transport starts.
        • Yes. For now. We can add it later if needed

API Approved!

Microsoft.AspNetCore.Hosting:

  • current-requests -> http-server-current-requests
  • request-duration -> http-server-request-duration

Microsoft.AspNetCore.Http.Connections:

  • current-connections ->
  • connection-duration -> signalr-http-transport-connection-duration
  • current-transports -> signalr-http-transport-current-connections

Microsoft.AspNetCore.Server.Kestrel:

  • current-connections -> kestrel-current-connections
  • connection-duration -> kestrel-connection-duration
  • rejected-connections -> kestrel-rejected-connections
  • queued-connections -> kestrel-queued-connections
  • queued-requests -> kestrel-queued-requests
  • current-upgraded-connections -> kestrel-current-upgraded-connections
  • tls-handshake-duration -> kestrel-tls-handshake-duration
  • current-tls-handshakes -> kestrel-current-tls-handshakes

Microsoft.AspNetCore.RateLimiting:

  • current-leased-requests -> rate-limiting-current-leased-requests
  • leased-request-duration -> rate-limiting-leased-request-duration
  • current-queued-requests -> rate-limiting-current-queued-requests
  • queued-request-duration -> rate-limiting-queued-request-duration
  • lease-failed-requests -> rate-limiting-lease-failed-requests

@halter73 halter73 added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Jun 8, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jul 13, 2023
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants
@halter73 @JamesNK @amcasey and others