Fix duplicate error.type tags (#55211)

Author: JamesNK

src/Hosting/Hosting/src/Internal/HostingMetrics.cs

@@ -69,12 +69,8 @@ public void RequestEnd(string protocol, bool isHttps, string scheme, string meth
             {
                 tags.Add("http.route", route);
             }
-            // This exception is only present if there is an unhandled exception.
-            // An exception caught by ExceptionHandlerMiddleware and DeveloperExceptionMiddleware isn't thrown to here. Instead, those middleware add error.type to custom tags.
-            if (exception != null)
-            {
-                tags.Add("error.type", exception.GetType().FullName);
-            }
+
+            // Add before some built in tags so custom tags are prioritized when dealing with duplicates.
             if (customTags != null)
             {
                 for (var i = 0; i < customTags.Count; i++)
@@ -83,6 +79,15 @@ public void RequestEnd(string protocol, bool isHttps, string scheme, string meth
                 }
             }
 
+            // This exception is only present if there is an unhandled exception.
+            // An exception caught by ExceptionHandlerMiddleware and DeveloperExceptionMiddleware isn't thrown to here. Instead, those middleware add error.type to custom tags.
+            if (exception != null)
+            {
+                // Exception tag could have been added by middleware. If an exception is later thrown in request pipeline
+                // then we don't want to add a duplicate tag here because that breaks some metrics systems.
+                tags.TryAddTag("error.type", exception.GetType().FullName);
+            }
+
             var duration = Stopwatch.GetElapsedTime(startTimestamp, currentTimestamp);
             _requestDuration.Record(duration.TotalSeconds, tags);
         }

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

@@ -16,6 +16,7 @@
     <Compile Include="$(SharedSourceRoot)StackTrace\**\*.cs" />
     <Compile Include="$(SharedSourceRoot)ErrorPage\**\*.cs" />
     <Compile Include="$(SharedSourceRoot)StaticWebAssets\**\*.cs" LinkBase="StaticWebAssets" />
+    <Compile Include="$(SharedSourceRoot)Metrics\MetricsExtensions.cs" />
   </ItemGroup>
 
   <ItemGroup>

src/Middleware/Diagnostics/src/DiagnosticsTelemetry.cs

@@ -15,7 +15,9 @@ public static void ReportUnhandledException(ILogger logger, HttpContext context,
 
         if (context.Features.Get<IHttpMetricsTagsFeature>() is { } tagsFeature)
         {
-            tagsFeature.Tags.Add(new KeyValuePair<string, object?>("error.type", ex.GetType().FullName));
+            // Multiple exception middleware could be registered that have already added the tag.
+            // We don't want to add a duplicate tag here because that breaks some metrics systems.
+            tagsFeature.TryAddTag("error.type", ex.GetType().FullName);
         }
     }
 }

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

@@ -17,6 +17,7 @@
     <Compile Include="$(SharedSourceRoot)TypeNameHelper\*.cs" />
     <Compile Include="$(SharedSourceRoot)Reroute.cs" />
     <Compile Include="$(SharedSourceRoot)HttpExtensions.cs" />
+    <Compile Include="$(SharedSourceRoot)Metrics\MetricsExtensions.cs" />
   </ItemGroup>
 
   <ItemGroup>

src/Middleware/Diagnostics/test/FunctionalTests/ExceptionHandlerSampleTest.cs

@@ -1,4 +1,4 @@
-// Licensed to the .NET Foundation under one or more agreements.
+// Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System.Net;

src/Middleware/Diagnostics/test/UnitTests/ExceptionHandlerTest.cs

@@ -15,6 +15,7 @@
 using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Logging.Testing;
 using Microsoft.Extensions.Diagnostics.Metrics.Testing;
+using System.Net.Http;
 
 namespace Microsoft.AspNetCore.Diagnostics;
 
@@ -965,4 +966,136 @@ public async Task UnhandledError_ExceptionNameTagAdded()
                 Assert.Equal("System.Exception", (string)m.Tags["error.type"]);
             });
     }
