Rate limiter context HttpContext feature API proposal

[MadL1me]

Background and Motivation

In https://github.com/dotnet/aspnetcore/issues/44140 issue, one of The dream scenario for a better ASP.NET Core rate limiter middleware as @maartenba says, would be able Have a feature on the current HttpContext that gives access to the current rate limit context, so these details can also be returned on successful requests, or added to telemetry.. This API proposal addresses this concern.

Proposed API

Below API supposes, that we have RateLimiterMiddleware in our app, which responsibility is to create IRateLimiterContextFeature for each request:

var app = builder.Build();

app.UseRateLimiter();

Now we can access to this interface in any custom middleware or controller:

namespace Microsoft.AspNetCore.RateLimiting.Features;

public interface IRateLimiterContextFeature
{
     HttpContext HttpContext { get; set; }

     RateLimitLease Lease { get; set; } 

     PartitionedRateLimiter<HttpContext>? GlobalLimiter { get; set; }

     PartitionedRateLimiter<HttpContext> EndpointLimiter { get; set; }
}

Usage Examples

Scenario 1: get statistics from limiters in custom telemetry middleware:

public Task Invoke(HttpContext context)
{
    var rlContext = context.Features.Get<IRateLimiterContextFeature>();

    var globalStatistics = rlContext.GlobalLimiter.GetStatistics(context);
    var endpointStatistics = rlContext.EndpointLimiter.GetStatistics(context);

    _someTelemetryService.PushStatisticsToPrometheus(globalStatistics);
    _someTelemetryService.PushStatisticsToPrometheus(endpointStatistics);
}

Scenario 2: Get metadata from successful RateLimiterLease for this request

