dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.98k stars 4.66k forks source link

[API Proposal]: AggregateRateLimiter #98017

Open TonyValenti opened 7 months ago

TonyValenti commented 7 months ago

Background and motivation

It would be nice to be able to AND (and possibly OR) rate limiters together.

For example, "I want a rate limiter that allows no more than 10 requests per second and no more than 2 concurrent requsts".

API Proposal

namespace System.Threading.RateLimiting;

public class AggregateRateLimiter : RateLimiter
{
  public AggregateRateLimiter(IEnumerable<RateLimiter> Children);
}

API Usage

var Limiter1 = ...;
var Limiter2 = ...;
var Limiter = new AggregateRateLimiter([Limit1, Limit2]);

Alternative Designs

People can always roll their own.

Risks

None that I can think of.

ghost commented 7 months ago

Tagging subscribers to this area: @mangod9 See info in area-owners.md if you want to be subscribed.

Issue Details
### Background and motivation It would be nice to be able to AND (and possibly OR) rate limiters together. For example, "I want a rate limiter that allows no more than 10 requests per second and no more than 2 concurrent requsts". ### API Proposal ```csharp namespace System.Threading.RateLimiting public class AggregateRateLimiter : RateLimiter { public AggregateRateLimiter(IEnumerable Children); } ``` ### API Usage ```csharp var Limiter1 = ...; var Limiter2 = ...; var Limiter = new AggregateRateLimiter([Limit1, Limit2]); ``` ### Alternative Designs People can always roll their own. ### Risks None that I can think of.
Author: TonyValenti
Assignees: -
Labels: `api-suggestion`, `area-System.Threading`
Milestone: -
stephentoub commented 7 months ago

cc: @BrennanConroy

BrennanConroy commented 7 months ago

I'm not against it, we didn't add this for non-partitioned rate limiters because it's not particularly hard to write yourself. But we do have prior-art for adding similar API recently: https://github.com/dotnet/runtime/issues/93623

I have seen a few similar asks as well on other issues.

API would probably be:

+ public class ChainedRateLimiter : RateLimiter
+ {
+     public ChainedRateLimiter(params RateLimiter[] limiters);
+ }

or

public abstract class RateLimiter
{
+    public static RateLimiter CreateChained(params RateLimiter[] limiters);
}
TonyValenti commented 4 months ago

Is there any chance someone could add this? I could really use it.

SbiCA commented 4 months ago

Is there any chance someone could add this? I could really use it.

@TonyValenti +1 on that could you please also add your thoughts here since you provided a draft of how this could look like. It seems there are a few other people wanting this feature.

https://github.com/dotnet/aspnetcore/discussions/54051

TonyValenti commented 4 months ago

The link you listed is for ASP.NET stuff with rate limiting. My use case is in a desktop app where we still need to limit requests to an API. Personally, I prefer RateLimiter.Aggregate but whatever the name is, I dont really care, I just could really use it.

SbiCA commented 4 months ago

@BrennanConroy would it make sense to separate the Asp.Net core part into an own issue then?

BrennanConroy commented 4 months ago

Background and motivation

It would be nice to be able to AND rate limiters together.

For example, "I want a rate limiter that allows no more than 10 requests per second and no more than 2 concurrent requests".

PartitionedRateLimiter has static PartitionedRateLimiter CreateChained(params PartitionedRateLimiter[] limiters); already.

Recent similar API is public static TextWriter CreateBroadcasting(params TextWriter[] writers); https://github.com/dotnet/runtime/issues/93623

API Proposal

namespace System.Threading.RateLimiting;

+ public sealed class ChainedRateLimiter : RateLimiter
+ {
+    public ChainedRateLimiter(ReadOnlySpan<RateLimiter> limiters);
+ }

API Usage

var Limiter1 = new ConcurrencyLimiter(new ConcurrencyLimiterOptions() { PermitLimit = 5 };
var Limiter2 = new TokenBucketRateLimiter(new TokenBucketRateLimiterOptions() { TokenLimit = 10, ReplenishPeriod = TimeSpan.FromSeconds(1), TokensPerPeriod = 2 };
var Limiter = new ChainedRateLimiter([Limiter1, Limiter2]);

Alternative Designs

public abstract class RateLimiter
{
+    public static RateLimiter CreateChained(ReadOnlySpan<RateLimiter> limiters);
}
+ public sealed class AggregateRateLimiter : RateLimiter
+ {
+     public AggregateRateLimiter(ReadOnlySpan<RateLimiter> limiters);
+ }

Risks

None.

dersia commented 1 month ago

is this really aggregation or rather chaining? what would it mean, if they are aggregated?

I would suggest calling these either CompositeRateLimiter or ChainedRateLimiter

julealgon commented 1 month ago

It would be nice to be able to AND (and possibly OR) rate limiters together.

Wouldn't this be a key part of the implementation? Which should be picked? How would a consumer pick among the two if both were supported?

In general, I just don't see why the framework should implement a composite or chain of responsibility pattern directly. Leaving it to the consumer allows the consumer to create whatever is needed in their own context, without forcing an over-complicated API surface in the framework that needs to fit all use cases.

The aggregation logic can be basically anything, so to me this proposal just doesn't scale properly.

terrajobst commented 1 month ago

Video

namespace System.Threading.RateLimiting;

public abstract class RateLimiter
{
    public static RateLimiter CreateChained(params RateLimiter[] limiters);
}