Skip to content
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

Fix metrics duration and http.route tag with exception handling #52652

Merged
merged 8 commits into from
Dec 13, 2023
Merged
Show file tree
Hide file tree
Changes from 7 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
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,8 @@ public void RequestEnd(HttpContext httpContext, Exception? exception, HostingApp

if (context.MetricsEnabled)
{
var route = httpContext.GetEndpoint()?.Metadata.GetMetadata<IRouteDiagnosticsMetadata>()?.Route;
var endpoint = HttpExtensions.GetOriginalEndpoint(httpContext);
var route = endpoint?.Metadata.GetMetadata<IRouteDiagnosticsMetadata>()?.Route;
var customTags = context.MetricsTagsFeature?.TagsList;

_metrics.RequestEnd(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
<Reference Include="Microsoft.Extensions.Options" />

<Compile Include="$(SharedSourceRoot)TypeNameHelper\*.cs" />
<Compile Include="$(SharedSourceRoot)HttpExtensions.cs" />
</ItemGroup>

<ItemGroup>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -247,12 +247,7 @@ private static void ClearHttpContext(HttpContext context)

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

private static Task ClearCacheHeaders(object state)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
<Compile Include="$(SharedSourceRoot)StackTrace\**\*.cs" />
<Compile Include="$(SharedSourceRoot)TypeNameHelper\*.cs" />
<Compile Include="$(SharedSourceRoot)Reroute.cs" />
<Compile Include="$(SharedSourceRoot)HttpExtensions.cs" />
</ItemGroup>

<ItemGroup>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -190,11 +190,7 @@ private static Func<StatusCodeContext, Task> CreateHandler(string pathFormat, st

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

context.HttpContext.Request.Path = newPath;
context.HttpContext.Request.QueryString = newQueryString;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System.Diagnostics;
using System.Diagnostics.Metrics;
using System.Net;
using System.Net.Http;
using System.Net.Http.Headers;
using System.Net.Http.Json;
Expand All @@ -11,20 +12,22 @@
using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Http.Features;
using Microsoft.AspNetCore.InternalTesting;
using Microsoft.AspNetCore.Mvc;
using Microsoft.AspNetCore.Routing;
using Microsoft.AspNetCore.Routing.Patterns;
using Microsoft.AspNetCore.TestHost;
using Microsoft.AspNetCore.InternalTesting;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Diagnostics.Metrics;
using Microsoft.Extensions.Diagnostics.Metrics.Testing;
using Microsoft.Extensions.Hosting;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Logging.Abstractions;
using Microsoft.Extensions.Options;
using Moq;

namespace Microsoft.AspNetCore.Diagnostics;

public class ExceptionHandlerMiddlewareTest
public class ExceptionHandlerMiddlewareTest : LoggedTest
{
[Fact]
public async Task ExceptionIsSetOnProblemDetailsContext()
Expand Down Expand Up @@ -291,6 +294,133 @@ public async Task Metrics_ExceptionThrown_DefaultSettings_Handled_Reported()
m => AssertRequestException(m, "System.InvalidOperationException", "handled", null));
}

[Fact]
public async Task Metrics_ExceptionThrown_Handled_UseOriginalRoute()
{
// Arrange
var originalEndpointBuilder = new RouteEndpointBuilder(c => Task.CompletedTask, RoutePatternFactory.Parse("/path"), 0);
var originalEndpoint = originalEndpointBuilder.Build();

var meterFactory = new TestMeterFactory();
using var requestDurationCollector = new MetricCollector<double>(meterFactory, "Microsoft.AspNetCore.Hosting", "http.server.request.duration");
using var requestExceptionCollector = new MetricCollector<long>(meterFactory, DiagnosticsMetrics.MeterName, "aspnetcore.diagnostics.exceptions");

using var host = new HostBuilder()
.ConfigureServices(s =>
{
s.AddSingleton<IMeterFactory>(meterFactory);
s.AddSingleton(LoggerFactory);
})
.ConfigureWebHost(webHostBuilder =>
{
webHostBuilder
.UseTestServer()
.Configure(app =>
{
app.UseExceptionHandler(new ExceptionHandlerOptions
{
ExceptionHandler = (c) => Task.CompletedTask
});
app.Run(context =>
{
context.SetEndpoint(originalEndpoint);
throw new Exception("Test exception");
});

});
}).Build();

await host.StartAsync();

var server = host.GetTestServer();

// Act
var response = await server.CreateClient().GetAsync("/path");
Assert.Equal(HttpStatusCode.InternalServerError, response.StatusCode);

await requestDurationCollector.WaitForMeasurementsAsync(minCount: 1).DefaultTimeout();

// Assert
Assert.Collection(
requestDurationCollector.GetMeasurementSnapshot(),
m =>
{
Assert.True(m.Value > 0);
Assert.Equal(500, (int)m.Tags["http.response.status_code"]);
Assert.Equal("System.Exception", (string)m.Tags["error.type"]);
Assert.Equal("/path", (string)m.Tags["http.route"]);
});
Assert.Collection(requestExceptionCollector.GetMeasurementSnapshot(),
m => AssertRequestException(m, "System.Exception", "handled"));
}

[Fact]
public async Task Metrics_ExceptionThrown_Handled_UseNewRoute()
{
// Arrange
var originalEndpointBuilder = new RouteEndpointBuilder(c => Task.CompletedTask, RoutePatternFactory.Parse("/path"), 0);
var originalEndpoint = originalEndpointBuilder.Build();

var newEndpointBuilder = new RouteEndpointBuilder(c => Task.CompletedTask, RoutePatternFactory.Parse("/new"), 0);
var newEndpoint = newEndpointBuilder.Build();

var meterFactory = new TestMeterFactory();
using var requestDurationCollector = new MetricCollector<double>(meterFactory, "Microsoft.AspNetCore.Hosting", "http.server.request.duration");
using var requestExceptionCollector = new MetricCollector<long>(meterFactory, DiagnosticsMetrics.MeterName, "aspnetcore.diagnostics.exceptions");

using var host = new HostBuilder()
.ConfigureServices(s =>
{
s.AddSingleton<IMeterFactory>(meterFactory);
s.AddSingleton(LoggerFactory);
})
.ConfigureWebHost(webHostBuilder =>
{
webHostBuilder
.UseTestServer()
.Configure(app =>
{
app.UseExceptionHandler(new ExceptionHandlerOptions
{
ExceptionHandler = (c) =>
{
c.SetEndpoint(newEndpoint);
return Task.CompletedTask;
}
});
app.Run(context =>
{
context.SetEndpoint(originalEndpoint);
throw new Exception("Test exception");
});

});
}).Build();

await host.StartAsync();

var server = host.GetTestServer();

// Act
var response = await server.CreateClient().GetAsync("/path");
Assert.Equal(HttpStatusCode.InternalServerError, response.StatusCode);

await requestDurationCollector.WaitForMeasurementsAsync(minCount: 1).DefaultTimeout();

// Assert
Assert.Collection(
requestDurationCollector.GetMeasurementSnapshot(),
m =>
{
Assert.True(m.Value > 0);
Assert.Equal(500, (int)m.Tags["http.response.status_code"]);
Assert.Equal("System.Exception", (string)m.Tags["error.type"]);
Assert.Equal("/new", (string)m.Tags["http.route"]);
});
Assert.Collection(requestExceptionCollector.GetMeasurementSnapshot(),
m => AssertRequestException(m, "System.Exception", "handled"));
}

[Fact]
public async Task Metrics_ExceptionThrown_Unhandled_Reported()
{
Expand Down
36 changes: 36 additions & 0 deletions src/Shared/HttpExtensions.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Http.Features;

internal static class HttpExtensions
{
Expand All @@ -10,6 +12,8 @@ internal static class HttpExtensions
internal static bool IsValidHttpMethodForForm(string method) =>
HttpMethods.IsPost(method) || HttpMethods.IsPut(method) || HttpMethods.IsPatch(method);

internal const string OriginalEndpointKey = "__OriginalEndpoint";
JamesNK marked this conversation as resolved.
Show resolved Hide resolved
JamesNK marked this conversation as resolved.
Show resolved Hide resolved

internal static bool IsValidContentTypeForForm(string? contentType)
{
if (contentType == null)
Expand All @@ -27,4 +31,36 @@ internal static bool IsValidContentTypeForForm(string? contentType)
return contentType.Equals(UrlEncodedFormContentType, StringComparison.OrdinalIgnoreCase) ||
contentType.StartsWith(MultipartFormContentType, StringComparison.OrdinalIgnoreCase);
}

internal static Endpoint? GetOriginalEndpoint(HttpContext context)
{
var endpoint = context.GetEndpoint();

// 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.
// The original endpoint is stashed with a known key in HttpContext.Items. Use it as a fallback.
if (endpoint == null && context.Items.TryGetValue(OriginalEndpointKey, out var e) && e is Endpoint originalEndpoint)
{
endpoint = originalEndpoint;
}
return endpoint;
}

internal static void ClearEndpoint(HttpContext context)
{
var endpoint = context.GetEndpoint();
if (endpoint != null)
{
context.Items[OriginalEndpointKey] = endpoint;

// An endpoint may have already been set. Since we're going to re-invoke the middleware pipeline we need to reset
// the endpoint and route values to ensure things are re-calculated.
context.SetEndpoint(endpoint: null);
}

var routeValuesFeature = context.Features.Get<IRouteValuesFeature>();
if (routeValuesFeature != null)
{
routeValuesFeature.RouteValues = null!;
}
}
}
Loading