[ASP.NET Core] Add error.type attribute for tracing and metrics (#4986

)

src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreMetrics.cs

@@ -36,6 +36,8 @@ internal sealed class AspNetCoreMetrics : IDisposable
         "Microsoft.AspNetCore.Hosting.HttpRequestIn",
         "Microsoft.AspNetCore.Hosting.HttpRequestIn.Start",
         "Microsoft.AspNetCore.Hosting.HttpRequestIn.Stop",
+        "Microsoft.AspNetCore.Diagnostics.UnhandledException",
+        "Microsoft.AspNetCore.Hosting.UnhandledException",
     };
 
     private readonly Func<string, object, object, bool> isEnabled = (eventName, _, _)

src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md

@@ -18,6 +18,14 @@
   `http` or `http/dup`.
   ([#5001](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5001))
 
+* An additional attribute `error.type` will be added to activity and
+`http.server.request.duration` metric when the request results in unhandled
+exception. The attribute value will be set to full name of exception type.
+
+  The attribute will only be added when `OTEL_SEMCONV_STABILITY_OPT_IN`
+  environment variable is set to `http` or `http/dup`.
+  ([#4986](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4986))
+
 ## 1.6.0-beta.2
 
 Released 2023-Oct-26

src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs

@@ -425,6 +425,11 @@ public void OnException(Activity activity, object payload)
                 return;
             }
 
+            if (this.emitNewAttributes)
+            {
+                activity.SetTag(SemanticConventions.AttributeErrorType, exc.GetType().FullName);
+            }
+
             if (this.options.RecordException)
             {
                 activity.RecordException(exc);

src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs

@@ -20,6 +20,7 @@
 using OpenTelemetry.Internal;
 
 #if NET6_0_OR_GREATER
+using System.Diagnostics.CodeAnalysis;
 using Microsoft.AspNetCore.Routing;
 #endif
 using OpenTelemetry.Trace;
@@ -32,9 +33,14 @@ internal sealed class HttpInMetricsListener : ListenerHandler
     internal const string HttpServerDurationMetricName = "http.server.duration";
     internal const string HttpServerRequestDurationMetricName = "http.server.request.duration";
 
+    internal const string OnUnhandledHostingExceptionEvent = "Microsoft.AspNetCore.Hosting.UnhandledException";
+    internal const string OnUnhandledDiagnosticsExceptionEvent = "Microsoft.AspNetCore.Diagnostics.UnhandledException";
     private const string OnStopEvent = "Microsoft.AspNetCore.Hosting.HttpRequestIn.Stop";
     private const string EventName = "OnStopActivity";
     private const string NetworkProtocolName = "http";
+    private static readonly PropertyFetcher<Exception> ExceptionPropertyFetcher = new("Exception");
+    private static readonly PropertyFetcher<HttpContext> HttpContextPropertyFetcher = new("HttpContext");
+    private static readonly object ErrorTypeHttpContextItemsKey = new();
 
     private readonly Meter meter;
     private readonly AspNetCoreMetricsInstrumentationOptions options;
@@ -66,23 +72,65 @@ internal HttpInMetricsListener(string name, Meter meter, AspNetCoreMetricsInstru
 
     public override void OnEventWritten(string name, object payload)
     {
-        if (name == OnStopEvent)
+        switch (name)
         {
-            if (this.emitOldAttributes)
-            {
-                this.OnEventWritten_Old(name, payload);
-            }
+            case OnUnhandledDiagnosticsExceptionEvent:
+            case OnUnhandledHostingExceptionEvent:
+                {
+                    if (this.emitNewAttributes)
+                    {
+                        this.OnExceptionEventWritten(name, payload);
+                    }
+                }
+
+                break;
+            case OnStopEvent:
+                {
+                    if (this.emitOldAttributes)
+                    {
+                        this.OnEventWritten_Old(name, payload);
+                    }
+
+                    if (this.emitNewAttributes)
+                    {
+                        this.OnEventWritten_New(name, payload);
+                    }
+                }
+
+                break;
+        }
+    }
 
-            if (this.emitNewAttributes)
-            {
-                this.OnEventWritten_New(name, payload);
-            }
+    public void OnExceptionEventWritten(string name, object payload)
+    {
+        // We need to use reflection here as the payload type is not a defined public type.
+        if (!TryFetchException(payload, out Exception exc) || !TryFetchHttpContext(payload, out HttpContext ctx))
+        {
+            AspNetCoreInstrumentationEventSource.Log.NullPayload(nameof(HttpInMetricsListener), nameof(this.OnExceptionEventWritten), HttpServerDurationMetricName);
+            return;
         }
+
+        ctx.Items.Add(ErrorTypeHttpContextItemsKey, exc.GetType().FullName);
+
+        // See https://github.com/dotnet/aspnetcore/blob/690d78279e940d267669f825aa6627b0d731f64c/src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs#L252
+        // and https://github.com/dotnet/aspnetcore/blob/690d78279e940d267669f825aa6627b0d731f64c/src/Middleware/Diagnostics/src/DeveloperExceptionPage/DeveloperExceptionPageMiddlewareImpl.cs#L174
+        // this makes sure that top-level properties on the payload object are always preserved.
+#if NET6_0_OR_GREATER
+        [UnconditionalSuppressMessage("Trimming", "IL2026", Justification = "The ASP.NET Core framework guarantees that top level properties are preserved")]
+#endif
+        static bool TryFetchException(object payload, out Exception exc)
+            => ExceptionPropertyFetcher.TryFetch(payload, out exc) && exc != null;
+#if NET6_0_OR_GREATER
+        [UnconditionalSuppressMessage("Trimming", "IL2026", Justification = "The ASP.NET Core framework guarantees that top level properties are preserved")]
+#endif
+        static bool TryFetchHttpContext(object payload, out HttpContext ctx)
+            => HttpContextPropertyFetcher.TryFetch(payload, out ctx) && ctx != null;
     }
 
     public void OnEventWritten_Old(string name, object payload)
     {
         var context = payload as HttpContext;
+
         if (context == null)
         {
             AspNetCoreInstrumentationEventSource.Log.NullPayload(nameof(HttpInMetricsListener), EventName, HttpServerDurationMetricName);
@@ -170,6 +218,10 @@ public void OnEventWritten_New(string name, object payload)
             tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeHttpRoute, route));
         }
 #endif
+        if (context.Items.TryGetValue(ErrorTypeHttpContextItemsKey, out var errorType))
+        {
+            tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeErrorType, errorType));
+        }
 
         // We are relying here on ASP.NET Core to set duration before writing the stop event.
         // https://github.com/dotnet/aspnetcore/blob/d6fa351048617ae1c8b47493ba1abbe94c3a24cf/src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs#L449

src/Shared/SemanticConventions.cs

@@ -110,6 +110,7 @@ internal static class SemanticConventions
     public const string AttributeExceptionType = "exception.type";
     public const string AttributeExceptionMessage = "exception.message";
     public const string AttributeExceptionStacktrace = "exception.stacktrace";
+    public const string AttributeErrorType = "error.type";
 
     // v1.21.0
     // https://github.com/open-telemetry/semantic-conventions/blob/v1.21.0/docs/http/http-spans.md

test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests_New.cs

@@ -39,8 +39,8 @@ public IncomingRequestsCollectionsIsAccordingToTheSpecTests_New(WebApplicationFa
     }
 
     [Theory]
-    [InlineData("/api/values", null, "user-agent", 503, "503")]
-    [InlineData("/api/values", "?query=1", null, 503, null)]
+    [InlineData("/api/values", null, "user-agent", 200, null)]
+    [InlineData("/api/values", "?query=1", null, 200, null)]
     [InlineData("/api/exception", null, null, 503, null)]
     [InlineData("/api/exception", null, null, 503, null, true)]
     public async Task SuccessfulTemplateControllerCallGeneratesASpan_New(
@@ -123,6 +123,7 @@ public async Task SuccessfulTemplateControllerCallGeneratesASpan_New(
             if (statusCode == 503)
             {
                 Assert.Equal(ActivityStatusCode.Error, activity.Status);
+                Assert.Equal("System.Exception", activity.GetTagValue(SemanticConventions.AttributeErrorType));
             }
             else
             {

test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs

@@ -185,8 +185,10 @@ public async Task ValidateNet8RateLimitingMetricsAsync()
     }
 #endif
 
-    [Fact]
-    public async Task RequestMetricIsCaptured_New()
+    [Theory]
+    [InlineData("/api/values/2", "api/Values/{id}", null, 200)]
+    [InlineData("/api/Error", "api/Error", "System.Exception", 500)]
+    public async Task RequestMetricIsCaptured_New(string api, string expectedRoute, string expectedErrorType, int expectedStatusCode)
     {
         var configuration = new ConfigurationBuilder()
             .AddInMemoryCollection(new Dictionary<string, string> { [SemanticConventionOptInKeyName] = "http" })
@@ -207,11 +209,15 @@ public async Task RequestMetricIsCaptured_New()
             })
             .CreateClient())
         {
-            using var response1 = await client.GetAsync("/api/values").ConfigureAwait(false);
-            using var response2 = await client.GetAsync("/api/values/2").ConfigureAwait(false);
-
-            response1.EnsureSuccessStatusCode();
-            response2.EnsureSuccessStatusCode();
+            try
+            {
+                using var response = await client.GetAsync(api).ConfigureAwait(false);
+                response.EnsureSuccessStatusCode();
+            }
+            catch
+            {
+                // ignore error.
+            }
         }
 
         // We need to let End callback execute as it is executed AFTER response was returned.
@@ -229,12 +235,14 @@ public async Task RequestMetricIsCaptured_New()
 
         Assert.Equal("s", metric.Unit);
         var metricPoints = GetMetricPoints(metric);
-        Assert.Equal(2, metricPoints.Count);
+        Assert.Single(metricPoints);
 
         AssertMetricPoints_New(
             metricPoints: metricPoints,
-            expectedRoutes: new List<string> { "api/Values", "api/Values/{id}" },
-            expectedTagsCount: 6);
+            expectedRoutes: new List<string> { expectedRoute },
+            expectedErrorType,
+            expectedStatusCode,
+            expectedTagsCount: expectedErrorType == null ? 6 : 7);
     }
 
     [Theory]
