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<HttpContext> 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; }
+ }