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.

Sign up for GitHub

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

Jump to bottom

Add metrics to rate limiting #47758

Merged
merged 6 commits into from
May 3, 2023
Merged

Add metrics to rate limiting #47758

merged 6 commits into from
May 3, 2023

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

src/Middleware/RateLimiting/src/RateLimitingMetrics.cs Outdated Show resolved Hide resolved
src/Middleware/RateLimiting/src/RateLimiterApplicationBuilderExtensions.cs Show resolved Hide resolved
@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 middlesware 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

@captainsafia captainsafia captainsafia approved these changes

@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 mgravell is a code owner

Assignees
No one assigned
Labels
area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware 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