From 02234bf531badce7f6e59db85cc20abdd6f8e133 Mon Sep 17 00:00:00 2001 From: Ciaran Liedeman Date: Wed, 6 Dec 2023 13:07:46 +0200 Subject: [PATCH 1/7] Fixed Missing Endpoint When an exception if processed by the ExceptionHandler middleware the original endpoint is deleted --- .../Implementation/HttpInListener.cs | 4 ++- .../Implementation/HttpInMetricsListener.cs | 5 +++- .../RouteTests/RoutingTestCases.json | 10 +++++++ .../TestApplication/TestApplicationFactory.cs | 29 +++++++++++++++++++ 4 files changed, 46 insertions(+), 2 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs index a9aefacc005..e2a9d236a4f 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs @@ -9,6 +9,7 @@ using System.Runtime.CompilerServices; using Microsoft.AspNetCore.Http; #if !NETSTANDARD +using Microsoft.AspNetCore.Diagnostics; using Microsoft.AspNetCore.Routing; #endif using OpenTelemetry.Context.Propagation; @@ -236,7 +237,8 @@ public void OnStopActivity(Activity activity, object payload) var response = context.Response; #if !NETSTANDARD - var routePattern = (context.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText; + var routePattern = (context.Features.Get()?.Endpoint as RouteEndpoint ?? + context.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText; if (!string.IsNullOrEmpty(routePattern)) { activity.DisplayName = GetDisplayName(context.Request.Method, routePattern); diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs index c2afcd02a81..f0b8cb76ff7 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs @@ -9,6 +9,7 @@ #if NET6_0_OR_GREATER using System.Diagnostics.CodeAnalysis; +using Microsoft.AspNetCore.Diagnostics; using Microsoft.AspNetCore.Routing; #endif using OpenTelemetry.Trace; @@ -88,7 +89,9 @@ public static void OnStopEventWritten(string name, object payload) tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRequestMethod, httpMethod)); #if NET6_0_OR_GREATER - var route = (context.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText; + // Check the exception handler feature first in case the endpoint was overwritten + var route = (context.Features.Get()?.Endpoint as RouteEndpoint ?? + context.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText; if (!string.IsNullOrEmpty(route)) { tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRoute, route)); diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/RoutingTestCases.json b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/RoutingTestCases.json index 4d871d986a1..a404958ab4a 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/RoutingTestCases.json +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/RoutingTestCases.json @@ -198,5 +198,15 @@ "expectedStatusCode": 200, "currentHttpRoute": null, "expectedHttpRoute": "/MinimalApiUsingMapGroup/{id}" + }, + { + "name": "Exception Handled by Exception Handler Middleware", + "minimumDotnetVersion": 7, + "testApplicationScenario": "ExceptionMiddleware", + "httpMethod": "GET", + "path": "/Exception", + "expectedStatusCode": 500, + "currentHttpRoute": null, + "expectedHttpRoute": "/Exception" } ] diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/TestApplication/TestApplicationFactory.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/TestApplication/TestApplicationFactory.cs index 87f79f21e96..662ff429780 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/TestApplication/TestApplicationFactory.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/TestApplication/TestApplicationFactory.cs @@ -33,6 +33,11 @@ public enum TestApplicationScenario /// An Razor Pages application. /// RazorPages, + + /// + /// Application with Exception Handling Middleware. + /// + ExceptionMiddleware, } internal class TestApplicationFactory @@ -53,6 +58,8 @@ internal class TestApplicationFactory return CreateMinimalApiApplication(); case TestApplicationScenario.RazorPages: return CreateRazorPagesApplication(); + case TestApplicationScenario.ExceptionMiddleware: + return CreateExceptionHandlerApplication(); default: throw new ArgumentException($"Invalid {nameof(TestApplicationScenario)}"); } @@ -154,4 +161,26 @@ private static WebApplication CreateRazorPagesApplication() return app; } + + private static WebApplication CreateExceptionHandlerApplication() + { + var builder = WebApplication.CreateBuilder(); + builder.Logging.ClearProviders(); + +#if NET7_0_OR_GREATER + builder.Services.AddProblemDetails(); +#endif + + var app = builder.Build(); + +#if NET7_0_OR_GREATER + app.UseExceptionHandler(); +#endif + app.Urls.Clear(); + app.Urls.Add("http://[::1]:0"); + + app.MapGet("/Exception", (ctx) => throw new ApplicationException()); + + return app; + } } From 069bb8914a32773b2a8669dc1b35d467635caa65 Mon Sep 17 00:00:00 2001 From: Ciaran Liedeman Date: Sat, 13 Jan 2024 14:46:36 +0200 Subject: [PATCH 2/7] PR Feedback --- .../RouteTests/RoutingTestCases.json | 1 - .../RouteTests/TestApplication/TestApplicationFactory.cs | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/RoutingTestCases.json b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/RoutingTestCases.json index a404958ab4a..2d7dac9727a 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/RoutingTestCases.json +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/RoutingTestCases.json @@ -201,7 +201,6 @@ }, { "name": "Exception Handled by Exception Handler Middleware", - "minimumDotnetVersion": 7, "testApplicationScenario": "ExceptionMiddleware", "httpMethod": "GET", "path": "/Exception", diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/TestApplication/TestApplicationFactory.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/TestApplication/TestApplicationFactory.cs index 662ff429780..76dcda68df3 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/TestApplication/TestApplicationFactory.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/TestApplication/TestApplicationFactory.cs @@ -167,13 +167,13 @@ private static WebApplication CreateExceptionHandlerApplication() var builder = WebApplication.CreateBuilder(); builder.Logging.ClearProviders(); -#if NET7_0_OR_GREATER +#if NET6_0_OR_GREATER builder.Services.AddProblemDetails(); #endif var app = builder.Build(); -#if NET7_0_OR_GREATER +#if NET6_0_OR_GREATER app.UseExceptionHandler(); #endif app.Urls.Clear(); From 8063e2b76cae224310185a6a8d0d683d640169b4 Mon Sep 17 00:00:00 2001 From: Ciaran Liedeman Date: Sat, 13 Jan 2024 15:14:53 +0200 Subject: [PATCH 3/7] Add dotnet6 support for test --- .../RouteTests/README.net6.0.md | 18 ++++++++++++++++++ .../TestApplication/TestApplicationFactory.cs | 15 ++++++++++----- 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/README.net6.0.md b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/README.net6.0.md index c12dc40dadc..000f911f62f 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/README.net6.0.md +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/README.net6.0.md @@ -22,6 +22,7 @@ | :green_heart: | RazorPages | [Static content](#razorpages-static-content) | | :green_heart: | MinimalApi | [Action without parameter](#minimalapi-action-without-parameter) | | :green_heart: | MinimalApi | [Action with parameter](#minimalapi-action-with-parameter) | +| :green_heart: | ExceptionMiddleware | [Exception Handled by Exception Handler Middleware](#exceptionmiddleware-exception-handled-by-exception-handler-middleware) | ## ConventionalRouting: Root path @@ -590,3 +591,20 @@ } } ``` + +```json +{ + "IdealHttpRoute": "/Exception", + "ActivityDisplayName": "GET /Exception", + "ActivityHttpRoute": "/Exception", + "MetricHttpRoute": "/Exception", + "RouteInfo": { + "HttpMethod": "GET", + "Path": "/Exception", + "RoutePattern.RawText": null, + "IRouteDiagnosticsMetadata.Route": null, + "HttpContext.GetRouteData()": {}, + "ActionDescriptor": null + } +} +``` diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/TestApplication/TestApplicationFactory.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/TestApplication/TestApplicationFactory.cs index 76dcda68df3..009929b1227 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/TestApplication/TestApplicationFactory.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/TestApplication/TestApplicationFactory.cs @@ -5,6 +5,7 @@ using System.Diagnostics; using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Diagnostics; using Microsoft.AspNetCore.Http; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.FileProviders; @@ -167,14 +168,18 @@ private static WebApplication CreateExceptionHandlerApplication() var builder = WebApplication.CreateBuilder(); builder.Logging.ClearProviders(); -#if NET6_0_OR_GREATER - builder.Services.AddProblemDetails(); -#endif - var app = builder.Build(); #if NET6_0_OR_GREATER - app.UseExceptionHandler(); + app.UseExceptionHandler(exceptionHandlerApp => + { + exceptionHandlerApp.Run(async context => + { + context.Response.StatusCode = StatusCodes.Status500InternalServerError; + var exceptionHandlerPathFeature = context.Features.Get(); + await context.Response.WriteAsync(exceptionHandlerPathFeature?.Error.Message ?? "An exception was thrown."); + }); + }); #endif app.Urls.Clear(); app.Urls.Add("http://[::1]:0"); From 4dc0b823a76cb7fb536027a2aaa779ae5fc0f571 Mon Sep 17 00:00:00 2001 From: Ciaran Liedeman <3578740+cliedeman@users.noreply.github.com> Date: Fri, 26 Jan 2024 23:26:38 +0200 Subject: [PATCH 4/7] Update test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/TestApplication/TestApplicationFactory.cs Co-authored-by: Alan West <3676547+alanwest@users.noreply.github.com> --- .../RouteTests/TestApplication/TestApplicationFactory.cs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/TestApplication/TestApplicationFactory.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/TestApplication/TestApplicationFactory.cs index 009929b1227..857265fa4d0 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/TestApplication/TestApplicationFactory.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/TestApplication/TestApplicationFactory.cs @@ -184,7 +184,16 @@ private static WebApplication CreateExceptionHandlerApplication() app.Urls.Clear(); app.Urls.Add("http://[::1]:0"); +// TODO: Remove this condition once ASP.NET Core 8.0.2. +// Currently, .NET 8 has a different behavior than .NET 6 and 7. +// This is because ASP.NET Core 8+ has native metric instrumentation. +// When ASP.NET Core 8.0.2 is released then its behavior will align with .NET 6/7. +// See: https://github.com/dotnet/aspnetcore/issues/52648#issuecomment-1853432776 +#if !NET8_0_OR_GREATER app.MapGet("/Exception", (ctx) => throw new ApplicationException()); +#else + app.MapGet("/Exception", () => Results.Content(content: "Error", contentType: null, contentEncoding: null, statusCode: 500)); +#endif return app; } From f9a356a09c5190c462f2c4bae202b70fe7344bc9 Mon Sep 17 00:00:00 2001 From: Ciaran Liedeman Date: Sat, 13 Jan 2024 15:14:53 +0200 Subject: [PATCH 5/7] Add dotnet6 support for test --- .../RouteTests/TestApplication/TestApplicationFactory.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/TestApplication/TestApplicationFactory.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/TestApplication/TestApplicationFactory.cs index 857265fa4d0..3093d619bc3 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/TestApplication/TestApplicationFactory.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/TestApplication/TestApplicationFactory.cs @@ -170,7 +170,6 @@ private static WebApplication CreateExceptionHandlerApplication() var app = builder.Build(); -#if NET6_0_OR_GREATER app.UseExceptionHandler(exceptionHandlerApp => { exceptionHandlerApp.Run(async context => @@ -180,7 +179,7 @@ private static WebApplication CreateExceptionHandlerApplication() await context.Response.WriteAsync(exceptionHandlerPathFeature?.Error.Message ?? "An exception was thrown."); }); }); -#endif + app.Urls.Clear(); app.Urls.Add("http://[::1]:0"); From ab846dc9b21604619d1766d76dc9aa6abd994f16 Mon Sep 17 00:00:00 2001 From: Ciaran Liedeman Date: Fri, 26 Jan 2024 23:30:44 +0200 Subject: [PATCH 6/7] updated tests --- .../RouteTests/README.net6.0.md | 2 ++ .../RouteTests/README.net7.0.md | 20 +++++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/README.net6.0.md b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/README.net6.0.md index 000f911f62f..d2c3b5b29f7 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/README.net6.0.md +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/README.net6.0.md @@ -592,6 +592,8 @@ } ``` +## ExceptionMiddleware: Exception Handled by Exception Handler Middleware + ```json { "IdealHttpRoute": "/Exception", diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/README.net7.0.md b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/README.net7.0.md index ec252654a2a..35a0e331882 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/README.net7.0.md +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/README.net7.0.md @@ -24,6 +24,7 @@ | :green_heart: | MinimalApi | [Action with parameter](#minimalapi-action-with-parameter) | | :green_heart: | MinimalApi | [Action without parameter (MapGroup)](#minimalapi-action-without-parameter-mapgroup) | | :green_heart: | MinimalApi | [Action with parameter (MapGroup)](#minimalapi-action-with-parameter-mapgroup) | +| :green_heart: | ExceptionMiddleware | [Exception Handled by Exception Handler Middleware](#exceptionmiddleware-exception-handled-by-exception-handler-middleware) | ## ConventionalRouting: Root path @@ -632,3 +633,22 @@ } } ``` + +## ExceptionMiddleware: Exception Handled by Exception Handler Middleware + +```json +{ + "IdealHttpRoute": "/Exception", + "ActivityDisplayName": "GET /Exception", + "ActivityHttpRoute": "/Exception", + "MetricHttpRoute": "/Exception", + "RouteInfo": { + "HttpMethod": "GET", + "Path": "/Exception", + "RoutePattern.RawText": null, + "IRouteDiagnosticsMetadata.Route": null, + "HttpContext.GetRouteData()": {}, + "ActionDescriptor": null + } +} +``` From e340c76a80500888d4677689faba47c042746652 Mon Sep 17 00:00:00 2001 From: Alan West <3676547+alanwest@users.noreply.github.com> Date: Fri, 26 Jan 2024 16:52:44 -0800 Subject: [PATCH 7/7] Update test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/TestApplication/TestApplicationFactory.cs --- .../TestApplication/TestApplicationFactory.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/TestApplication/TestApplicationFactory.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/TestApplication/TestApplicationFactory.cs index 3093d619bc3..b030ab7f42b 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/TestApplication/TestApplicationFactory.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/TestApplication/TestApplicationFactory.cs @@ -183,11 +183,11 @@ private static WebApplication CreateExceptionHandlerApplication() app.Urls.Clear(); app.Urls.Add("http://[::1]:0"); -// TODO: Remove this condition once ASP.NET Core 8.0.2. -// Currently, .NET 8 has a different behavior than .NET 6 and 7. -// This is because ASP.NET Core 8+ has native metric instrumentation. -// When ASP.NET Core 8.0.2 is released then its behavior will align with .NET 6/7. -// See: https://github.com/dotnet/aspnetcore/issues/52648#issuecomment-1853432776 + // TODO: Remove this condition once ASP.NET Core 8.0.2. + // Currently, .NET 8 has a different behavior than .NET 6 and 7. + // This is because ASP.NET Core 8+ has native metric instrumentation. + // When ASP.NET Core 8.0.2 is released then its behavior will align with .NET 6/7. + // See: https://github.com/dotnet/aspnetcore/issues/52648#issuecomment-1853432776 #if !NET8_0_OR_GREATER app.MapGet("/Exception", (ctx) => throw new ApplicationException()); #else