Rate limiter context HttpContext feature API proposal

[MadL1me]

Background and Motivation

In #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.