From c4f42edd914d6044935c3b3411acba5361879031 Mon Sep 17 00:00:00 2001 From: Eddy Nakamura Date: Tue, 27 Oct 2020 17:38:47 -0300 Subject: [PATCH 01/10] Enable OnException for AspNetCore instrumentation --- .../AspNetCoreInstrumentationOptions.cs | 5 ++++ .../Implementation/HttpInListener.cs | 24 +++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreInstrumentationOptions.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreInstrumentationOptions.cs index 262b8496140..ad7430f325d 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreInstrumentationOptions.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreInstrumentationOptions.cs @@ -53,5 +53,10 @@ public class AspNetCoreInstrumentationOptions /// The type of this object depends on the event, which is given by the above parameter. /// public Action Enrich { get; set; } + + /// + /// Gets or sets a value indicating whether the exception will be recorded or not. + /// + public bool RecordException { get; set; } } } diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs index 5d9df2b6101..3ab3967a2dc 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs @@ -34,6 +34,7 @@ internal class HttpInListener : ListenerHandler private static readonly Func> HttpRequestHeaderValuesGetter = (request, name) => request.Headers[name]; private readonly PropertyFetcher startContextFetcher = new PropertyFetcher("HttpContext"); private readonly PropertyFetcher stopContextFetcher = new PropertyFetcher("HttpContext"); + private readonly PropertyFetcher stopExceptionFetcher = new PropertyFetcher("Exception"); private readonly PropertyFetcher beforeActionActionDescriptorFetcher = new PropertyFetcher("actionDescriptor"); private readonly PropertyFetcher beforeActionAttributeRouteInfoFetcher = new PropertyFetcher("AttributeRouteInfo"); private readonly PropertyFetcher beforeActionTemplateFetcher = new PropertyFetcher("Template"); @@ -225,6 +226,29 @@ public override void OnCustom(string name, Activity activity, object payload) } } + public override void OnException(Activity activity, object payload) + { + if (activity.IsAllDataRequested && this.options.RecordException) + { + if (!this.stopExceptionFetcher.TryFetch(payload, out Exception exc) || exc == null) + { + AspNetCoreInstrumentationEventSource.Log.NullPayload(nameof(HttpInListener), nameof(this.OnException)); + return; + } + + try + { + this.options.Enrich?.Invoke(activity, "OnException", payload); + } + catch (Exception ex) + { + AspNetCoreInstrumentationEventSource.Log.EnrichmentException(ex); + } + + activity.SetStatus(Status.Error.WithDescription(exc.Message)); + } + } + private static string GetUri(HttpRequest request) { var builder = new StringBuilder(); From 5c55c7e069bfda4b614d818decc166939c2fa5c4 Mon Sep 17 00:00:00 2001 From: Eddy Nakamura Date: Tue, 27 Oct 2020 17:41:01 -0300 Subject: [PATCH 02/10] updating variable to enrich --- .../Implementation/HttpInListener.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs index 3ab3967a2dc..d5337c5a27e 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs @@ -238,7 +238,7 @@ public override void OnException(Activity activity, object payload) try { - this.options.Enrich?.Invoke(activity, "OnException", payload); + this.options.Enrich?.Invoke(activity, "OnException", exc); } catch (Exception ex) { From 90522f8e592cc2aa3310d4a2507a954b911e3304 Mon Sep 17 00:00:00 2001 From: Eddy Nakamura Date: Tue, 27 Oct 2020 17:44:43 -0300 Subject: [PATCH 03/10] updating changelogs --- src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md | 4 ++++ src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md index a3383f554bd..b82e386b2df 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md @@ -2,6 +2,10 @@ ## Unreleased +* Record `Exception` in AspNetCore instrumentation based on `RecordException` in + `AspNetCoreInstrumentationOptions` + ([#1408](https://github.com/open-telemetry/opentelemetry-dotnet/issues/1408)) + ## 0.7.0-beta.1 Released 2020-Oct-16 diff --git a/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md index 89b00879f77..a015ead665a 100644 --- a/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md @@ -5,7 +5,7 @@ * Instrumentation for `HttpWebRequest` no longer store raw objects like `HttpWebRequest` in Activity.CustomProperty. To enrich activity, use the Enrich action on the instrumentation. - ([#1261](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1407)) + ([#1407](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1407)) ## 0.7.0-beta.1 From 23afda87e7ca16a9f535c8a0b697ac3afc7a9053 Mon Sep 17 00:00:00 2001 From: Eddy Nakamura Date: Tue, 27 Oct 2020 18:43:36 -0300 Subject: [PATCH 04/10] recording exceptions --- .../Implementation/HttpInListener.cs | 1 + .../Implementation/HttpHandlerDiagnosticListener.cs | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs index d5337c5a27e..309f1a01066 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs @@ -245,6 +245,7 @@ public override void OnException(Activity activity, object payload) AspNetCoreInstrumentationEventSource.Log.EnrichmentException(ex); } + activity.RecordException(exc); activity.SetStatus(Status.Error.WithDescription(exc.Message)); } } diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs index b4e7e9fbbdc..6d10b76bff9 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs @@ -183,7 +183,7 @@ public override void OnException(Activity activity, object payload) { HttpInstrumentationEventSource.Log.EnrichmentException(ex); } - + activity.RecordException(exc); if (exc is HttpRequestException) { if (exc.InnerException is SocketException exception) From 407a37c48cf56c416433f4cf0af0940ebd3838fa Mon Sep 17 00:00:00 2001 From: Eddy Nakamura Date: Tue, 27 Oct 2020 18:53:00 -0300 Subject: [PATCH 05/10] fixing spacing --- .../Implementation/HttpHandlerDiagnosticListener.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs index 6d10b76bff9..0d5c23914a5 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs @@ -183,7 +183,9 @@ public override void OnException(Activity activity, object payload) { HttpInstrumentationEventSource.Log.EnrichmentException(ex); } + activity.RecordException(exc); + if (exc is HttpRequestException) { if (exc.InnerException is SocketException exception) From c0d6c2766e3760e8939a30f70e235d9687b5d385 Mon Sep 17 00:00:00 2001 From: Eddy Nakamura Date: Tue, 27 Oct 2020 19:12:38 -0300 Subject: [PATCH 06/10] recording exception in activity if record is true --- .../Implementation/HttpInListener.cs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs index 309f1a01066..ed695fa405c 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs @@ -228,7 +228,7 @@ public override void OnCustom(string name, Activity activity, object payload) public override void OnException(Activity activity, object payload) { - if (activity.IsAllDataRequested && this.options.RecordException) + if (activity.IsAllDataRequested) { if (!this.stopExceptionFetcher.TryFetch(payload, out Exception exc) || exc == null) { @@ -245,7 +245,11 @@ public override void OnException(Activity activity, object payload) AspNetCoreInstrumentationEventSource.Log.EnrichmentException(ex); } - activity.RecordException(exc); + if (this.options.RecordException) + { + activity.RecordException(exc); + } + activity.SetStatus(Status.Error.WithDescription(exc.Message)); } } From 3ab821acb5c8e14b24e31b16d304b2028dea2eea Mon Sep 17 00:00:00 2001 From: Eddy Nakamura Date: Tue, 27 Oct 2020 19:31:40 -0300 Subject: [PATCH 07/10] reverting --- .../Implementation/HttpHandlerDiagnosticListener.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs index 0d5c23914a5..b4e7e9fbbdc 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs @@ -184,8 +184,6 @@ public override void OnException(Activity activity, object payload) HttpInstrumentationEventSource.Log.EnrichmentException(ex); } - activity.RecordException(exc); - if (exc is HttpRequestException) { if (exc.InnerException is SocketException exception) From b3868058b5af9808eb329eacf2f8f98d59e60989 Mon Sep 17 00:00:00 2001 From: Eddy Nakamura Date: Tue, 27 Oct 2020 22:00:12 -0300 Subject: [PATCH 08/10] testing exception --- ...estsCollectionsIsAccordingToTheSpecTests.cs | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests.cs index e4f6d49af03..6efa32a71f0 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests.cs @@ -46,6 +46,7 @@ public IncomingRequestsCollectionsIsAccordingToTheSpecTests(WebApplicationFactor [Theory] [InlineData("/api/values", "user-agent", 503, "503")] [InlineData("/api/values", null, 503, null)] + [InlineData("/api/exception", null, 503, null)] public async Task SuccessfulTemplateControllerCallGeneratesASpan( string urlPath, string userAgent, @@ -106,7 +107,7 @@ public async Task SuccessfulTemplateControllerCallGeneratesASpan( if (statusCode == 503) { - Assert.Equal(Status.Error, activity.GetStatus()); + Assert.Equal(Status.Error.StatusCode, activity.GetStatus().StatusCode); } else { @@ -115,7 +116,14 @@ public async Task SuccessfulTemplateControllerCallGeneratesASpan( // Instrumentation is not expected to set status description // as the reason can be inferred from SemanticConventions.AttributeHttpStatusCode - Assert.True(string.IsNullOrEmpty(activity.GetStatus().Description)); + if (!urlPath.EndsWith("exception")) + { + Assert.True(string.IsNullOrEmpty(activity.GetStatus().Description)); + } + else + { + Assert.Equal("exception description", activity.GetStatus().Description); + } this.ValidateTagValue(activity, SemanticConventions.AttributeHttpUserAgent, userAgent); } @@ -148,6 +156,12 @@ public override async Task ProcessAsync(HttpContext context) context.Response.StatusCode = this.statusCode; context.Response.HttpContext.Features.Get().ReasonPhrase = this.reasonPhrase; await context.Response.WriteAsync("empty"); + + if (context.Request.Path.Value.EndsWith("exception")) + { + throw new Exception("exception description"); + } + return false; } } From 89cb4c3e424a97dbaca6be595687d37e27f67420 Mon Sep 17 00:00:00 2001 From: Eddy Nakamura Date: Tue, 27 Oct 2020 22:27:34 -0300 Subject: [PATCH 09/10] testing recordExceptionFlag --- .../AspNetCoreInstrumentationOptions.cs | 5 ++++- ...questsCollectionsIsAccordingToTheSpecTests.cs | 16 ++++++++++++++-- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreInstrumentationOptions.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreInstrumentationOptions.cs index ad7430f325d..2e39619e082 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreInstrumentationOptions.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreInstrumentationOptions.cs @@ -55,8 +55,11 @@ public class AspNetCoreInstrumentationOptions public Action Enrich { get; set; } /// - /// Gets or sets a value indicating whether the exception will be recorded or not. + /// Gets or sets a value indicating whether the exception will be recorded as ActivityEvent or not. /// + /// + /// https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/exceptions.md + /// public bool RecordException { get; set; } } } diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests.cs index 6efa32a71f0..872bb2fdd31 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests.cs @@ -16,6 +16,7 @@ using System; using System.Diagnostics; +using System.Linq; using System.Threading.Tasks; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Http.Features; @@ -47,11 +48,13 @@ public IncomingRequestsCollectionsIsAccordingToTheSpecTests(WebApplicationFactor [InlineData("/api/values", "user-agent", 503, "503")] [InlineData("/api/values", null, 503, null)] [InlineData("/api/exception", null, 503, null)] + [InlineData("/api/exception", null, 503, null, true)] public async Task SuccessfulTemplateControllerCallGeneratesASpan( string urlPath, string userAgent, int statusCode, - string reasonPhrase) + string reasonPhrase, + bool recordException = false) { var processor = new Mock>(); @@ -61,7 +64,7 @@ public async Task SuccessfulTemplateControllerCallGeneratesASpan( builder.ConfigureTestServices((IServiceCollection services) => { services.AddSingleton(new TestCallbackMiddlewareImpl(statusCode, reasonPhrase)); - services.AddOpenTelemetryTracing((builder) => builder.AddAspNetCoreInstrumentation() + services.AddOpenTelemetryTracing((builder) => builder.AddAspNetCoreInstrumentation(options => options.RecordException = recordException) .AddProcessor(processor.Object)); })) .CreateClient()) @@ -125,7 +128,16 @@ public async Task SuccessfulTemplateControllerCallGeneratesASpan( Assert.Equal("exception description", activity.GetStatus().Description); } + if (recordException) + { + Assert.Single(activity.Events); + Assert.Equal("exception", activity.Events.First().Name); + } + this.ValidateTagValue(activity, SemanticConventions.AttributeHttpUserAgent, userAgent); + + activity.Dispose(); + processor.Object.Dispose(); } private void ValidateTagValue(Activity activity, string attribute, string expectedValue) From ea29179c90a533175f89d0527a5e5a10130b45c5 Mon Sep 17 00:00:00 2001 From: Eddy Nakamura Date: Tue, 27 Oct 2020 22:32:39 -0300 Subject: [PATCH 10/10] fixing remarks text --- .../AspNetCoreInstrumentationOptions.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreInstrumentationOptions.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreInstrumentationOptions.cs index 2e39619e082..38e8f552a34 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreInstrumentationOptions.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreInstrumentationOptions.cs @@ -58,7 +58,7 @@ public class AspNetCoreInstrumentationOptions /// Gets or sets a value indicating whether the exception will be recorded as ActivityEvent or not. /// /// - /// https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/exceptions.md + /// https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/exceptions.md. /// public bool RecordException { get; set; } }