+
+    [Fact]
+    public async Task UnhandledError_MultipleHandlers_ExceptionNameTagAddedOnce()
+    {
+        // Arrange
+        var meterFactory = new TestMeterFactory();
+        using var instrumentCollector = new MetricCollector<double>(meterFactory, "Microsoft.AspNetCore.Hosting", "http.server.request.duration");
+
+        using var host = new HostBuilder()
+            .ConfigureServices(s =>
+            {
+                s.AddSingleton<IMeterFactory>(meterFactory);
+            })
+            .ConfigureWebHost(webHostBuilder =>
+            {
+                webHostBuilder
+                .UseTestServer()
+                .Configure(app =>
+                {
+                    // Second error and handler
+                    app.UseExceptionHandler(new ExceptionHandlerOptions()
+                    {
+                        ExceptionHandler = httpContext =>
+                        {
+                            httpContext.Response.StatusCode = StatusCodes.Status500InternalServerError;
+                            return Task.CompletedTask;
+                        }
+                    });
+                    app.Use(async (context, next) =>
+                    {
+                        await next();
+                        throw new InvalidOperationException("Test exception2");
+                    });
+
+                    // First error and handler
+                    app.UseExceptionHandler(new ExceptionHandlerOptions()
+                    {
+                        ExceptionHandler = httpContext =>
+                        {
+                            httpContext.Response.StatusCode = StatusCodes.Status404NotFound;
+                            return Task.CompletedTask;
+                        }
+                    });
+                    app.Run(context =>
+                    {
+                        throw new Exception("Test exception1");
+                    });
+                });
+            }).Build();
+
+        await host.StartAsync();
+
+        var server = host.GetTestServer();
+
+        // Act
+        var response = await server.CreateClient().GetAsync("/path");
+        Assert.Equal(HttpStatusCode.InternalServerError, response.StatusCode);
+
+        await instrumentCollector.WaitForMeasurementsAsync(minCount: 1).DefaultTimeout();
+
+        // Assert
+        Assert.Collection(
+            instrumentCollector.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"]);
+            });
+    }
+
+    [Fact]
+    public async Task UnhandledError_ErrorAfterHandler_ExceptionNameTagAddedOnce()
+    {
+        // Arrange
+        var meterFactory = new TestMeterFactory();
+        using var instrumentCollector = new MetricCollector<double>(meterFactory, "Microsoft.AspNetCore.Hosting", "http.server.request.duration");
+
+        using var host = new HostBuilder()
+            .ConfigureServices(s =>
+            {
+                s.AddSingleton<IMeterFactory>(meterFactory);
+            })
+            .ConfigureWebHost(webHostBuilder =>
+            {
+                webHostBuilder
+                .UseTestServer()
+                .Configure(app =>
+                {
+                    // Second error
+                    app.Use(async (context, next) =>
+                    {
+                        await next();
+
+                        throw new InvalidOperationException("Test exception2");
+                    });
+
+                    // First error and handler
+                    app.UseExceptionHandler(new ExceptionHandlerOptions()
+                    {
+                        ExceptionHandler = httpContext =>
+                        {
+                            httpContext.Response.StatusCode = StatusCodes.Status404NotFound;
+                            return httpContext.Response.WriteAsync("Custom handler");
+                        }
+                    });
+                    app.Run(context =>
+                    {
+                        throw new Exception("Test exception1");
+                    });
+                });
+            }).Build();
+
+        await host.StartAsync();
+
+        var server = host.GetTestServer();
+
+        // Act
+        await Assert.ThrowsAsync<HttpRequestException>(async () => await server.CreateClient().GetAsync("/path"));
+
+        await instrumentCollector.WaitForMeasurementsAsync(minCount: 1).DefaultTimeout();
+
+        // Assert
+        Assert.Collection(
+            instrumentCollector.GetMeasurementSnapshot(),
+            m =>
+            {
+                Assert.True(m.Value > 0);
+                Assert.Equal(404, (int)m.Tags["http.response.status_code"]);
+                Assert.Equal("System.Exception", (string)m.Tags["error.type"]);
+            });
+    }
 }