public Task Invoke(HttpContext context)
{
    var rlContext = context.Features.Get<IRateLimiterContextFeature>();

    if (rlContext.Lease.TryGetMetadata("SOME_METADATA", out var metadata)
    {
        // Do some additional stuff, depends on metadata
    }
}

Alternative Design - 1

As @Tratcher said, there is a risk, that RateLimitLease would be disposed early. To prevent that, we can introduce facade-like class to wrap Lease with undisposable entity:

public interface IRateLimiterContextFeature
{
         HttpContext HttpContext { get; }

         RateLimitLeaseInfo LeaseInfo { get; } 

         PartitionedRateLimiter<HttpContext>? GlobalLimiter { get; }

         PartitionedRateLimiter<HttpContext> EndpointLimiter { get; }
}

public abstract class RateLimitLeaseInfo 
{
        // Same props as RateLimitLease, but without Dispose()
}

Alternative Design - 2

Also, Instead of using HttpContext Features, we can use HttpContext.Items and Extension methods, like its done in Microsoft.AspNetCore.Authentication extention methods (HttpContext.SingIn, HttpContext.ChallangeAsync, etc). So, we use the Alternative Design - 1 approach with Facade-like api, but implemented with extension methods

// very simple example of implementation (for only 1 method) just to show the API
public static class HttpContextRateLimiterExtentions
{
    public static RateLimiterStatistics GetRateLimiterStatistics(this HttpContext context)
    {
        // just for example - the code is not correct
        var limiter = (PartitionedRateLimiter<HttpContext>)context.Items["SomeGlobalLimiter"];
        var stats = limiter?.GetStatistics(context);
        return stats;
    }
}

// and then in some middleware:
var statistics = HttpContext.GetRateLimiterStatistics();

Risks

In Design 1:

• would anyone use this to dispose of the lease early? Would that have any strange side-effects, especially if it got double-disposed by the middleware later?

[MadL1me]

Related to #45652

[Tratcher]

Risk

• would anyone use this to dispose of the lease early? Would that have any strange side-effects, especially if it got double-disposed by the middleware later?

[Tratcher]

IRateLimiterContextFeature - Consider making all of the properties read only. There's no scenario for replacing them, correct?

[MadL1me]

Yep, I think making them read only would be best way to do. At firstly, I thought we can provide more flexibility by not restricting it, but there is really no way to use setters in any adequate scenario

[MadL1me]

Risk

• would anyone use this to dispose of the lease early? Would that have any strange side-effects, especially if it got double-disposed by the middleware later?

Investigated dotnet/runtime. All limiters there correctly implement IDisposable, and can't be disposed twice:

• ConcurrencyLimiter
• SlidingWindowRateLimiter
• TokenBucketRateLimiter
• DefaultPartitionedRateLimiter

Your point about disposing is right - yes, someone can dispose it later in middleware. I don't know about any scenarios how that can be used, all my thought about use case of this - add some ("admin"/"user" filtering logic - something we can already do with partitioned rate limiter)

[MadL1me]

After more thought, I think we can do this:

namespace Microsoft.AspNetCore.RateLimiting.Features;

public interface IRateLimiterContextFeature
{
         HttpContext HttpContext { get; }

         RateLimitLeaseInfo LeaseInfo { get; } 

         PartitionedRateLimiter<HttpContext>? GlobalLimiter { get; }

         PartitionedRateLimiter<HttpContext> EndpointLimiter { get; }
}

public abstract class RateLimitLeaseInfo 
{
        public abstract bool IsAcquired { get; }

        public abstract bool TryGetMetadata(string metadataName, out object? metadata);

        public bool TryGetMetadata<T>(MetadataName<T> metadataName, [MaybeNull] out T metadata)
        {
            if (metadataName.Name == null)
            {
                metadata = default;
                return false;
            }

            bool successful = TryGetMetadata(metadataName.Name, out object? rawMetadata);
            if (successful)
            {
                metadata = rawMetadata is null ? default : (T)rawMetadata;
                return true;
            }

            metadata = default;
            return false;
        }

        public abstract IEnumerable<string> MetadataNames { get; }

        public virtual IEnumerable<KeyValuePair<string, object?>> GetAllMetadata()
        {
            foreach (string name in MetadataNames)
            {
                if (TryGetMetadata(name, out object? metadata))
                {
                    yield return new KeyValuePair<string, object?>(name, metadata);
                }
            }
        }
}

There is 2 important changes:

• All properties are read-only now (as @Tratcher proposed)
• Instead of using RateLimitLease - we can use facade-like abstraction which have almost the same API, but without IDisposable, so problem with disposing and breaking invariants dissapear

[MadL1me]

@Tratcher is there anything I can research/investigate for this issue to continue in discussing? I think #44140 is pretty wanted as feature set

[Tratcher]

We'll review the API in a week or two when people are back from the holidays.

[cristipufu]

Let's imagine the statistics are stored in Redis/SQL, I wouldn't want to call GetStatistics() on every http request

[Tratcher]

Having HttpContext on a Feature interface is weirdly circular, normally you're retrieving features from the HttpContext. Why would you need to store the context?

[MadL1me]

GetStatistics() require context as parameter, because limiters in middleware are PartitionedLimiter and require context as parameter. I think it would be better for API , because for example, you only need to pass IRateLimiterContextFeature to method without HttpContext to do the job - get the statistics. All required things are stored in one interface.

[MadL1me]

Also, I added alternative design in first message: https://github.com/dotnet/aspnetcore/issues/45658#issue-1501829704

[ghost]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[halter73]

How important is it to expose the global and endpoint PartitionedRateLimiter<HttpContext> instances directly? Do we expect anyone will want to manually acquire additional leases with these? If that's something people really want to do, we might be able to expose both of these instances as services instead of features. Both PartitionedRateLimiter instances are basically singletons, although you could have multiple if you're using multiple instances of the rate limiting middleware.

If acquiring additional leases is not a goal, it might be cleaner to expose RateLimiterStatistics? GetGlobalStatistics() and RateLimiterStatistics? GetEndpointStatistics() methods on the feature directly. That way we wouldn't have to worry about flowing the HttpContext. Do you have any thoughts on this @BrennanConroy ?

[halter73]

API Review Notes:

• Given that it might not be optimal to query statistics on every request, do we have a good way to query these statistics in the background? Putting the PartitionedRateLimiter<HttpContext> instances in a singleton service might help with getting the limiters outside a request context, but you still need an HttpContext to call GetStatistics. Could we parameters the EndpointRateLimiter on Endpoint instead of HttpContext?
  • In the future, the EndpointRateLimiter may partition within an endpoint. This doesn't happen today, but we'd be locking ourselves in a bit if we expose a PartitionedRateLimiter<Endpoint> instead of PartitionedRateLimiter<HttpContext>.
  • And in the case of the global rate limiter we cannot really use any key other than HttpContext because there could be a completely custom partitioning scheme.
• What about RateLimitLeaseInfo in Alternative Design - 1? We don't want people calling Dispose() on it themselves, but we're not sure this warrants a mostly redundant type.
• An issue with making the rate limiters services is there could be multiple instances of the middleware.
• Do we need the IRateLimiterContextFeature.HttpContext property? It could be convenient to save passing around an extra object sometimes, but it seem unnecessary. It could always be added later if it's too difficult to use without it.
• Do we want to make this opt-in? It could save an allocation if we don't pool. You don't have to opt in at the PartitionRateLimiter level, but MemoryCache has an opt-in API to avoid perf regressions. (https://github.com/dotnet/runtime/issues/50406#issuecomment-1074521694). We could pool IRateLimiterContextFeature if we really want to.
  • We could add an opt-out later in a non-breaking way, but that'd be ugly for the majority of users who might not need this feature.
  • Let's make it opt in.
• We'd like to provide GetStatistics() APIs rather than expose the entire PartitionedRateLimiter<HttpContext> yet. If it's the global one, it should be accessible to the user because they set it on the options.
• Can we come up with a better name than IRateLimiterContextFeature? Can we segregate the interface?
• Do we prefer IRateLimitLeaseFeature or IRateLimiterLeaseFeature? Let's stick with "RateLimiter" even though it doesn't appear in the RateLimitLease type.
• Should IRateLimiterLeaseFeature.Lease be nullable? Endpoint is nullable for the endpoint feature. And there won't be any lease if you disable rate limiting with the attribute, so yes.
• Are we concerned about exposing the the RateLimitLease after it's disposed? Yes. Let's unset it. This means it's only accessible in inner middleware and OnRejected, but then we can pool.

The following version the the API is approved!

+ namespace Microsoft.AspNetCore.RateLimiting.Features;

+ public interface IRateLimiterStatisticsFeature
+ {
+     RateLimiterStatistics? GetGlobalStatistics();
+     RateLimiterStatistics? GetEndpointStatistics();
+ }

+ public interface IRateLimiterLeaseFeature
+ {
+      RateLimitLease? Lease { get; } 
+ }

namespace Microsoft.AspNetCore.RateLimiting;

public sealed class RateLimiterOptions
{
    // IRateLimiterStatisticsFeature won't be set unless this is opted into, but will not be unset after the rate limiting middleware exits.
    // IRateLimiterLeaseFeature will always be set, but we should be able to pool it since we unset to avoid exposing a disposed lease.
+    public bool TrackStatistics { get; set; }
}

[MadL1me]

@halter73 thank you for review, and sorry if my proposal wasted a lot of time to rethinking my design. I'll try to learn from your thought process, and I hope I'll make better API proposals next time

[halter73]

@MadL1me Your proposal was really good and got us thinking in the right direction. Thanks!

And the API review notes are not from me specifically. I'm summarizing feedback from a bunch of engineers who discussed this in an API review meeting. I don't think any one of us would have come up with this exact design initially. It's a process.

[adityamandaleeka]

@MadL1me Will you be updating your PR (https://github.com/dotnet/aspnetcore/pull/45652/files) based on this API review feedback?

[MadL1me]

Yes! I have an exam in 2 days, after that I'll submit a PR @adityamandaleeka

[MadL1me]

Decided to make proposed this API with 2 separate PR's for each feature, so I'm closed this old PR (https://github.com/dotnet/aspnetcore/pull/45652). All work is moved to https://github.com/dotnet/aspnetcore/pull/46028

[ghost]

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[adityamandaleeka]

@MadL1me Thanks, and feel free to ignore the bot message above. Look forward to seeing your PRs go in!

[Tratcher]

+ namespace Microsoft.AspNetCore.RateLimiting.Features;

@halter73 why did we decide to put this in the Features namespace? That's not something we've done for other components like https://github.com/dotnet/aspnetcore/blob/334da01db159058defbd39f128d659ddf3ae3f7e/src/Middleware/OutputCaching/src/IOutputCacheFeature.cs#L4-L9 https://github.com/dotnet/aspnetcore/blob/334da01db159058defbd39f128d659ddf3ae3f7e/src/Middleware/Diagnostics.Abstractions/src/IExceptionHandlerFeature.cs#L7-L12

[davidfowl]

Agreed

[halter73]

I think we added the namespace because most of our HTTP and connection-level features live in Features namespaces, and we wanted to be consistent. However, I agree we should move the new features middleware's primary namespace considering we've already done that for output caching and exception handler middleware. It's nice to have less namespaces, and it's not like it's too cluttered.

[ghost]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[MadL1me]

@halter73 in your message (https://github.com/dotnet/aspnetcore/issues/45658#issuecomment-1372967290) you wrote:

IRateLimiterLeaseFeature will always be set, but we should be able to pool it since we unset to avoid exposing a disposed lease.

Can you please clarify what that means? How is object pooling corresponds with avoiding exposing disposed lease?

[halter73]

Can you please clarify what that means? How is object pooling corresponds with avoiding exposing disposed lease?

I wouldn't do this as part of the initial change. But if we wanted to later, we could return the object to the pool in the rate limiting middleware after we unset the feature. We do not want to pool anything that is still accessible via the HttpContext.

[BrennanConroy]

Thinking about this a little, the GetEndpointStatistics and GetGlobalStatistics methods imply getting statistics for the specific Endpoint and for the whole app respectively. But in reality, they are getting statistics for a specific request type at the app level and at the Endpoint level.

What that means is that the statistics can be very different per request even if the requests end up on the same Endpoint. For example, if you partition based on User or query string you'll get a completely different limiter, even though the request goes to the same Endpoint, which means when you call one of the statistics methods you will get completely different statistics.

The method naming/usage does little to suggest this behavior which I would argue is going to be confusing to users.

[halter73]

That is a good point about the naming not indicating that the statistics are arbitrarily partitioned based on how the policies and/or global limiters are configured.

Do you have any suggestions for names that would make this clearer? Maybe something like GetCurrentPartitionStatistics()? The downside is that you couldn't query just the global or endpoint statistics for the given request, but I think that might be okay.

+ namespace Microsoft.AspNetCore.RateLimiting;

+ public interface IRateLimiterStatisticsFeature
+ {
+     RateLimiterMiddlewareStatistics GetCurrentPartitionStatistics();
+ }

+ public sealed class RateLimiterMiddlewareStatistics
+ {
+     RateLimiterStatistics? GlobalStatistics { get; init; }
+     RateLimiterStatistics? EndpointStatistics { get; init; }
+ }

[halter73]

API Review Notes:

• We like removing the Microsoft.AspNetCore.RateLimiting.Features namespace.
• We still are not sure what we want to do about the confusing nature of IRateLimiterStatisticsFeature only referencing the partition of the current request. We think it's fine to do that, but it might not be obvious enough what's really going on given the current API naming.

We will try to get back to this in API review Monday. I'm leaving it to @BrennanConroy to improve the proposal since he doesn't like my proposal 😆 .

[MadL1me]

Thinking about this a little, the GetEndpointStatistics and GetGlobalStatistics methods imply getting statistics for the specific Endpoint and for the whole app respectively. But in reality, they are getting statistics for a specific request type at the app level and at the Endpoint level.

What if we try to do it that way? For example, we could return a List of statistics, because we know that Middleware always uses PartitionedRateLimiter:

public sealed class RateLimiterMiddlewareStatistics
{
     // new props, contain statistics for ALL limiters in PartitonedRateLimiter
     List<RateLimiterStatistics?> GlobalStatistics { get; init; }
     List<RateLimiterStatistics?> EndpointStatistics { get; init; }

     // props proposed before, renamed to indicate that partition is used
     RateLimiterStatistics? PartitionGlobalStatistics{ get; init; }
     RateLimiterStatistics? PartitionEndpointStatistics { get; init; }
}

However, this probably would require add methods to fetch all statistics from PartitionedRateLimiter.

[mitchdenny]

@halter73 bubbling this to the top of your stack. See issue #47456 where a scenario involving accessing the rate limiter from the feature came up. Basically the idea is that in addition to getting a ticket to execute the endpoint, the developer might decide to eat up some additional tickets after execution IF the request warranted it.

Sometimes it's hard to know whether a given request will be expensive in advance, so deducting the tickets after execution.

[MadL1me]

@halter73 @mitchdenny I think we should combine statistics solution with #47456 idea.

If we create and expose entire IRateLimiterFeature, it'll make possible for a custom RateLimiterMiddleware to be made, which we can use with #47456 scenario as well as "fetching Statistics" problem.

[mitchdenny]

It looks like this hasn't been implemented yet. I'd like to revisit the conversation in light of the following issue:

https://github.com/dotnet/aspnetcore/issues/47456

This issue illustrates a scenario where being able to access the global/endpoint limiters via a feature inside some middleware could be useful. For example you might use rate limiters to automatically consume a token for every request, but after a request has been processed you might decide that particular request was more expensive so you want to take more tokens.

If we exposed the rate limiters via a feature this could be easily implemented. Even though this API proposal was more about statistics, by exposing the rate limiters we would allow both scenarios.

/cc @halter73

[MadL1me]

@mitchdenny agreed, I think exposing limiters via features would add a great flexibility to feature usage. I would like to continue to work on this issue, after .net team discussion.

[hacst]

It seems like some parts of this proposal are still stuck in the discussion phase. Personally I am not that interested in the IRateLimiterStatisticsFeature portion right now but in accessing the current lease using the proposed IRateLimiterLeaseFeature.

In https://github.com/cristipufu/aspnetcore-redis-rate-limiting/wiki/Rate-Limiting-Headers when acquiring the lease remaining/limit/reset information is cheaply available, stored in the lease and made available as metadata on the lease. Having IRateLimiterLeaseFeature would solve the initial idea behind #44140 on its own for these rate limiters and others using a similar approach. As far as I can tell it is also the only approach to return exact values from the time of acquire if they are available. It would of course also unlock any other use-case that can be built based on the lease information on its own.

@BrennanConroy (or whoever can decide this): Is it possible to approve and pursue IRateLimiterLeaseFeature independently of the statistics specific parts of the proposal?

[hacst]

Independent of my previous question I have some thoughts on the statistics portion of the proposal: RateLimiter.GetStatistics is a blocking method without any specific guidance on how it should behave.

If the expectation is to be able to readily call it for every request to populate RateLimiterStatistics fields this has to be clarified as that is not easily achieved in distributed scenarios. E.g. just doing a redis query in there as https://github.com/cristipufu/aspnetcore-redis-rate-limiting/ does will lead to thread pool exhaustion with a bit of load. https://github.com/dotnet/runtime/issues/88592 proposes an async variant of GetStatistics but it hasn't progressed yet and it might still be an expensive call.

If the expectation is to do updates in the background around the calls as hinted at in the review discussion from Jan 6th 23 I am unsure how that could be achieved generically in an efficient way for a heavily partitioned rate limiter. It could always serve stale data from the last call and schedule a refresh after but that might get very stale and will not work for the first call. Doing regular updates outside of that would retrieve a lot of statistics for rate limiters you might not end up needing for any request in that timeframe. Also bound to be a bit stale but more predictable.

The rate limiter itself might be able to get up-to-date statistics and cache them for GetStatistics calls when a lease is acquired. Where possible that would give the best results for this proposal and #44140. But then what about other uses of GetStatistics outside of a specific request context that expect to get a current snapshot?

TL;DR: Imo behavior of RateLimiter.GetStatistics has to be clarified before it is possible to say whether it can be safely and efficiently used to retrieve statistics here.

[abergs]

Came across this issue while googling and I'll just add on here that I've been experimenting with consuming a variable amount of tokens, but also to manually acquire leases and expect the statistics. So I'd very much like a nice way to get access to active limiters.