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 IRateLimiterStatisticsFeature with default implementation and tests #46028

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

Add IRateLimiterStatisticsFeature with default implementation and tests #46028

wants to merge 8 commits into from

Conversation

MadL1me
Copy link
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

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/Features/IRateLimiterStatisticsFeature.cs Outdated Show resolved Hide resolved
src/Middleware/RateLimiting/src/Features/DefaultRateLimiterStatisticsFeature.cs Outdated Show resolved Hide resolved
src/Middleware/RateLimiting/src/Features/IRateLimiterStatisticsFeature.cs Outdated Show resolved Hide resolved
src/Middleware/RateLimiting/src/Features/IRateLimiterStatisticsFeature.cs Outdated Show resolved Hide resolved
src/Middleware/RateLimiting/src/Features/IRateLimiterStatisticsFeature.cs Outdated Show resolved Hide resolved
src/Middleware/RateLimiting/src/Features/IRateLimiterStatisticsFeature.cs Outdated Show resolved Hide resolved
src/Middleware/RateLimiting/src/Features/IRateLimiterStatisticsFeature.cs Outdated Show resolved Hide resolved
src/Middleware/RateLimiting/src/RateLimiterOptions.cs Outdated Show resolved Hide resolved
src/Middleware/RateLimiting/src/RateLimiterOptions.cs Outdated Show resolved Hide resolved
src/Middleware/RateLimiting/src/RateLimitingMiddleware.cs
@@ -242,6 +251,11 @@ private PartitionedRateLimiter<HttpContext> CreateEndpointLimiter()
}, new DefaultKeyTypeEqualityComparer());
}

private void AddRateLimiterStatisticsFeature(HttpContext context)
{
context.Features.Set<IRateLimiterStatisticsFeature>(new DefaultRateLimiterStatisticsFeature(_globalLimiter, _endpointLimiter, context));
Copy link
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
@@ -242,6 +255,12 @@ private PartitionedRateLimiter<HttpContext> CreateEndpointLimiter()
}, new DefaultKeyTypeEqualityComparer());
}

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

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
Member

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

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

@MadL1me
Copy link
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 middlesware and removed area-runtime labels Jun 2, 2023
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 13, 2024
@dotnet dotnet deleted a comment Feb 13, 2024
@dotnet-policy-service dotnet-policy-service bot added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Feb 13, 2024
@wtgodbe wtgodbe removed the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Feb 13, 2024
@MadL1me
Copy link
Contributor Author

MadL1me commented Feb 13, 2024

@dotnet-policy-service agree

@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

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
Member

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

MadL1me commented Mar 11, 2024

Yeah, sure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BrennanConroy BrennanConroy BrennanConroy left review comments

@Tratcher Tratcher Tratcher left review comments

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