src/Middleware/Diagnostics/test/testassets/ExceptionHandlerSample/ExceptionHandlerSample.csproj

@@ -10,5 +10,6 @@
     <Reference Include="Microsoft.AspNetCore.Server.IISIntegration" />
     <Reference Include="Microsoft.AspNetCore.Server.Kestrel" />
     <Reference Include="Microsoft.AspNetCore.StaticFiles" />
+    <Reference Include="Microsoft.AspNetCore.WebSockets" />
   </ItemGroup>
 </Project>

src/Middleware/Diagnostics/test/testassets/ExceptionHandlerSample/StartupWithWebSocket.cs

@@ -0,0 +1,49 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System.Diagnostics;
+using System.Text;
+using System.Text.Encodings.Web;
+using Microsoft.AspNetCore.Diagnostics;
+using Microsoft.AspNetCore.Http.Metadata;
+
+namespace ExceptionHandlerSample;
+
+// Note that this class isn't used in tests as TestServer doesn't have the right behavior to test web sockets
+// in the way we need. But leaving here so it can be used in Program.cs when starting the app manually.
+public class StartupWithWebSocket
+{
+    public void ConfigureServices(IServiceCollection services)
+    {
+    }
+
+    public void Configure(IApplicationBuilder app)
+    {
+        app.UseExceptionHandler(options => { }); // Exception handling middleware introduces duplicate tag
+        app.UseWebSockets();
+
+        app.Use(async (HttpContext context, Func<Task> next) =>
+        {
+            try
+            {
+                if (context.WebSockets.IsWebSocketRequest)
+                {
+                    using var ws = await context.WebSockets.AcceptWebSocketAsync();
+                    await ws.SendAsync(new ArraySegment<byte>(Encoding.UTF8.GetBytes("Hello")), System.Net.WebSockets.WebSocketMessageType.Text, true, context.RequestAborted);
+                    await ws.CloseAsync(System.Net.WebSockets.WebSocketCloseStatus.NormalClosure, "done", context.RequestAborted);
+                    throw new InvalidOperationException("Throw after websocket request completion to produce the bug");
+                }
+                else
+                {
+                    await context.Response.WriteAsync($"Not a web socket request. PID: {Process.GetCurrentProcess().Id}");
+                }
+            }
+            catch (Exception ex)
+            {
+                _ = ex;
+                throw;
+            }
+        });
+    }
+}
+

src/Shared/Metrics/MetricsExtensions.cs

@@ -0,0 +1,55 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System.Diagnostics;
+using Microsoft.AspNetCore.Http.Features;
+
+namespace Microsoft.AspNetCore.Http;
+
+internal static class MetricsExtensions
+{
+    public static bool TryAddTag(this IHttpMetricsTagsFeature feature, string name, object? value)
+    {
+        var tags = feature.Tags;
+
+        // Tags is internally represented as a List<T>.
+        // Prefer looping through the list to avoid allocating an enumerator.
+        if (tags is List<KeyValuePair<string, object?>> list)
+        {
+            foreach (var tag in list)
+            {
+                if (tag.Key == name)
+                {
+                    return false;
+                }
+            }
+        }
+        else
+        {
+            foreach (var tag in tags)
+            {
+                if (tag.Key == name)
+                {
+                    return false;
+                }
+            }
+        }
+
+        tags.Add(new KeyValuePair<string, object?>(name, value));
+        return true;
+    }
+
+    public static bool TryAddTag(this ref TagList tags, string name, object? value)
+    {
+        for (var i = 0; i < tags.Count; i++)
+        {
+            if (tags[i].Key == name)
+            {
+                return false;
+            }
+        }
+
+        tags.Add(new KeyValuePair<string, object?>(name, value));
+        return true;
+    }
+}