Modify ASP.NET Core instrumentation to take advantage of sampling dec…

…ision (#1899)
open-telemetry · Mar 18, 2021 · 0c8b5e1 · 0c8b5e1
1 parent e1ee334
commit 0c8b5e1
Show file tree

Hide file tree

Showing 6 changed files with 402 additions and 92 deletions.
diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md
@@ -6,6 +6,10 @@
   and ActivityProcessors. Samplers, ActivityProcessor.OnStart will now get the
   Activity before any enrichment done by the instrumentation.
   ([#1836](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1836))
+* Performance optimization by leveraging sampling decision and short circuiting
+  activity enrichment. `Filter` and `Enrich` are now only called if
+  `activity.IsAllDataRequested` is `true`
+  ([#1899](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1899))
 
 ## 1.0.0-rc2
 

diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs
@@ -57,29 +57,29 @@ public HttpInListener(string name, AspNetCoreInstrumentationOptions options)
         [System.Diagnostics.CodeAnalysis.SuppressMessage("Reliability", "CA2000:Dispose objects before losing scope", Justification = "The objects should not be disposed.")]
         public override void OnStartActivity(Activity activity, object payload)
         {
-            _ = this.startContextFetcher.TryFetch(payload, out HttpContext context);
-            if (context == null)
+            // The overall flow of what AspNetCore library does is as below:
+            // Activity.Start()
+            // DiagnosticSource.WriteEvent("Start", payload)
+            // DiagnosticSource.WriteEvent("Stop", payload)
+            // Activity.Stop()
+
+            // This method is in the WriteEvent("Start", payload) path.
+            // By this time, samplers have already run and
+            // activity.IsAllDataRequested populated accordingly.
+
+            if (Sdk.SuppressInstrumentation)
             {
-                AspNetCoreInstrumentationEventSource.Log.NullPayload(nameof(HttpInListener), nameof(this.OnStartActivity));
                 return;
             }
 
-            try
-            {
-                if (this.options.Filter?.Invoke(context) == false)
-                {
-                    AspNetCoreInstrumentationEventSource.Log.RequestIsFilteredOut(activity.OperationName);
-                    activity.IsAllDataRequested = false;
-                    return;
-                }
-            }
-            catch (Exception ex)
+            _ = this.startContextFetcher.TryFetch(payload, out HttpContext context);
+            if (context == null)
             {
-                AspNetCoreInstrumentationEventSource.Log.RequestFilterException(ex);
-                activity.IsAllDataRequested = false;
+                AspNetCoreInstrumentationEventSource.Log.NullPayload(nameof(HttpInListener), nameof(this.OnStartActivity));
                 return;
             }
 
+            // Ensure context extraction irrespective of sampling decision
             var request = context.Request;
             var textMapPropagator = Propagators.DefaultTextMapPropagator;
             if (!this.hostingSupportsW3C || !(textMapPropagator is TraceContextPropagator))
@@ -110,11 +110,29 @@ public override void OnStartActivity(Activity activity, object payload)
                 }
             }
 
-            ActivityInstrumentationHelper.SetActivitySourceProperty(activity, ActivitySource);
-            ActivityInstrumentationHelper.SetKindProperty(activity, ActivityKind.Server);
-
+            // enrich Activity from payload only if sampling decision
+            // is favorable.
             if (activity.IsAllDataRequested)
             {
+                try
+                {
+                    if (this.options.Filter?.Invoke(context) == false)
+                    {
+                        AspNetCoreInstrumentationEventSource.Log.RequestIsFilteredOut(activity.OperationName);
+                        activity.IsAllDataRequested = false;
+                        return;
+                    }
+                }
+                catch (Exception ex)
+                {
+                    AspNetCoreInstrumentationEventSource.Log.RequestFilterException(ex);
+                    activity.IsAllDataRequested = false;
+                    return;
+                }
+
+                ActivityInstrumentationHelper.SetActivitySourceProperty(activity, ActivitySource);
+                ActivityInstrumentationHelper.SetKindProperty(activity, ActivityKind.Server);
+
                 var path = (request.PathBase.HasValue || request.Path.HasValue) ? (request.PathBase + request.Path).ToString() : "/";
                 activity.DisplayName = path;