Skip to content

Commit 62a85bf

Browse files
JamesNKJamesNK
authored
[release/8.0] Fix metrics duration and http.route tag with exception handling (#52790)
* Fix metrics duration and http.route tag with exception handling * Fix build
1 parent 148cba6 commit 62a85bf

File tree

7 files changed

+176
-15
lines changed

7 files changed

+176
-15
lines changed

‎src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,8 @@ public void RequestEnd(HttpContext httpContext, Exception? exception, HostingApp
150150

151151
if (context.MetricsEnabled)
152152
{
153-
var route = httpContext.GetEndpoint()?.Metadata.GetMetadata<IRouteDiagnosticsMetadata>()?.Route;
153+
var endpoint = HttpExtensions.GetOriginalEndpoint(httpContext);
154+
var route = endpoint?.Metadata.GetMetadata<IRouteDiagnosticsMetadata>()?.Route;
154155
var customTags = context.MetricsTagsFeature?.TagsList;
155156

156157
_metrics.RequestEnd(

‎src/Hosting/Hosting/src/Microsoft.AspNetCore.Hosting.csproj

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
<Reference Include="Microsoft.Extensions.Options" />
3535

3636
<Compile Include="$(SharedSourceRoot)TypeNameHelper\*.cs" />
37+
<Compile Include="$(SharedSourceRoot)HttpExtensions.cs" />
3738
</ItemGroup>
3839

3940
<ItemGroup>

‎src/Middleware/Diagnostics/src/ExceptionHandler/ExceptionHandlerMiddlewareImpl.cs

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -247,12 +247,7 @@ private static void ClearHttpContext(HttpContext context)
247247

248248
// An endpoint may have already been set. Since we're going to re-invoke the middleware pipeline we need to reset
249249
// the endpoint and route values to ensure things are re-calculated.
250-
context.SetEndpoint(endpoint: null);
251-
var routeValuesFeature = context.Features.Get<IRouteValuesFeature>();
252-
if (routeValuesFeature != null)
253-
{
254-
routeValuesFeature.RouteValues = null!;
255-
}
250+
HttpExtensions.ClearEndpoint(context);
256251
}
257252

258253
private static Task ClearCacheHeaders(object state)

‎src/Middleware/Diagnostics/src/Microsoft.AspNetCore.Diagnostics.csproj

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
<Compile Include="$(SharedSourceRoot)StackTrace\**\*.cs" />
1717
<Compile Include="$(SharedSourceRoot)TypeNameHelper\*.cs" />
1818
<Compile Include="$(SharedSourceRoot)Reroute.cs" />
19+
<Compile Include="$(SharedSourceRoot)HttpExtensions.cs" />
1920
</ItemGroup>
2021

2122
<ItemGroup>

‎src/Middleware/Diagnostics/src/StatusCodePage/StatusCodePagesExtensions.cs

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -190,11 +190,7 @@ private static Func<StatusCodeContext, Task> CreateHandler(string pathFormat, st
190190

191191
// An endpoint may have already been set. Since we're going to re-invoke the middleware pipeline we need to reset
192192
// the endpoint and route values to ensure things are re-calculated.
193-
context.HttpContext.SetEndpoint(endpoint: null);
194-
if (routeValuesFeature != null)
195-
{
196-
routeValuesFeature.RouteValues = null!;
197-
}
193+
HttpExtensions.ClearEndpoint(context.HttpContext);
198194

199195
context.HttpContext.Request.Path = newPath;
200196
context.HttpContext.Request.QueryString = newQueryString;

‎src/Middleware/Diagnostics/test/UnitTests/ExceptionHandlerMiddlewareTest.cs

Lines changed: 133 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33

44
using System.Diagnostics;
55
using System.Diagnostics.Metrics;
6+
using System.Net;
67
using System.Net.Http;
78
using System.Net.Http.Headers;
89
using System.Net.Http.Json;
@@ -12,19 +13,21 @@
1213
using Microsoft.AspNetCore.Http;
1314
using Microsoft.AspNetCore.Http.Features;
1415
using Microsoft.AspNetCore.Mvc;
16+
using Microsoft.AspNetCore.Routing;
17+
using Microsoft.AspNetCore.Routing.Patterns;
1518
using Microsoft.AspNetCore.TestHost;
1619
using Microsoft.AspNetCore.Testing;
1720
using Microsoft.Extensions.DependencyInjection;
18-
using Microsoft.Extensions.Diagnostics.Metrics;
21+
using Microsoft.Extensions.Diagnostics.Metrics.Testing;
1922
using Microsoft.Extensions.Hosting;
23+
using Microsoft.Extensions.Logging;
2024
using Microsoft.Extensions.Logging.Abstractions;
2125
using Microsoft.Extensions.Options;
22-
using Microsoft.Extensions.Diagnostics.Metrics.Testing;
2326
using Moq;
2427

2528
namespace Microsoft.AspNetCore.Diagnostics;
2629

27-
public class ExceptionHandlerMiddlewareTest
30+
public class ExceptionHandlerMiddlewareTest : LoggedTest
2831
{
2932
[Fact]
3033
public async Task ExceptionIsSetOnProblemDetailsContext()
@@ -291,6 +294,133 @@ public async Task Metrics_ExceptionThrown_DefaultSettings_Handled_Reported()
291294
m => AssertRequestException(m, "System.InvalidOperationException", "handled", null));
292295
}
293296

297+
[Fact]
298+
public async Task Metrics_ExceptionThrown_Handled_UseOriginalRoute()
299+
{
300+
// Arrange
301+
var originalEndpointBuilder = new RouteEndpointBuilder(c => Task.CompletedTask, RoutePatternFactory.Parse("/path"), 0);
302+
var originalEndpoint = originalEndpointBuilder.Build();
303+
304+
var meterFactory = new TestMeterFactory();
305+
using var requestDurationCollector = new MetricCollector<double>(meterFactory, "Microsoft.AspNetCore.Hosting", "http.server.request.duration");
306+
using var requestExceptionCollector = new MetricCollector<long>(meterFactory, DiagnosticsMetrics.MeterName, "aspnetcore.diagnostics.exceptions");
307+
308+
using var host = new HostBuilder()
309+
.ConfigureServices(s =>
310+
{
311+
s.AddSingleton<IMeterFactory>(meterFactory);
312+
s.AddSingleton(LoggerFactory);
313+
})
314+
.ConfigureWebHost(webHostBuilder =>
315+
{
316+
webHostBuilder
317+
.UseTestServer()
318+
.Configure(app =>
319+
{
320+
app.UseExceptionHandler(new ExceptionHandlerOptions
321+
{
322+
ExceptionHandler = (c) => Task.CompletedTask
323+
});
324+
app.Run(context =>
325+
{
326+
context.SetEndpoint(originalEndpoint);
327+
throw new Exception("Test exception");
328+
});
329+
330+
});
331+
}).Build();
332+
333+
await host.StartAsync();
334+
335+
var server = host.GetTestServer();
336+
337+
// Act
338+
var response = await server.CreateClient().GetAsync("/path");
339+
Assert.Equal(HttpStatusCode.InternalServerError, response.StatusCode);
340+
341+
await requestDurationCollector.WaitForMeasurementsAsync(minCount: 1).DefaultTimeout();
342+
343+
// Assert
344+
Assert.Collection(
345+
requestDurationCollector.GetMeasurementSnapshot(),
346+
m =>
347+
{
348+
Assert.True(m.Value > 0);
349+
Assert.Equal(500, (int)m.Tags["http.response.status_code"]);
350+
Assert.Equal("System.Exception", (string)m.Tags["error.type"]);
351+
Assert.Equal("/path", (string)m.Tags["http.route"]);
352+
});
353+
Assert.Collection(requestExceptionCollector.GetMeasurementSnapshot(),
354+
m => AssertRequestException(m, "System.Exception", "handled"));
355+
}
356+
357+
[Fact]
358+
public async Task Metrics_ExceptionThrown_Handled_UseNewRoute()
359+
{
360+
// Arrange
361+
var originalEndpointBuilder = new RouteEndpointBuilder(c => Task.CompletedTask, RoutePatternFactory.Parse("/path"), 0);
362+
var originalEndpoint = originalEndpointBuilder.Build();
363+
364+
var newEndpointBuilder = new RouteEndpointBuilder(c => Task.CompletedTask, RoutePatternFactory.Parse("/new"), 0);
365+
var newEndpoint = newEndpointBuilder.Build();
366+
367+
var meterFactory = new TestMeterFactory();
368+
using var requestDurationCollector = new MetricCollector<double>(meterFactory, "Microsoft.AspNetCore.Hosting", "http.server.request.duration");
369+
using var requestExceptionCollector = new MetricCollector<long>(meterFactory, DiagnosticsMetrics.MeterName, "aspnetcore.diagnostics.exceptions");
370+
371+
using var host = new HostBuilder()
372+
.ConfigureServices(s =>
373+
{
374+
s.AddSingleton<IMeterFactory>(meterFactory);
375+
s.AddSingleton(LoggerFactory);
376+
})
377+
.ConfigureWebHost(webHostBuilder =>
378+
{
379+
webHostBuilder
380+
.UseTestServer()
381+
.Configure(app =>
382+
{
383+
app.UseExceptionHandler(new ExceptionHandlerOptions
384+
{
385+
ExceptionHandler = (c) =>
386+
{
387+
c.SetEndpoint(newEndpoint);
388+
return Task.CompletedTask;
389+
}
390+
});
391+
app.Run(context =>
392+
{
393+
context.SetEndpoint(originalEndpoint);
394+
throw new Exception("Test exception");
395+
});
396+
397+
});
398+
}).Build();
399+
400+
await host.StartAsync();
401+
402+
var server = host.GetTestServer();
403+
404+
// Act
405+
var response = await server.CreateClient().GetAsync("/path");
406+
Assert.Equal(HttpStatusCode.InternalServerError, response.StatusCode);
407+
408+
await requestDurationCollector.WaitForMeasurementsAsync(minCount: 1).DefaultTimeout();
409+
410+
// Assert
411+
Assert.Collection(
412+
requestDurationCollector.GetMeasurementSnapshot(),
413+
m =>
414+
{
415+
Assert.True(m.Value > 0);
416+
Assert.Equal(500, (int)m.Tags["http.response.status_code"]);
417+
Assert.Equal("System.Exception", (string)m.Tags["error.type"]);
418+
Assert.Equal("/new", (string)m.Tags["http.route"]);
419+
});
420+
Assert.Collection(requestExceptionCollector.GetMeasurementSnapshot(),
421+
m => AssertRequestException(m, "System.Exception", "handled"));
422+
}
423+
294424
[Fact]
295425
public async Task Metrics_ExceptionThrown_Unhandled_Reported()
296426
{

‎src/Shared/HttpExtensions.cs

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
// Licensed to the .NET Foundation under one or more agreements.
22
// The .NET Foundation licenses this file to you under the MIT license.
3+
34
using Microsoft.AspNetCore.Http;
5+
using Microsoft.AspNetCore.Http.Features;
46

57
internal static class HttpExtensions
68
{
@@ -10,6 +12,9 @@ internal static class HttpExtensions
1012
internal static bool IsValidHttpMethodForForm(string method) =>
1113
HttpMethods.IsPost(method) || HttpMethods.IsPut(method) || HttpMethods.IsPatch(method);
1214

15+
// Key is a string so shared code works across different assemblies (hosting, error handling middleware, etc).
16+
internal const string OriginalEndpointKey = "__OriginalEndpoint";
17+
1318
internal static bool IsValidContentTypeForForm(string? contentType)
1419
{
1520
if (contentType == null)
@@ -27,4 +32,36 @@ internal static bool IsValidContentTypeForForm(string? contentType)
2732
return contentType.Equals(UrlEncodedFormContentType, StringComparison.OrdinalIgnoreCase) ||
2833
contentType.StartsWith(MultipartFormContentType, StringComparison.OrdinalIgnoreCase);
2934
}
35+
36+
internal static Endpoint? GetOriginalEndpoint(HttpContext context)
37+
{
38+
var endpoint = context.GetEndpoint();
39+
40+
// Some middleware re-execute the middleware pipeline with the HttpContext. Before they do this, they clear state from context, such as the previously matched endpoint.
41+
// The original endpoint is stashed with a known key in HttpContext.Items. Use it as a fallback.
42+
if (endpoint == null && context.Items.TryGetValue(OriginalEndpointKey, out var e) && e is Endpoint originalEndpoint)
43+
{
44+
endpoint = originalEndpoint;
45+
}
46+
return endpoint;
47+
}
48+
49+
internal static void ClearEndpoint(HttpContext context)
50+
{
51+
var endpoint = context.GetEndpoint();
52+
if (endpoint != null)
53+
{
54+
context.Items[OriginalEndpointKey] = endpoint;
55+
56+
// An endpoint may have already been set. Since we're going to re-invoke the middleware pipeline we need to reset
57+
// the endpoint and route values to ensure things are re-calculated.
58+
context.SetEndpoint(endpoint: null);
59+
}
60+
61+
var routeValuesFeature = context.Features.Get<IRouteValuesFeature>();
62+
if (routeValuesFeature != null)
63+
{
64+
routeValuesFeature.RouteValues = null!;
65+
}
66+
}
3067
}

0 commit comments

Comments
 (0)