Skip to content

Add IRateLimiterStatisticsFeature with default implementation and tests#46028

Draft
MadL1me wants to merge 8 commits intodotnet:mainfrom
MadL1me:MadL1me/features/rate-limiter-statistics
Draft

Add IRateLimiterStatisticsFeature with default implementation and tests#46028
MadL1me wants to merge 8 commits intodotnet:mainfrom
MadL1me:MadL1me/features/rate-limiter-statistics

Conversation

@MadL1me
Copy link
Copy Markdown
Contributor

@MadL1me MadL1me commented Jan 11, 2023

Add IRateLimiterStatisticsFeature with default implementation and tests

This PR implements first half of this proposed API for RateLimiterMiddleware. Relates to: #44140.

Description

Based on 1 of 2 parts from this API proposal: #45658.

In this PR:

  • Created IRateLimiterStatisticsFeature with XML documentation
  • Added default implementation for that interface
  • Added option to turn on/off statistics tracking via RateLimiterOptions.TrackStatistics
  • Added unit tests for cases:
    • Statistics are tracked
    • Statistics are not tracked
    • Statistics are got from endpoint/global limiters successfully

@ghost ghost added area-runtime community-contribution Indicates that the PR has been added by a community member labels Jan 11, 2023
@ghost
Copy link
Copy Markdown

ghost commented Jan 11, 2023

Thanks for your PR, @MadL1me. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

This was referenced Jan 11, 2023
Closed
Open
BrennanConroy
BrennanConroy reviewed Jan 13, 2023
View reviewed changes
src/Middleware/RateLimiting/src/RateLimitingMiddleware.cs

private void AddRateLimiterStatisticsFeature(HttpContext context)
{
context.Features.Set<IRateLimiterStatisticsFeature>(new DefaultRateLimiterStatisticsFeature(_globalLimiter, _endpointLimiter, context));
Copy link
Copy Markdown
Member

@BrennanConroy BrennanConroy Jan 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It looks like we can allocate this once and reuse it

BrennanConroy
BrennanConroy reviewed Jan 13, 2023
View reviewed changes
src/Middleware/RateLimiting/src/RateLimitingMiddleware.cs Outdated

private void AddRateLimiterStatisticsFeature(HttpContext context)
{
_statisticsFeature?.SetHttpContext(context);
Copy link
Copy Markdown
Member

@BrennanConroy BrennanConroy Jan 13, 2023

Choose a reason for hiding this comment

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

Oh, this stores the HttpContext, we need to allocate it every time then. You can't reuse it because parallel requests will end up accessing someone else's HttpContext.

Copy link
Copy Markdown
Member

@BrennanConroy BrennanConroy Jan 13, 2023

Choose a reason for hiding this comment

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

Sorry for the extra work, my bad 😄

@build-analysis build-analysis bot mentioned this pull request Jan 15, 2023
Open
5 tasks
halter73
halter73 reviewed Jan 19, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

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

I think this looks really good so far. We just need to finish discussion in #45658 and nail down the final API. We should do that later today. Thanks for your patience.

@halter73
Copy link
Copy Markdown
Member

halter73 commented Jan 20, 2023

We're still working on getting the right API for this. We will revisit the API again on Monday.

@MadL1me
Copy link
Copy Markdown
Contributor Author

MadL1me commented Jan 20, 2023

@halter73 no problem, take as much time as you need - its better to ship well designed API than poorly designed, and I appreciate your time and work for this

@ghost ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 2, 2023
@amcasey amcasey added area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares and removed area-runtime labels Jun 2, 2023
@wtgodbe wtgodbe removed the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Feb 13, 2024
@MadL1me
Copy link
Copy Markdown
Contributor Author

MadL1me commented Feb 13, 2024

@dotnet-policy-service agree

@dotnet-policy-service
Copy link
Copy Markdown
Contributor

dotnet-policy-service bot commented Feb 20, 2024

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no conflicting changes have occurred, please rerun validation before merging. You can do this by leaving an /azp run comment here (requires commit rights), or by simply closing and reopening.

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 20, 2024
@BrennanConroy
Copy link
Copy Markdown
Member

BrennanConroy commented Mar 5, 2024

@MadL1me can you change this PR to a Draft PR for now? Hopefully we can figure something out for this soon.

@MadL1me MadL1me marked this pull request as draft March 11, 2024 16:24
@MadL1me
Copy link
Copy Markdown
Contributor Author

MadL1me commented Mar 11, 2024

Yeah, sure

This was referenced Apr 4, 2026
Closed
Open
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Open
Closed
Closed
Open
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@halter73 halter73 halter73 left review comments

@Tratcher Tratcher Tratcher left review comments

@BrennanConroy BrennanConroy BrennanConroy left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@BrennanConroy BrennanConroy

Labels

area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares community-contribution Indicates that the PR has been added by a community member pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@MadL1me @halter73 @BrennanConroy @Tratcher @adityamandaleeka @amcasey @wtgodbe