-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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.
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
[API Proposal]: Improve RateLimiter metrics #71804
Comments
Tagging subscribers to this area: @mangod9 Issue DetailsBackground and motivationOne of the original potential use cases cited for API Proposalnamespace System.Threading.RateLimiting;
public abstract class RateLimiter : IAsyncDisposable, IDisposable
{
- public abstract int GetAvailablePermits();
+ public abstract RateLimiterStatistics? GetStatistics();
}
public abstract class PartitionedRateLimiter<TResource> : IAsyncDisposable, IDisposable
{
- public abstract int GetAvailablePermits(TResource resourceID);
+ public abstract RateLimiterStatistics? GetStatistics(TResource resourceID);
}
+ public class RateLimiterStatistics
+ {
+ public RateLimiterStatistics();
+ public long CurrentAvailablePermits { get; init; }
+ public long CurrentQueuedCount { get; init; }
+ public long TotalFailedLeases { get; init; }
+ public long TotalSuccessfulLeases { get; init; }
+ } API UsageMeter meter = new Meter("RateLimiter", "1.0.0");
RateLimiter limiter = new ConcurrencyLimiter(
new ConcurrencyLimiterOptions(100, QueueProcessingOrder.OldestFirst, 20));
meter.CreateObservableGauge<long>("available-permits", GetAvailablePermits);
IEnumerable<Measurement<long>> GetAvailablePermits()
{
return new Measurement<long>[]
{
new Measurement<long>(limiter.GetStatistics()!.CurrentAvailablePermits,
new KeyValuePair<string, object?>("Limiter", "Concurrent")),
};
} Alternative DesignsAn alternative name is Another property that might be useful to include is There has been discussion around allowing rate limiter specific state to be returned, either via this API or another API. This would be similar to
Two ways to allow state without introducing additional APIs to what is already proposed is to use the existing metadata on RateLimitLease and call Acquire with 0 permits to always get back a lease without acquiring any permits. Or we keep the new RateLimiterStatistics class that's being proposed as non-sealed and allow implementations to provide their own statistics type that contains custom state.
RisksIf someone was wanting to query
|
Looks good as proposed. (Changed resourceID to resource to match an in-flight rename request) namespace System.Threading.RateLimiting;
public abstract class RateLimiter : IAsyncDisposable, IDisposable
{
- public abstract int GetAvailablePermits();
+ public abstract RateLimiterStatistics? GetStatistics();
}
public abstract class PartitionedRateLimiter<TResource> : IAsyncDisposable, IDisposable
{
- public abstract int GetAvailablePermits(TResource resourceID);
+ public abstract RateLimiterStatistics? GetStatistics(TResource resource);
}
+ public class RateLimiterStatistics
+ {
+ public RateLimiterStatistics();
+ public long CurrentAvailablePermits { get; init; }
+ public long CurrentQueuedCount { get; init; }
+ public long TotalFailedLeases { get; init; }
+ public long TotalSuccessfulLeases { get; init; }
+ } |
Background and motivation
One of the original potential use cases cited for
RateLimiter.GetAvailablePermits()
was for diagnostics. While it's a good starting point, it would make more sense to include more information to include in diagnostics. So, taking inspiration from MemoryCache we are proposing changingGetAvailablePermits
to a more useful API:GetStatistics
.API Proposal
API Usage
Alternative Designs
An alternative name is
GetCurrentStatistics
which is what MemoryCache uses.Another property that might be useful to include is
long TotalAcquiredPermits { get; init; }
There has been discussion around allowing rate limiter specific state to be returned, either via this API or another API. This would be similar to
HttpClientHandler.Properties
runtime/src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.cs
Line 300 in 8cd9d63
Two ways to allow state without introducing additional APIs to what is already proposed is to use the existing metadata on
RateLimitLease
and callAcquire
with 0 permits to always get back a lease without acquiring any permits. Or we keep the newRateLimiterStatistics
class that's being proposed as non-sealed and allow implementations to provide their own statistics type that contains custom state.Risks
If someone was wanting to query
GetAvailablePermits
before every call toAcquire
orWaitAsync
they would now need to call a more expensive method (allocates every call) to get the same behavior. I don't believe this would be a common pattern, if someone wants to acquire permits quickly without potential waiting, they can callAcquire
which will already check for availability and be more accurate than callingGetAvailablePermits
thenAcquire
if there are enough permits.The text was updated successfully, but these errors were encountered: