From 2f1329ea273d67ba9f740004688a6619f1264fb9 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Sun, 11 Jun 2023 12:28:24 +0800 Subject: [PATCH 1/2] Remove route and method tags from rate limiting --- src/Middleware/RateLimiting/src/MetricsContext.cs | 6 +----- .../RateLimiting/src/RateLimitingMetrics.cs | 12 ++---------- .../RateLimiting/src/RateLimitingMiddleware.cs | 12 +++--------- 3 files changed, 6 insertions(+), 24 deletions(-) diff --git a/src/Middleware/RateLimiting/src/MetricsContext.cs b/src/Middleware/RateLimiting/src/MetricsContext.cs index 5a77cd9c4cd3..08e58c90cf32 100644 --- a/src/Middleware/RateLimiting/src/MetricsContext.cs +++ b/src/Middleware/RateLimiting/src/MetricsContext.cs @@ -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; } diff --git a/src/Middleware/RateLimiting/src/RateLimitingMetrics.cs b/src/Middleware/RateLimiting/src/RateLimitingMetrics.cs index e06d8c405487..b2307b5d2063 100644 --- a/src/Middleware/RateLimiting/src/RateLimitingMetrics.cs +++ b/src/Middleware/RateLimiting/src/RateLimitingMetrics.cs @@ -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); } } diff --git a/src/Middleware/RateLimiting/src/RateLimitingMiddleware.cs b/src/Middleware/RateLimiting/src/RateLimitingMiddleware.cs index 18c1e9bf1bd4..a1277888206e 100644 --- a/src/Middleware/RateLimiting/src/RateLimitingMiddleware.cs +++ b/src/Middleware/RateLimiting/src/RateLimitingMiddleware.cs @@ -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; @@ -78,15 +77,10 @@ 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()?.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; @@ -94,7 +88,7 @@ private async Task InvokeInternal(HttpContext context, EnableRateLimitingAttribu // 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); From 16bbcdda7bc4ab3216363548260721c779840ceb Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Sun, 11 Jun 2023 13:10:48 +0800 Subject: [PATCH 2/2] Update tests --- .../test/RateLimitingMetricsTests.cs | 28 ++++++++----------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/src/Middleware/RateLimiting/test/RateLimitingMetricsTests.cs b/src/Middleware/RateLimiting/test/RateLimitingMetricsTests.cs index 03b054d50725..69562f4c8b0a 100644 --- a/src/Middleware/RateLimiting/test/RateLimitingMetricsTests.cs +++ b/src/Middleware/RateLimiting/test/RateLimitingMetricsTests.cs @@ -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(); @@ -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()); @@ -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] @@ -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. @@ -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] @@ -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 measurement, long value, string method, string route, string policy) + private static void AssertCounter(Measurement 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 measurement, string method, string route, string policy) + private static void AssertDuration(Measurement measurement, string policy) { Assert.True(measurement.Value > 0); - AssertTag(measurement.Tags, "method", method); - AssertTag(measurement.Tags, "route", route); AssertTag(measurement.Tags, "policy", policy); }