Skip to content

Add metrics to rate limiting#47758

Merged
JamesNK merged 6 commits intomainfrom
jamesnk/ratelimiting-metrics
May 3, 2023
Merged

Add metrics to rate limiting#47758
JamesNK merged 6 commits intomainfrom
jamesnk/ratelimiting-metrics

Conversation

@JamesNK
Copy link
Member

@JamesNK JamesNK commented Apr 18, 2023

Adds metrics counters to rate limiting. See #47745 for details about the counters.

@JamesNK JamesNK mentioned this pull request Apr 18, 2023
Closed
12 tasks
captainsafia
captainsafia reviewed Apr 20, 2023
View reviewed changes
@JamesNK JamesNK added the blocked The work on this issue is blocked due to some dependency label Apr 25, 2023
@JamesNK
Copy link
Member Author

JamesNK commented Apr 25, 2023

Blocked on API review of #47745

@JamesNK JamesNK removed the blocked The work on this issue is blocked due to some dependency label Apr 29, 2023
halter73
halter73 reviewed May 1, 2023
View reviewed changes
halter73
halter73 approved these changes May 3, 2023
View reviewed changes
Copy link
Member

@halter73 halter73 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Are we going to update Kestrel, Hosting and SignalR current- metrics to use a MetricsContext like this in a follow up PR?

As an aside, could we wait until after a transport is selected to start SignalR's "current-connections" counter and remove "current-transports"? Do we really need to know how many current connections are in a pre-transport state? If so, maybe we could add a "current-negotiating-connections" or "current-pending-connections" counter. I know this went through API review, but this is my first time looking closely at it. @BrennanConroy

@JamesNK @halter73
Update src/Middleware/RateLimiting/src/RateLimitingMetrics.cs
d4c1ba3 
Co-authored-by: Stephen Halter <halter73@gmail.com>
@JamesNK
Copy link
Member Author

JamesNK commented May 3, 2023

Breaking change announcement: aspnet/Announcements#506

@JamesNK JamesNK enabled auto-merge (squash) May 3, 2023 01:49
@JamesNK JamesNK merged commit d78d2a0 into main May 3, 2023
@JamesNK JamesNK deleted the jamesnk/ratelimiting-metrics branch May 3, 2023 02:54
@ghost ghost added this to the 8.0-preview5 milestone May 3, 2023
@JamesNK
Copy link
Member Author

JamesNK commented May 3, 2023

As an aside, could we wait until after a transport is selected to start SignalR's "current-connections" counter and remove "current-transports"? Do we really need to know how many current connections are in a pre-transport state? If so, maybe we could add a "current-negotiating-connections" or "current-pending-connections" counter. I know this went through API review, but this is my first time looking closely at it. @BrennanConroy

We decided to start the SignalR connection counter at its current point because that's when memory is allocated for it on the server. Splitting it up into multiple counters didn't feel that valuable.

@JamesNK JamesNK mentioned this pull request May 3, 2023
Merged
@JamesNK JamesNK mentioned this pull request May 31, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@halter73 halter73 halter73 approved these changes

@Tratcher Tratcher Awaiting requested review from Tratcher

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

@mgravell mgravell Awaiting requested review from mgravell

+1 more reviewer

@captainsafia captainsafia captainsafia approved these changes

Reviewers whose approvals may not affect merge requirements

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

Projects

None yet

Milestone

8.0-preview5

Development

Successfully merging this pull request may close these issues.

5 participants

@JamesNK @halter73 @Tratcher @captainsafia @amcasey