Implement minimal RateLimitingMiddleware #41008
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wtgodbe
requested review from
a team,
Tratcher,
BrennanConroy,
dougbu and
Pilchie
as code owners
wtgodbe
commented
Apr 1, 2022
| _onRejected = options.Value.OnRejected; | ||
| } | ||
|
|
||
| // TODO - EventSource? |
There was a problem hiding this comment.
Could be valuable to add EventSource logging to this, part of the next pass?
There was a problem hiding this comment.
I'm fine with waiting for the next pass. It might be better to add EventSource logging to the rate limiter implemenations, but that might be too low level to be useful if there are many.
There was a problem hiding this comment.
Is there an issue tracking this yet?
Tratcher
reviewed
Apr 1, 2022
wtgodbe
commented
Apr 1, 2022
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
halter73
reviewed
Apr 15, 2022
pranavkm
reviewed
Apr 15, 2022
src/Middleware/RateLimiting/src/Microsoft.AspNetCore.RateLimiting.csproj
Outdated
Show resolved
Hide resolved
src/Middleware/RateLimiting/src/Microsoft.AspNetCore.RateLimiting.csproj
Show resolved
Hide resolved
src/Middleware/RateLimiting/test/RateLimitingMiddlewareTests.cs
Outdated
Show resolved
Hide resolved
|
@BrennanConroy @Tratcher @halter73 @davidfowl updated to reflect feedback from API review, and added a couple tests. After changing Going to add a sample though that might not make it for preview4 |
wtgodbe
commented
Apr 19, 2022
halter73
reviewed
Apr 19, 2022
src/Middleware/RateLimiting/src/RateLimitingApplicationBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
src/Middleware/RateLimiting/test/RateLimitingMiddlewareTests.cs
Outdated
Show resolved
Hide resolved
Tratcher
reviewed
Apr 19, 2022
Tratcher
reviewed
Apr 19, 2022
src/Middleware/RateLimiting/src/RateLimitingApplicationBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
halter73
approved these changes
Apr 19, 2022
Tratcher
approved these changes
Apr 19, 2022
BrennanConroy
approved these changes
Apr 19, 2022
ghost
added this to the 
amcasey
added
area-middleware
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Bare bones implementation of #37384. The only scenario currently supported is the user giving us a custom configured
PartitionedRateLimiter<HttpContext>, which we let do its thing. No endpoint awareness or piecemeal limiter construction is supported yet, that will be in the next pass (Right now if the user wants endpoint awareness, they must build it in to their PartitionedRateLimiter implementation). I have another branch with skeletons for that stuff (which I've deleted from this one) here: https://github.com/wtgodbe/aspnetcore/tree/wtgodbe/RateLimitMid.Also still need to work out how we support net6 & net7 with this, which will come in the next pass.
Also todo - add a sample