@@ -430,6 +438,8 @@ public async Task RequestMetricIsCaptured_Dup()
         AssertMetricPoints_New(
             metricPoints: metricPoints,
             expectedRoutes: new List<string> { "api/Values", "api/Values/{id}" },
+            null,
+            200,
             expectedTagsCount: 6);
     }
 #endif
@@ -456,6 +466,8 @@ private static List<MetricPoint> GetMetricPoints(Metric metric)
     private static void AssertMetricPoints_New(
         List<MetricPoint> metricPoints,
         List<string> expectedRoutes,
+        string expectedErrorType,
+        int expectedStatusCode,
         int expectedTagsCount)
     {
         // Assert that one MetricPoint exists for each ExpectedRoute
@@ -476,7 +488,7 @@ private static void AssertMetricPoints_New(
 
             if (metricPoint.HasValue)
             {
-                AssertMetricPoint_New(metricPoint.Value, expectedRoute, expectedTagsCount);
+                AssertMetricPoint_New(metricPoint.Value, expectedStatusCode, expectedRoute, expectedErrorType, expectedTagsCount);
             }
             else
             {
@@ -519,8 +531,10 @@ private static void AssertMetricPoints_Old(
 
     private static KeyValuePair<string, object>[] AssertMetricPoint_New(
         MetricPoint metricPoint,
-        string expectedRoute = "api/Values",
-        int expectedTagsCount = StandardTagsCount)
+        int expectedStatusCode,
+        string expectedRoute,
+        string expectedErrorType,
+        int expectedTagsCount)
     {
         var count = metricPoint.GetHistogramCount();
         var sum = metricPoint.GetHistogramSum();
@@ -540,7 +554,7 @@ private static KeyValuePair<string, object>[] AssertMetricPoint_New(
 
         var method = new KeyValuePair<string, object>(SemanticConventions.AttributeHttpRequestMethod, "GET");
         var scheme = new KeyValuePair<string, object>(SemanticConventions.AttributeUrlScheme, "http");
-        var statusCode = new KeyValuePair<string, object>(SemanticConventions.AttributeHttpResponseStatusCode, 200);
+        var statusCode = new KeyValuePair<string, object>(SemanticConventions.AttributeHttpResponseStatusCode, expectedStatusCode);
         var flavor = new KeyValuePair<string, object>(SemanticConventions.AttributeNetworkProtocolVersion, "1.1");
         var route = new KeyValuePair<string, object>(SemanticConventions.AttributeHttpRoute, expectedRoute);
         Assert.Contains(method, attributes);
@@ -549,6 +563,18 @@ private static KeyValuePair<string, object>[] AssertMetricPoint_New(
         Assert.Contains(flavor, attributes);
         Assert.Contains(route, attributes);
 
+        if (expectedErrorType != null)
+        {
+#if NET8_0_OR_GREATER
+            // Expected to change in next release
+            // https://github.com/dotnet/aspnetcore/issues/51029
+            var errorType = new KeyValuePair<string, object>("exception.type", expectedErrorType);
+#else
+            var errorType = new KeyValuePair<string, object>(SemanticConventions.AttributeErrorType, expectedErrorType);
+#endif
+            Assert.Contains(errorType, attributes);
+        }
+
         // Inspect Histogram Bounds
         var histogramBuckets = metricPoint.GetHistogramBuckets();
         var histogramBounds = new List<double>();