From 716c0f55660cad66bd2ddad36758ff5553edc1a9 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Fri, 13 Aug 2021 11:21:46 -0700 Subject: [PATCH 1/5] Update ASP.NET instrumentation to use the new TelemetryHttpModule. --- examples/AspNet/Web.config | 12 +- .../.publicApi/net461/PublicAPI.Unshipped.txt | 18 +- .../ActivityHelper.cs | 7 +- .../TelemetryHttpModule.cs | 43 +--- .../TelemetryHttpModuleOptions.cs | 66 ++++++ .../.publicApi/net461/PublicAPI.Unshipped.txt | 2 + .../AspNetInstrumentation.cs | 15 +- .../AspNetInstrumentationOptions.cs | 8 + .../Implementation/HttpInListener.cs | 193 +++++------------- ...penTelemetry.Instrumentation.AspNet.csproj | 8 +- .../TracerProviderBuilderExtensions.cs | 5 +- 11 files changed, 166 insertions(+), 211 deletions(-) create mode 100644 src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/TelemetryHttpModuleOptions.cs diff --git a/examples/AspNet/Web.config b/examples/AspNet/Web.config index fcf320ae33e..51a2496ecc5 100644 --- a/examples/AspNet/Web.config +++ b/examples/AspNet/Web.config @@ -37,20 +37,16 @@ - - + + - - - - - - + + diff --git a/src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/.publicApi/net461/PublicAPI.Unshipped.txt b/src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/.publicApi/net461/PublicAPI.Unshipped.txt index 04b7b15df9b..367f7a1e6e6 100644 --- a/src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/.publicApi/net461/PublicAPI.Unshipped.txt +++ b/src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/.publicApi/net461/PublicAPI.Unshipped.txt @@ -3,12 +3,14 @@ const OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule.AspNetSourceName OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule.Dispose() -> void OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule.Init(System.Web.HttpApplication context) -> void -OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule.OnExceptionCallback.get -> System.Action -OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule.OnExceptionCallback.set -> void -OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule.OnRequestStartedCallback.get -> System.Action -OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule.OnRequestStartedCallback.set -> void -OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule.OnRequestStoppedCallback.get -> System.Action -OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule.OnRequestStoppedCallback.set -> void OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule.TelemetryHttpModule() -> void -OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule.TextMapPropagator.get -> OpenTelemetry.Context.Propagation.TraceContextPropagator -OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule.TextMapPropagator.set -> void +OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModuleOptions +OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModuleOptions.OnExceptionCallback.get -> System.Action +OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModuleOptions.OnExceptionCallback.set -> void +OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModuleOptions.OnRequestStartedCallback.get -> System.Action +OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModuleOptions.OnRequestStartedCallback.set -> void +OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModuleOptions.OnRequestStoppedCallback.get -> System.Action +OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModuleOptions.OnRequestStoppedCallback.set -> void +OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModuleOptions.TextMapPropagator.get -> OpenTelemetry.Context.Propagation.TextMapPropagator +OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModuleOptions.TextMapPropagator.set -> void +static OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule.Options.get -> OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModuleOptions diff --git a/src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/ActivityHelper.cs b/src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/ActivityHelper.cs index 7675b8e4516..9503e66afd3 100644 --- a/src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/ActivityHelper.cs +++ b/src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/ActivityHelper.cs @@ -34,7 +34,8 @@ internal static class ActivityHelper /// private const string ActivityKey = "__AspnetActivity__"; - private static readonly ActivitySource AspNetSource = new ActivitySource(TelemetryHttpModule.AspNetSourceName); + private static readonly Version Version = typeof(ActivityHelper).Assembly.GetName().Version; + private static readonly ActivitySource AspNetSource = new ActivitySource(TelemetryHttpModule.AspNetSourceName, Version.ToString()); private static readonly Func> HttpRequestHeaderValuesGetter = (request, name) => request.Headers.GetValues(name); private static readonly object StartedButNotSampledObj = new object(); @@ -147,13 +148,13 @@ public static void StopAspNetActivity(Activity aspNetActivity, HttpContext conte } [MethodImpl(MethodImplOptions.AggressiveInlining)] - public static void WriteActivityException(Activity aspNetActivity, Exception exception, Action onExceptionCallback) + public static void WriteActivityException(Activity aspNetActivity, HttpContext context, Exception exception, Action onExceptionCallback) { if (aspNetActivity != null) { try { - onExceptionCallback?.Invoke(aspNetActivity, exception); + onExceptionCallback?.Invoke(aspNetActivity, context, exception); } catch (Exception callbackEx) { diff --git a/src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/TelemetryHttpModule.cs b/src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/TelemetryHttpModule.cs index f6cb354290d..337b956a648 100644 --- a/src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/TelemetryHttpModule.cs +++ b/src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/TelemetryHttpModule.cs @@ -15,11 +15,9 @@ // using System; -using System.ComponentModel; using System.Diagnostics; using System.Reflection; using System.Web; -using OpenTelemetry.Context.Propagation; namespace OpenTelemetry.Instrumentation.AspNet { @@ -46,37 +44,10 @@ public class TelemetryHttpModule : IHttpModule private static readonly MethodInfo OnStepMethodInfo = typeof(HttpApplication).GetMethod("OnExecuteRequestStep"); - private TraceContextPropagator traceContextPropagator = new TraceContextPropagator(); - - /// - /// Gets or sets the to use to - /// extract from incoming requests. - /// - [EditorBrowsable(EditorBrowsableState.Never)] - public TraceContextPropagator TextMapPropagator - { - get => this.traceContextPropagator; - set => this.traceContextPropagator = value ?? throw new ArgumentNullException(nameof(value)); - } - - /// - /// Gets or sets a callback action to be fired when a request is started. - /// - [EditorBrowsable(EditorBrowsableState.Never)] - public Action OnRequestStartedCallback { get; set; } - - /// - /// Gets or sets a callback action to be fired when a request is stopped. - /// - [EditorBrowsable(EditorBrowsableState.Never)] - public Action OnRequestStoppedCallback { get; set; } - /// - /// Gets or sets a callback action to be fired when an unhandled - /// exception is thrown processing a request. + /// Gets the applied to requests processed by the handler. /// - [EditorBrowsable(EditorBrowsableState.Never)] - public Action OnExceptionCallback { get; set; } + public static TelemetryHttpModuleOptions Options { get; } = new TelemetryHttpModuleOptions(); /// public void Dispose() @@ -136,7 +107,7 @@ private void Application_BeginRequest(object sender, EventArgs e) { var context = ((HttpApplication)sender).Context; AspNetTelemetryEventSource.Log.TraceCallback("Application_BeginRequest"); - ActivityHelper.StartAspNetActivity(this.TextMapPropagator, context, this.OnRequestStartedCallback); + ActivityHelper.StartAspNetActivity(Options.TextMapPropagator, context, Options.OnRequestStartedCallback); } private void Application_PreRequestHandlerExecute(object sender, EventArgs e) @@ -168,13 +139,13 @@ private void Application_EndRequest(object sender, EventArgs e) else { // Activity has never been started - aspNetActivity = ActivityHelper.StartAspNetActivity(this.TextMapPropagator, context, this.OnRequestStartedCallback); + aspNetActivity = ActivityHelper.StartAspNetActivity(Options.TextMapPropagator, context, Options.OnRequestStartedCallback); } } if (trackActivity) { - ActivityHelper.StopAspNetActivity(aspNetActivity, context, this.OnRequestStoppedCallback); + ActivityHelper.StopAspNetActivity(aspNetActivity, context, Options.OnRequestStoppedCallback); } } @@ -189,10 +160,10 @@ private void Application_Error(object sender, EventArgs e) { if (!ActivityHelper.HasStarted(context, out Activity aspNetActivity)) { - aspNetActivity = ActivityHelper.StartAspNetActivity(this.TextMapPropagator, context, this.OnRequestStartedCallback); + aspNetActivity = ActivityHelper.StartAspNetActivity(Options.TextMapPropagator, context, Options.OnRequestStartedCallback); } - ActivityHelper.WriteActivityException(aspNetActivity, exception, this.OnExceptionCallback); + ActivityHelper.WriteActivityException(aspNetActivity, context, exception, Options.OnExceptionCallback); } } } diff --git a/src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/TelemetryHttpModuleOptions.cs b/src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/TelemetryHttpModuleOptions.cs new file mode 100644 index 00000000000..ec87eba38b2 --- /dev/null +++ b/src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/TelemetryHttpModuleOptions.cs @@ -0,0 +1,66 @@ +// +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +using System; +using System.ComponentModel; +using System.Diagnostics; +using System.Web; +using OpenTelemetry.Context.Propagation; + +namespace OpenTelemetry.Instrumentation.AspNet +{ + /// + /// Stores options for the . + /// + public class TelemetryHttpModuleOptions + { + private TextMapPropagator textMapPropagator = new TraceContextPropagator(); + + internal TelemetryHttpModuleOptions() + { + } + + /// + /// Gets or sets the to use to + /// extract from incoming requests. + /// + [EditorBrowsable(EditorBrowsableState.Never)] + public TextMapPropagator TextMapPropagator + { + get => this.textMapPropagator; + set => this.textMapPropagator = value ?? throw new ArgumentNullException(nameof(value)); + } + + /// + /// Gets or sets a callback action to be fired when a request is started. + /// + [EditorBrowsable(EditorBrowsableState.Never)] + public Action OnRequestStartedCallback { get; set; } + + /// + /// Gets or sets a callback action to be fired when a request is stopped. + /// + [EditorBrowsable(EditorBrowsableState.Never)] + public Action OnRequestStoppedCallback { get; set; } + + /// + /// Gets or sets a callback action to be fired when an unhandled + /// exception is thrown processing a request. + /// + [EditorBrowsable(EditorBrowsableState.Never)] + public Action OnExceptionCallback { get; set; } + } +} diff --git a/src/OpenTelemetry.Instrumentation.AspNet/.publicApi/net461/PublicAPI.Unshipped.txt b/src/OpenTelemetry.Instrumentation.AspNet/.publicApi/net461/PublicAPI.Unshipped.txt index 569931e171a..35b5f87638f 100644 --- a/src/OpenTelemetry.Instrumentation.AspNet/.publicApi/net461/PublicAPI.Unshipped.txt +++ b/src/OpenTelemetry.Instrumentation.AspNet/.publicApi/net461/PublicAPI.Unshipped.txt @@ -4,5 +4,7 @@ OpenTelemetry.Instrumentation.AspNet.AspNetInstrumentationOptions.Enrich.get -> OpenTelemetry.Instrumentation.AspNet.AspNetInstrumentationOptions.Enrich.set -> void OpenTelemetry.Instrumentation.AspNet.AspNetInstrumentationOptions.Filter.get -> System.Func OpenTelemetry.Instrumentation.AspNet.AspNetInstrumentationOptions.Filter.set -> void +OpenTelemetry.Instrumentation.AspNet.AspNetInstrumentationOptions.RecordException.get -> bool +OpenTelemetry.Instrumentation.AspNet.AspNetInstrumentationOptions.RecordException.set -> void OpenTelemetry.Trace.TracerProviderBuilderExtensions static OpenTelemetry.Trace.TracerProviderBuilderExtensions.AddAspNetInstrumentation(this OpenTelemetry.Trace.TracerProviderBuilder builder, System.Action configureAspNetInstrumentationOptions = null) -> OpenTelemetry.Trace.TracerProviderBuilder diff --git a/src/OpenTelemetry.Instrumentation.AspNet/AspNetInstrumentation.cs b/src/OpenTelemetry.Instrumentation.AspNet/AspNetInstrumentation.cs index 3360b08abea..2bd2a988be7 100644 --- a/src/OpenTelemetry.Instrumentation.AspNet/AspNetInstrumentation.cs +++ b/src/OpenTelemetry.Instrumentation.AspNet/AspNetInstrumentation.cs @@ -13,6 +13,7 @@ // See the License for the specific language governing permissions and // limitations under the License. // + using System; using OpenTelemetry.Instrumentation.AspNet.Implementation; @@ -21,11 +22,9 @@ namespace OpenTelemetry.Instrumentation.AspNet /// /// Asp.Net Requests instrumentation. /// - internal class AspNetInstrumentation : IDisposable + internal sealed class AspNetInstrumentation : IDisposable { - internal const string AspNetDiagnosticListenerName = "OpenTelemetry.Instrumentation.AspNet.Telemetry"; - - private readonly DiagnosticSourceSubscriber diagnosticSourceSubscriber; + private readonly HttpInListener httpInListener; /// /// Initializes a new instance of the class. @@ -33,17 +32,13 @@ internal class AspNetInstrumentation : IDisposable /// Configuration options for ASP.NET instrumentation. public AspNetInstrumentation(AspNetInstrumentationOptions options) { - this.diagnosticSourceSubscriber = new DiagnosticSourceSubscriber( - name => new HttpInListener(name, options), - listener => listener.Name == AspNetDiagnosticListenerName, - null); - this.diagnosticSourceSubscriber.Subscribe(); + this.httpInListener = new HttpInListener(options); } /// public void Dispose() { - this.diagnosticSourceSubscriber?.Dispose(); + this.httpInListener?.Dispose(); } } } diff --git a/src/OpenTelemetry.Instrumentation.AspNet/AspNetInstrumentationOptions.cs b/src/OpenTelemetry.Instrumentation.AspNet/AspNetInstrumentationOptions.cs index 613b3f922f7..c75b07ae37b 100644 --- a/src/OpenTelemetry.Instrumentation.AspNet/AspNetInstrumentationOptions.cs +++ b/src/OpenTelemetry.Instrumentation.AspNet/AspNetInstrumentationOptions.cs @@ -43,5 +43,13 @@ public class AspNetInstrumentationOptions /// 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 as ActivityEvent or not. + /// + /// + /// https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/exceptions.md. + /// + public bool RecordException { get; set; } } } diff --git a/src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInListener.cs index 07867a036c1..ef9cf68839e 100644 --- a/src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInListener.cs @@ -15,9 +15,7 @@ // using System; -using System.Collections.Generic; using System.Diagnostics; -using System.Reflection; using System.Web; using System.Web.Routing; using OpenTelemetry.Context.Propagation; @@ -25,91 +23,52 @@ namespace OpenTelemetry.Instrumentation.AspNet.Implementation { - internal class HttpInListener : ListenerHandler + internal sealed class HttpInListener : IDisposable { - internal const string ActivityNameByHttpInListener = "ActivityCreatedByHttpInListener"; - internal const string ActivityOperationName = "Microsoft.AspNet.HttpReqIn"; - internal static readonly AssemblyName AssemblyName = typeof(HttpInListener).Assembly.GetName(); - internal static readonly string ActivitySourceName = AssemblyName.Name; - internal static readonly Version Version = AssemblyName.Version; - internal static readonly ActivitySource ActivitySource = new ActivitySource(ActivitySourceName, Version.ToString()); - private static readonly Func> HttpRequestHeaderValuesGetter = (request, name) => request.Headers.GetValues(name); private readonly PropertyFetcher routeFetcher = new PropertyFetcher("Route"); private readonly PropertyFetcher routeTemplateFetcher = new PropertyFetcher("RouteTemplate"); private readonly AspNetInstrumentationOptions options; - public HttpInListener(string name, AspNetInstrumentationOptions options) - : base(name) + public HttpInListener(AspNetInstrumentationOptions options) { this.options = options ?? throw new ArgumentNullException(nameof(options)); - } - [System.Diagnostics.CodeAnalysis.SuppressMessage("Reliability", "CA2000:Dispose objects before losing scope", Justification = "Activity is retrieved from Activity.Current later and disposed.")] - public override void OnStartActivity(Activity activity, object payload) - { - // The overall flow of what AspNet library does is as below: - // Activity.Start() - // DiagnosticSource.WriteEvent("Start", payload) - // DiagnosticSource.WriteEvent("Stop", payload) - // Activity.Stop() + TelemetryHttpModule.Options.TextMapPropagator = Propagators.DefaultTextMapPropagator; - // This method is in the WriteEvent("Start", payload) path. - // By this time, samplers have already run and - // activity.IsAllDataRequested populated accordingly. + TelemetryHttpModule.Options.OnRequestStartedCallback += this.OnStartActivity; + TelemetryHttpModule.Options.OnRequestStoppedCallback += this.OnStopActivity; + TelemetryHttpModule.Options.OnExceptionCallback += this.OnException; + } - if (Sdk.SuppressInstrumentation) - { - return; - } + public void Dispose() + { + TelemetryHttpModule.Options.OnRequestStartedCallback -= this.OnStartActivity; + TelemetryHttpModule.Options.OnRequestStoppedCallback -= this.OnStopActivity; + TelemetryHttpModule.Options.OnExceptionCallback -= this.OnException; + } - // Ensure context extraction irrespective of sampling decision - var context = HttpContext.Current; - if (context == null) + /// + /// Gets the OpenTelemetry standard uri tag value for a span based on its request . + /// + /// . + /// Span uri value. + private static string GetUriTagValueFromRequestUri(Uri uri) + { + if (string.IsNullOrEmpty(uri.UserInfo)) { - AspNetInstrumentationEventSource.Log.NullPayload(nameof(HttpInListener), nameof(this.OnStartActivity)); - return; + return uri.ToString(); } - var request = context.Request; - var requestValues = request.Unvalidated; - var textMapPropagator = Propagators.DefaultTextMapPropagator; - - if (!(textMapPropagator is TraceContextPropagator)) - { - var ctx = textMapPropagator.Extract(default, request, HttpRequestHeaderValuesGetter); - - if (ctx.ActivityContext.IsValid() - && ctx.ActivityContext != new ActivityContext(activity.TraceId, activity.ParentSpanId, activity.ActivityTraceFlags, activity.TraceStateString, true)) - { - // Create a new activity with its parent set from the extracted context. - // This makes the new activity as a "sibling" of the activity created by - // ASP.NET. - Activity newOne = new Activity(ActivityNameByHttpInListener); - newOne.SetParentId(ctx.ActivityContext.TraceId, ctx.ActivityContext.SpanId, ctx.ActivityContext.TraceFlags); - newOne.TraceStateString = ctx.ActivityContext.TraceState; - - // Starting the new activity make it the Activity.Current one. - newOne.Start(); - - // Both new activity and old one store the other activity - // inside them. This is required in the Stop step to - // correctly stop and restore Activity.Current. - newOne.SetCustomProperty("OTel.ActivityByAspNet", activity); - activity.SetCustomProperty("OTel.ActivityByHttpInListener", newOne); - - // Set IsAllDataRequested to false for the activity created by the framework to only export the sibling activity and not the framework activity - activity.IsAllDataRequested = false; - activity = newOne; - } - - if (ctx.Baggage != default) - { - Baggage.Current = ctx.Baggage; - } - } + return string.Concat(uri.Scheme, Uri.SchemeDelimiter, uri.Authority, uri.PathAndQuery, uri.Fragment); + } + private void OnStartActivity(Activity activity, HttpContext context) + { if (activity.IsAllDataRequested) { + var request = context.Request; + var requestValues = request.Unvalidated; + try { if (this.options.Filter?.Invoke(context) == false) @@ -128,9 +87,6 @@ public override void OnStartActivity(Activity activity, object payload) return; } - ActivityInstrumentationHelper.SetActivitySourceProperty(activity, ActivitySource); - ActivityInstrumentationHelper.SetKindProperty(activity, ActivityKind.Server); - // see the spec https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/http.md var path = requestValues.Path; activity.DisplayName = path; @@ -160,47 +116,17 @@ public override void OnStartActivity(Activity activity, object payload) } } - public override void OnStopActivity(Activity activity, object payload) + private void OnStopActivity(Activity activity, HttpContext context) { - Activity activityToEnrich = activity; - Activity createdActivity = null; - - var textMapPropagator = Propagators.DefaultTextMapPropagator; - bool isCustomPropagator = !(textMapPropagator is TraceContextPropagator); - - if (isCustomPropagator) - { - // If using custom context propagator, then the activity here - // could be either the one from Asp.Net, or the one - // this instrumentation created in Start. - // This is because Asp.Net, under certain circumstances, restores Activity.Current - // to its own activity. - if (activity.OperationName.Equals(ActivityOperationName, StringComparison.Ordinal)) - { - // This block is hit if Asp.Net did restore Current to its own activity, - // and we need to retrieve the one created by HttpInListener, - // or an additional activity was never created. - createdActivity = (Activity)activity.GetCustomProperty("OTel.ActivityByHttpInListener"); - activityToEnrich = createdActivity ?? activity; - } - } - - if (activityToEnrich.IsAllDataRequested) + if (activity.IsAllDataRequested) { - var context = HttpContext.Current; - if (context == null) - { - AspNetInstrumentationEventSource.Log.NullPayload(nameof(HttpInListener), nameof(this.OnStopActivity)); - return; - } - var response = context.Response; - activityToEnrich.SetTag(SemanticConventions.AttributeHttpStatusCode, response.StatusCode); + activity.SetTag(SemanticConventions.AttributeHttpStatusCode, response.StatusCode); - if (activityToEnrich.GetStatus().StatusCode == StatusCode.Unset) + if (activity.GetStatus().StatusCode == StatusCode.Unset) { - activityToEnrich.SetStatus(SpanHelper.ResolveSpanStatusForHttpStatusCode(response.StatusCode)); + activity.SetStatus(SpanHelper.ResolveSpanStatusForHttpStatusCode(response.StatusCode)); } var routeData = context.Request.RequestContext.RouteData; @@ -227,58 +153,45 @@ public override void OnStopActivity(Activity activity, object payload) if (!string.IsNullOrEmpty(template)) { // Override the name that was previously set to the path part of URL. - activityToEnrich.DisplayName = template; - activityToEnrich.SetTag(SemanticConventions.AttributeHttpRoute, template); + activity.DisplayName = template; + activity.SetTag(SemanticConventions.AttributeHttpRoute, template); } try { - this.options.Enrich?.Invoke(activityToEnrich, "OnStopActivity", response); + this.options.Enrich?.Invoke(activity, "OnStopActivity", response); } catch (Exception ex) { AspNetInstrumentationEventSource.Log.EnrichmentException(ex); } } + } - if (isCustomPropagator) + private void OnException(Activity activity, HttpContext context, Exception exception) + { + if (activity.IsAllDataRequested) { - if (activity.OperationName.Equals(ActivityNameByHttpInListener, StringComparison.Ordinal)) + if (this.options.RecordException) { - // If instrumentation started a new Activity, it must - // be stopped here. - activity.Stop(); + activity.RecordException(exception); + } - // Restore the original activity as Current. - var activityByAspNet = (Activity)activity.GetCustomProperty("OTel.ActivityByAspNet"); - Activity.Current = activityByAspNet; + activity.SetStatus(Status.Error.WithDescription(exception.Message)); + + try + { + this.options.Enrich?.Invoke(activity, "OnException", exception); } - else if (createdActivity != null) + catch (Exception ex) { - // This block is hit if Asp.Net did restore Current to its own activity, - // then we need to retrieve the one created by HttpInListener - // and stop it. - createdActivity.Stop(); - - // Restore current back to the one created by Asp.Net - Activity.Current = activity; + AspNetInstrumentationEventSource.Log.EnrichmentException(ex); } } - } - - /// - /// Gets the OpenTelemetry standard uri tag value for a span based on its request . - /// - /// . - /// Span uri value. - private static string GetUriTagValueFromRequestUri(Uri uri) - { - if (string.IsNullOrEmpty(uri.UserInfo)) + else { - return uri.ToString(); + activity.SetStatus(Status.Error); } - - return string.Concat(uri.Scheme, Uri.SchemeDelimiter, uri.Authority, uri.PathAndQuery, uri.Fragment); } } } diff --git a/src/OpenTelemetry.Instrumentation.AspNet/OpenTelemetry.Instrumentation.AspNet.csproj b/src/OpenTelemetry.Instrumentation.AspNet/OpenTelemetry.Instrumentation.AspNet.csproj index 46790741b95..353cf46280b 100644 --- a/src/OpenTelemetry.Instrumentation.AspNet/OpenTelemetry.Instrumentation.AspNet.csproj +++ b/src/OpenTelemetry.Instrumentation.AspNet/OpenTelemetry.Instrumentation.AspNet.csproj @@ -3,7 +3,7 @@ net461 ASP.NET instrumentation for OpenTelemetry .NET $(PackageTags);distributed-tracing;AspNet;MVC;WebAPI - true + true @@ -11,7 +11,11 @@ - + + + + + diff --git a/src/OpenTelemetry.Instrumentation.AspNet/TracerProviderBuilderExtensions.cs b/src/OpenTelemetry.Instrumentation.AspNet/TracerProviderBuilderExtensions.cs index 372c7c38731..035b6c9e8da 100644 --- a/src/OpenTelemetry.Instrumentation.AspNet/TracerProviderBuilderExtensions.cs +++ b/src/OpenTelemetry.Instrumentation.AspNet/TracerProviderBuilderExtensions.cs @@ -16,7 +16,6 @@ using System; using OpenTelemetry.Instrumentation.AspNet; -using OpenTelemetry.Instrumentation.AspNet.Implementation; namespace OpenTelemetry.Trace { @@ -44,9 +43,7 @@ public static TracerProviderBuilder AddAspNetInstrumentation( configureAspNetInstrumentationOptions?.Invoke(aspnetOptions); builder.AddInstrumentation(() => new AspNetInstrumentation(aspnetOptions)); - builder.AddSource(HttpInListener.ActivitySourceName); - builder.AddLegacySource("Microsoft.AspNet.HttpReqIn"); // for the activities created by AspNetCore - builder.AddLegacySource("ActivityCreatedByHttpInListener"); // for the sibling activities created by the instrumentation library + builder.AddSource(TelemetryHttpModule.AspNetSourceName); return builder; } From c462dc52eb4a259b0bfbcb2fd874ff3cb343a512 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Fri, 13 Aug 2021 11:57:10 -0700 Subject: [PATCH 2/5] Fixed TelemetryHttpModule not starting its Activity objects. Added an example of request suppression. --- examples/AspNet/Examples.AspNet.csproj | 3 +- .../SuppressInstrumentationHttpModule.cs | 58 +++++++++++++++++++ examples/AspNet/Web.config | 1 + .../ActivityHelper.cs | 2 +- 4 files changed, 62 insertions(+), 2 deletions(-) create mode 100644 examples/AspNet/SuppressInstrumentationHttpModule.cs diff --git a/examples/AspNet/Examples.AspNet.csproj b/examples/AspNet/Examples.AspNet.csproj index b67f0c72313..fef00f55b5e 100644 --- a/examples/AspNet/Examples.AspNet.csproj +++ b/examples/AspNet/Examples.AspNet.csproj @@ -61,6 +61,7 @@ + @@ -149,4 +150,4 @@ - + \ No newline at end of file diff --git a/examples/AspNet/SuppressInstrumentationHttpModule.cs b/examples/AspNet/SuppressInstrumentationHttpModule.cs new file mode 100644 index 00000000000..fb832109182 --- /dev/null +++ b/examples/AspNet/SuppressInstrumentationHttpModule.cs @@ -0,0 +1,58 @@ +// +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +using System; +using System.Web; +using OpenTelemetry; + +namespace Examples.AspNet +{ + /// + /// A demo which will suppress ASP.NET + /// instrumentation if a request contains "suppress=true" on the query + /// string. Suppressed spans will not be processed/exported by the + /// OpenTelemetry SDK. + /// + public class SuppressInstrumentationHttpModule : IHttpModule + { + private IDisposable suppressionScope; + + public void Init(HttpApplication context) + { + context.BeginRequest += this.Application_BeginRequest; + context.EndRequest += this.Application_EndRequest; + } + + public void Dispose() + { + } + + private void Application_BeginRequest(object sender, EventArgs e) + { + var context = ((HttpApplication)sender).Context; + + if (context.Request.QueryString["suppress"] == "true") + { + this.suppressionScope = SuppressInstrumentationScope.Begin(); + } + } + + private void Application_EndRequest(object sender, EventArgs e) + { + this.suppressionScope?.Dispose(); + } + } +} diff --git a/examples/AspNet/Web.config b/examples/AspNet/Web.config index 51a2496ecc5..0ee991a134e 100644 --- a/examples/AspNet/Web.config +++ b/examples/AspNet/Web.config @@ -23,6 +23,7 @@ + diff --git a/src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/ActivityHelper.cs b/src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/ActivityHelper.cs index 9503e66afd3..29711309204 100644 --- a/src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/ActivityHelper.cs +++ b/src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/ActivityHelper.cs @@ -72,7 +72,7 @@ public static Activity StartAspNetActivity(TextMapPropagator textMapPropagator, { PropagationContext propagationContext = textMapPropagator.Extract(default, context.Request, HttpRequestHeaderValuesGetter); - Activity activity = AspNetSource.CreateActivity(TelemetryHttpModule.AspNetActivityName, ActivityKind.Server, propagationContext.ActivityContext); + Activity activity = AspNetSource.StartActivity(TelemetryHttpModule.AspNetActivityName, ActivityKind.Server, propagationContext.ActivityContext); if (activity != null) { From ffeb09b527873908a9ced79d43f997fd8447c24f Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Fri, 13 Aug 2021 12:49:11 -0700 Subject: [PATCH 3/5] Tweaks an logging improvements. --- .../AspNetTelemetryEventSource.cs | 5 ++- ...entation.AspNet.TelemetryHttpModule.csproj | 4 ++ .../TelemetryHttpModuleOptions.cs | 5 --- .../AspNetInstrumentationOptions.cs | 18 ++++++--- .../AspNetInstrumentationEventSource.cs | 40 +++++++++---------- .../Implementation/HttpInListener.cs | 20 ++++++---- 6 files changed, 53 insertions(+), 39 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/AspNetTelemetryEventSource.cs b/src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/AspNetTelemetryEventSource.cs index 38c6cb452ab..9fa3a5ef871 100644 --- a/src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/AspNetTelemetryEventSource.cs +++ b/src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/AspNetTelemetryEventSource.cs @@ -17,6 +17,7 @@ using System; using System.Diagnostics; using System.Diagnostics.Tracing; +using OpenTelemetry.Internal; namespace OpenTelemetry.Instrumentation.AspNet { @@ -63,7 +64,7 @@ public void ActivityException(Activity activity, Exception ex) { if (this.IsEnabled(EventLevel.Error, EventKeywords.All)) { - this.ActivityException(activity?.Id, ex.ToString()); + this.ActivityException(activity?.Id, ex.ToInvariantString()); } } @@ -72,7 +73,7 @@ public void CallbackException(Activity activity, string eventName, Exception ex) { if (this.IsEnabled(EventLevel.Error, EventKeywords.All)) { - this.CallbackException(activity?.Id, eventName, ex.ToString()); + this.CallbackException(activity?.Id, eventName, ex.ToInvariantString()); } } diff --git a/src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule.csproj b/src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule.csproj index 9f9c307f674..9ac9a472c1f 100644 --- a/src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule.csproj +++ b/src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule.csproj @@ -12,6 +12,10 @@ false + + + + diff --git a/src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/TelemetryHttpModuleOptions.cs b/src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/TelemetryHttpModuleOptions.cs index ec87eba38b2..5110bfeb769 100644 --- a/src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/TelemetryHttpModuleOptions.cs +++ b/src/OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule/TelemetryHttpModuleOptions.cs @@ -15,7 +15,6 @@ // using System; -using System.ComponentModel; using System.Diagnostics; using System.Web; using OpenTelemetry.Context.Propagation; @@ -37,7 +36,6 @@ internal TelemetryHttpModuleOptions() /// Gets or sets the to use to /// extract from incoming requests. /// - [EditorBrowsable(EditorBrowsableState.Never)] public TextMapPropagator TextMapPropagator { get => this.textMapPropagator; @@ -47,20 +45,17 @@ public TextMapPropagator TextMapPropagator /// /// Gets or sets a callback action to be fired when a request is started. /// - [EditorBrowsable(EditorBrowsableState.Never)] public Action OnRequestStartedCallback { get; set; } /// /// Gets or sets a callback action to be fired when a request is stopped. /// - [EditorBrowsable(EditorBrowsableState.Never)] public Action OnRequestStoppedCallback { get; set; } /// /// Gets or sets a callback action to be fired when an unhandled /// exception is thrown processing a request. /// - [EditorBrowsable(EditorBrowsableState.Never)] public Action OnExceptionCallback { get; set; } } } diff --git a/src/OpenTelemetry.Instrumentation.AspNet/AspNetInstrumentationOptions.cs b/src/OpenTelemetry.Instrumentation.AspNet/AspNetInstrumentationOptions.cs index c75b07ae37b..4209ce23e05 100644 --- a/src/OpenTelemetry.Instrumentation.AspNet/AspNetInstrumentationOptions.cs +++ b/src/OpenTelemetry.Instrumentation.AspNet/AspNetInstrumentationOptions.cs @@ -26,11 +26,19 @@ namespace OpenTelemetry.Instrumentation.AspNet public class AspNetInstrumentationOptions { /// - /// Gets or sets a Filter function that determines whether or not to collect telemetry about requests on a per request basis. - /// The Filter gets the HttpContext, and should return a boolean. - /// If Filter returns true, the request is collected. - /// If Filter returns false or throw exception, the request is filtered out. + /// Gets or sets a filter callback function that determines on a per + /// request basis whether or not to collect telemetry. /// + /// + /// The filter callback receives the for the + /// current request and should return a boolean. + /// + /// If filter returns the request is + /// collected. + /// If filter returns or throws an + /// exception the request is filtered out (NOT collected). + /// + /// public Func Filter { get; set; } /// @@ -48,7 +56,7 @@ public class AspNetInstrumentationOptions /// Gets or sets a value indicating whether the exception will be recorded as ActivityEvent or not. /// /// - /// https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/exceptions.md. + /// See: . /// public bool RecordException { get; set; } } diff --git a/src/OpenTelemetry.Instrumentation.AspNet/Implementation/AspNetInstrumentationEventSource.cs b/src/OpenTelemetry.Instrumentation.AspNet/Implementation/AspNetInstrumentationEventSource.cs index c66228b74df..3dada830225 100644 --- a/src/OpenTelemetry.Instrumentation.AspNet/Implementation/AspNetInstrumentationEventSource.cs +++ b/src/OpenTelemetry.Instrumentation.AspNet/Implementation/AspNetInstrumentationEventSource.cs @@ -29,45 +29,45 @@ internal class AspNetInstrumentationEventSource : EventSource public static AspNetInstrumentationEventSource Log = new AspNetInstrumentationEventSource(); [NonEvent] - public void RequestFilterException(Exception ex) + public void RequestFilterException(string operationName, Exception ex) { if (this.IsEnabled(EventLevel.Error, EventKeywords.All)) { - this.RequestFilterException(ex.ToInvariantString()); + this.RequestFilterException(operationName, ex.ToInvariantString()); } } - [Event(1, Message = "Payload is NULL in event '{1}' from handler '{0}', span will not be recorded.", Level = EventLevel.Warning)] - public void NullPayload(string handlerName, string eventName) + [NonEvent] + public void EnrichmentException(string eventName, Exception ex) { - this.WriteEvent(1, handlerName, eventName); + if (this.IsEnabled(EventLevel.Error, EventKeywords.All)) + { + this.EnrichmentException(eventName, ex.ToInvariantString()); + } } - [Event(2, Message = "Request is filtered out.", Level = EventLevel.Verbose)] - public void RequestIsFilteredOut(string eventName) + [Event(1, Message = "Payload is NULL in event '{1}' from handler '{0}', span will not be recorded.", Level = EventLevel.Warning)] + public void NullPayload(string handlerName, string eventName) { - this.WriteEvent(2, eventName); + this.WriteEvent(1, handlerName, eventName); } - [Event(3, Message = "InstrumentationFilter threw exception. Request will not be collected. Exception {0}.", Level = EventLevel.Error)] - public void RequestFilterException(string exception) + [Event(2, Message = "Request is filtered out and will not be collected. Operation='{0}'", Level = EventLevel.Verbose)] + public void RequestIsFilteredOut(string operationName) { - this.WriteEvent(3, exception); + this.WriteEvent(2, operationName); } - [NonEvent] - public void EnrichmentException(Exception ex) + [Event(3, Message = "InstrumentationFilter threw an exception. Request will not be collected. Operation='{0}': {1}", Level = EventLevel.Error)] + public void RequestFilterException(string operationName, string exception) { - if (this.IsEnabled(EventLevel.Error, EventKeywords.All)) - { - this.EnrichmentException(ex.ToInvariantString()); - } + this.WriteEvent(3, operationName, exception); } - [Event(4, Message = "Enrichment threw exception. Exception {0}.", Level = EventLevel.Error)] - public void EnrichmentException(string exception) + [Event(4, Message = "Enrichment threw an exception. Event='{0}': {1}", Level = EventLevel.Error)] + public void EnrichmentException(string eventName, string exception) { - this.WriteEvent(4, exception); + this.WriteEvent(4, eventName, exception); } } } diff --git a/src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInListener.cs index ef9cf68839e..3253f3c8f10 100644 --- a/src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInListener.cs @@ -66,11 +66,14 @@ private void OnStartActivity(Activity activity, HttpContext context) { if (activity.IsAllDataRequested) { - var request = context.Request; - var requestValues = request.Unvalidated; - try { + // todo: Ideally we would also check + // Sdk.SuppressInstrumentation here to prevent tagging a + // span that will not be collected but we can't do that + // without an SDK reference. Need the spec to come around on + // this. + if (this.options.Filter?.Invoke(context) == false) { AspNetInstrumentationEventSource.Log.RequestIsFilteredOut(activity.OperationName); @@ -81,12 +84,15 @@ private void OnStartActivity(Activity activity, HttpContext context) } catch (Exception ex) { - AspNetInstrumentationEventSource.Log.RequestFilterException(ex); + AspNetInstrumentationEventSource.Log.RequestFilterException(activity.OperationName, ex); activity.IsAllDataRequested = false; activity.ActivityTraceFlags &= ~ActivityTraceFlags.Recorded; return; } + var request = context.Request; + var requestValues = request.Unvalidated; + // see the spec https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/http.md var path = requestValues.Path; activity.DisplayName = path; @@ -111,7 +117,7 @@ private void OnStartActivity(Activity activity, HttpContext context) } catch (Exception ex) { - AspNetInstrumentationEventSource.Log.EnrichmentException(ex); + AspNetInstrumentationEventSource.Log.EnrichmentException("OnStartActivity", ex); } } } @@ -163,7 +169,7 @@ private void OnStopActivity(Activity activity, HttpContext context) } catch (Exception ex) { - AspNetInstrumentationEventSource.Log.EnrichmentException(ex); + AspNetInstrumentationEventSource.Log.EnrichmentException("OnStopActivity", ex); } } } @@ -185,7 +191,7 @@ private void OnException(Activity activity, HttpContext context, Exception excep } catch (Exception ex) { - AspNetInstrumentationEventSource.Log.EnrichmentException(ex); + AspNetInstrumentationEventSource.Log.EnrichmentException("OnException", ex); } } else From ad9a66987286891b1975cc79036d86f739f8b268 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Fri, 13 Aug 2021 12:52:11 -0700 Subject: [PATCH 4/5] Sealed AspNetInstrumentationEventSource. --- .../Implementation/AspNetInstrumentationEventSource.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNet/Implementation/AspNetInstrumentationEventSource.cs b/src/OpenTelemetry.Instrumentation.AspNet/Implementation/AspNetInstrumentationEventSource.cs index 3dada830225..f28310f4f62 100644 --- a/src/OpenTelemetry.Instrumentation.AspNet/Implementation/AspNetInstrumentationEventSource.cs +++ b/src/OpenTelemetry.Instrumentation.AspNet/Implementation/AspNetInstrumentationEventSource.cs @@ -24,7 +24,7 @@ namespace OpenTelemetry.Instrumentation.AspNet.Implementation /// EventSource events emitted from the project. /// [EventSource(Name = "OpenTelemetry-Instrumentation-AspNet")] - internal class AspNetInstrumentationEventSource : EventSource + internal sealed class AspNetInstrumentationEventSource : EventSource { public static AspNetInstrumentationEventSource Log = new AspNetInstrumentationEventSource(); From 6b5069bef50f6096b1366759425f17843cfb1788 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Fri, 13 Aug 2021 16:31:51 -0700 Subject: [PATCH 5/5] Code review. --- .../AspNetInstrumentationEventSource.cs | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNet/Implementation/AspNetInstrumentationEventSource.cs b/src/OpenTelemetry.Instrumentation.AspNet/Implementation/AspNetInstrumentationEventSource.cs index f28310f4f62..e7184746b22 100644 --- a/src/OpenTelemetry.Instrumentation.AspNet/Implementation/AspNetInstrumentationEventSource.cs +++ b/src/OpenTelemetry.Instrumentation.AspNet/Implementation/AspNetInstrumentationEventSource.cs @@ -46,28 +46,22 @@ public void EnrichmentException(string eventName, Exception ex) } } - [Event(1, Message = "Payload is NULL in event '{1}' from handler '{0}', span will not be recorded.", Level = EventLevel.Warning)] - public void NullPayload(string handlerName, string eventName) - { - this.WriteEvent(1, handlerName, eventName); - } - - [Event(2, Message = "Request is filtered out and will not be collected. Operation='{0}'", Level = EventLevel.Verbose)] + [Event(1, Message = "Request is filtered out and will not be collected. Operation='{0}'", Level = EventLevel.Verbose)] public void RequestIsFilteredOut(string operationName) { - this.WriteEvent(2, operationName); + this.WriteEvent(1, operationName); } - [Event(3, Message = "InstrumentationFilter threw an exception. Request will not be collected. Operation='{0}': {1}", Level = EventLevel.Error)] + [Event(2, Message = "Filter callback threw an exception. Request will not be collected. Operation='{0}': {1}", Level = EventLevel.Error)] public void RequestFilterException(string operationName, string exception) { - this.WriteEvent(3, operationName, exception); + this.WriteEvent(2, operationName, exception); } - [Event(4, Message = "Enrichment threw an exception. Event='{0}': {1}", Level = EventLevel.Error)] + [Event(3, Message = "Enrich callback threw an exception. Event='{0}': {1}", Level = EventLevel.Error)] public void EnrichmentException(string eventName, string exception) { - this.WriteEvent(4, eventName, exception); + this.WriteEvent(3, eventName, exception); } } }