Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions src/Middleware/RateLimiting/src/MetricsContext.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,12 @@ namespace Microsoft.AspNetCore.RateLimiting;
internal readonly struct MetricsContext
{
public readonly string? PolicyName;
public readonly string? Method;
public readonly string? Route;
public readonly bool CurrentLeasedRequestsCounterEnabled;
public readonly bool CurrentQueuedRequestsCounterEnabled;

public MetricsContext(string? policyName, string? method, string? route, bool currentLeasedRequestsCounterEnabled, bool currentQueuedRequestsCounterEnabled)
public MetricsContext(string? policyName, bool currentLeasedRequestsCounterEnabled, bool currentQueuedRequestsCounterEnabled)
{
PolicyName = policyName;
Method = method;
Route = route;
CurrentLeasedRequestsCounterEnabled = currentLeasedRequestsCounterEnabled;
CurrentQueuedRequestsCounterEnabled = currentQueuedRequestsCounterEnabled;
}
Expand Down
12 changes: 2 additions & 10 deletions src/Middleware/RateLimiting/src/RateLimitingMetrics.cs
Original file line number Diff line number Diff line change
Expand Up @@ -162,18 +162,10 @@ private static void InitializeRateLimitingTags(ref TagList tags, in MetricsConte
{
tags.Add("policy", metricsContext.PolicyName);
}
if (metricsContext.Method is not null)
{
tags.Add("method", metricsContext.Method);
}
if (metricsContext.Route is not null)
{
tags.Add("route", metricsContext.Route);
}
}

public MetricsContext CreateContext(string? policyName, string? method, string? route)
public MetricsContext CreateContext(string? policyName)
{
return new MetricsContext(policyName, method, route, _currentLeasedRequestsCounter.Enabled, _currentQueuedRequestsCounter.Enabled);
return new MetricsContext(policyName, _currentLeasedRequestsCounter.Enabled, _currentQueuedRequestsCounter.Enabled);
}
}
12 changes: 3 additions & 9 deletions src/Middleware/RateLimiting/src/RateLimitingMiddleware.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
using System.Diagnostics;
using System.Threading.RateLimiting;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Http.Metadata;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;

Expand Down Expand Up @@ -78,23 +77,18 @@ public Task Invoke(HttpContext context)
return _next(context);
}

// Only include route and method if we have an endpoint with a route.
// A request always has a HTTP request, but it isn't useful unless combined with a route.
var route = endpoint?.Metadata.GetMetadata<IRouteDiagnosticsMetadata>()?.Route;
var method = route is not null ? context.Request.Method : null;

return InvokeInternal(context, enableRateLimitingAttribute, method, route);
return InvokeInternal(context, enableRateLimitingAttribute);
}

private async Task InvokeInternal(HttpContext context, EnableRateLimitingAttribute? enableRateLimitingAttribute, string? method, string? route)
private async Task InvokeInternal(HttpContext context, EnableRateLimitingAttribute? enableRateLimitingAttribute)
{
var policyName = enableRateLimitingAttribute?.PolicyName;

// Cache the up/down counter enabled state at the start of the middleware.
// This ensures that the state is consistent for the entire request.
// For example, if a meter listener starts after a request is queued, when the request exits the queue
// the requests queued counter won't go into a negative value.
var metricsContext = _metrics.CreateContext(policyName, method, route);
var metricsContext = _metrics.CreateContext(policyName);

using var leaseContext = await TryAcquireAsync(context, metricsContext);

Expand Down
28 changes: 12 additions & 16 deletions src/Middleware/RateLimiting/test/RateLimitingMetricsTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ public async Task Metrics_Success()
await syncPoint.WaitForSyncPoint().DefaultTimeout();

Assert.Collection(currentLeaseRequestsRecorder.GetMeasurements(),
m => AssertCounter(m, 1, null, null, null));
m => AssertCounter(m, 1, null));
Assert.Empty(leaseRequestDurationRecorder.GetMeasurements());

syncPoint.Continue();
Expand All @@ -104,10 +104,10 @@ public async Task Metrics_Success()
Assert.Equal(StatusCodes.Status200OK, context.Response.StatusCode);

Assert.Collection(currentLeaseRequestsRecorder.GetMeasurements(),
m => AssertCounter(m, 1, null, null, null),
m => AssertCounter(m, -1, null, null, null));
m => AssertCounter(m, 1, null),
m => AssertCounter(m, -1, null));
Assert.Collection(leaseRequestDurationRecorder.GetMeasurements(),
m => AssertDuration(m, null, null, null));
m => AssertDuration(m, null));
Assert.Empty(currentRequestsQueuedRecorder.GetMeasurements());
Assert.Empty(queuedRequestDurationRecorder.GetMeasurements());
Assert.Empty(leaseFailedRequestsRecorder.GetMeasurements());
Expand Down Expand Up @@ -156,7 +156,7 @@ public async Task Metrics_ListenInMiddleOfRequest_CurrentLeasesNotDecreased()

Assert.Empty(currentLeaseRequestsRecorder.GetMeasurements());
Assert.Collection(leaseRequestDurationRecorder.GetMeasurements(),
m => AssertDuration(m, null, null, null));
m => AssertDuration(m, null));
}

[Fact]
Expand Down Expand Up @@ -214,7 +214,7 @@ public async Task Metrics_Queued()

// Assert second request is queued.
Assert.Collection(currentRequestsQueuedRecorder.GetMeasurements(),
m => AssertCounter(m, 1, "GET", "/", "concurrencyPolicy"));
m => AssertCounter(m, 1, "concurrencyPolicy"));
Assert.Empty(queuedRequestDurationRecorder.GetMeasurements());

// Allow both requests to finish.
Expand All @@ -224,10 +224,10 @@ public async Task Metrics_Queued()
await middlewareTask2.DefaultTimeout();

Assert.Collection(currentRequestsQueuedRecorder.GetMeasurements(),
m => AssertCounter(m, 1, "GET", "/", "concurrencyPolicy"),
m => AssertCounter(m, -1, "GET", "/", "concurrencyPolicy"));
m => AssertCounter(m, 1, "concurrencyPolicy"),
m => AssertCounter(m, -1, "concurrencyPolicy"));
Assert.Collection(queuedRequestDurationRecorder.GetMeasurements(),
m => AssertDuration(m, "GET", "/", "concurrencyPolicy"));
m => AssertDuration(m, "concurrencyPolicy"));
}

[Fact]
Expand Down Expand Up @@ -296,22 +296,18 @@ public async Task Metrics_ListenInMiddleOfQueued_CurrentQueueNotDecreased()

Assert.Empty(currentRequestsQueuedRecorder.GetMeasurements());
Assert.Collection(queuedRequestDurationRecorder.GetMeasurements(),
m => AssertDuration(m, "GET", "/", "concurrencyPolicy"));
m => AssertDuration(m, "concurrencyPolicy"));
}

private static void AssertCounter(Measurement<long> measurement, long value, string method, string route, string policy)
private static void AssertCounter(Measurement<long> measurement, long value, string policy)
{
Assert.Equal(value, measurement.Value);
AssertTag(measurement.Tags, "method", method);
AssertTag(measurement.Tags, "route", route);
AssertTag(measurement.Tags, "policy", policy);
}

private static void AssertDuration(Measurement<double> measurement, string method, string route, string policy)
private static void AssertDuration(Measurement<double> measurement, string policy)
{
Assert.True(measurement.Value > 0);
AssertTag(measurement.Tags, "method", method);
AssertTag(measurement.Tags, "route", route);
AssertTag(measurement.Tags, "policy", policy);
}

Expand Down