diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreMetrics.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreMetrics.cs index 64fb588a208..7c2879b29d3 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreMetrics.cs +++ b/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 isEnabled = (eventName, _, _) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md index 72f9147b46e..4091d5b9f1f 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md +++ b/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 diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs index 83ba919385b..fbf39179482 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs +++ b/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); diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs index d78628f3b5d..5d8db405158 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs +++ b/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 ExceptionPropertyFetcher = new("Exception"); + private static readonly PropertyFetcher 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(SemanticConventions.AttributeHttpRoute, route)); } #endif + if (context.Items.TryGetValue(ErrorTypeHttpContextItemsKey, out var errorType)) + { + tags.Add(new KeyValuePair(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 diff --git a/src/Shared/SemanticConventions.cs b/src/Shared/SemanticConventions.cs index 26c016b3e1f..5d5563ba1a6 100644 --- a/src/Shared/SemanticConventions.cs +++ b/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 diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests_New.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests_New.cs index ee51eba3c47..cf66df98ee7 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests_New.cs +++ b/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 { diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs index cbe1e012b9e..0185df65271 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs +++ b/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 { [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 { "api/Values", "api/Values/{id}" }, - expectedTagsCount: 6); + expectedRoutes: new List { 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 { "api/Values", "api/Values/{id}" }, + null, + 200, expectedTagsCount: 6); } #endif @@ -456,6 +466,8 @@ private static List GetMetricPoints(Metric metric) private static void AssertMetricPoints_New( List metricPoints, List 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[] 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[] AssertMetricPoint_New( var method = new KeyValuePair(SemanticConventions.AttributeHttpRequestMethod, "GET"); var scheme = new KeyValuePair(SemanticConventions.AttributeUrlScheme, "http"); - var statusCode = new KeyValuePair(SemanticConventions.AttributeHttpResponseStatusCode, 200); + var statusCode = new KeyValuePair(SemanticConventions.AttributeHttpResponseStatusCode, expectedStatusCode); var flavor = new KeyValuePair(SemanticConventions.AttributeNetworkProtocolVersion, "1.1"); var route = new KeyValuePair(SemanticConventions.AttributeHttpRoute, expectedRoute); Assert.Contains(method, attributes); @@ -549,6 +563,18 @@ private static KeyValuePair[] 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("exception.type", expectedErrorType); +#else + var errorType = new KeyValuePair(SemanticConventions.AttributeErrorType, expectedErrorType); +#endif + Assert.Contains(errorType, attributes); + } + // Inspect Histogram Bounds var histogramBuckets = metricPoint.GetHistogramBuckets(); var histogramBounds = new List();