Modify httpclient instrumentation to take advantage of sampling decis…

…ion (#1894) * Modify httpclient instrumentation to take advantage of sampling decision * changelog
open-telemetry · Mar 10, 2021 · 45a02a8 · 45a02a8
1 parent 822ce19
commit 45a02a8
Show file tree

Hide file tree

Showing 3 changed files with 39 additions and 16 deletions.
diff --git a/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md
@@ -2,6 +2,10 @@
 
 ## Unreleased
 
+* HttpClient (.NET Core) instrumentation performance optimization
+  by leveraging sampling decision and short circuiting path.
+  ([#1894](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1894))
+
 ## 1.0.0-rc2
 
 Released 2021-Jan-29

diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs
@@ -67,49 +67,67 @@ public HttpHandlerDiagnosticListener(HttpClientInstrumentationOptions options)
 
         public override void OnStartActivity(Activity activity, object payload)
         {
+            // The overall flow of what HttpClient 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)
+            {
+                return;
+            }
+
             if (!this.startRequestFetcher.TryFetch(payload, out HttpRequestMessage request) || request == null)
             {
                 HttpInstrumentationEventSource.Log.NullPayload(nameof(HttpHandlerDiagnosticListener), nameof(this.OnStartActivity));
                 return;
             }
 
+            // TODO: Investigate why this check is needed.
             if (Propagators.DefaultTextMapPropagator.Extract(default, request, HttpRequestMessageContextPropagation.HeaderValuesGetter) != default)
             {
                 // this request is already instrumented, we should back off
                 activity.IsAllDataRequested = false;
                 return;
             }
 
+            // Propagate context irrespective of sampling decision
             var textMapPropagator = Propagators.DefaultTextMapPropagator;
-
             if (!(this.httpClientSupportsW3C && textMapPropagator is TraceContextPropagator))
             {
                 textMapPropagator.Inject(new PropagationContext(activity.Context, Baggage.Current), request, HttpRequestMessageContextPropagation.HeaderValueSetter);
             }
 
-            try
+            // enrich Activity from payload only if sampling decision
+            // is favorable.
+            if (activity.IsAllDataRequested)
             {
-                if (this.options.EventFilter(activity.OperationName, request) == false)
+                try
                 {
-                    HttpInstrumentationEventSource.Log.RequestIsFilteredOut(activity.OperationName);
+                    if (this.options.EventFilter(activity.OperationName, request) == false)
+                    {
+                        HttpInstrumentationEventSource.Log.RequestIsFilteredOut(activity.OperationName);
+                        activity.IsAllDataRequested = false;
+                        return;
+                    }
+                }
+                catch (Exception ex)
+                {
+                    HttpInstrumentationEventSource.Log.RequestFilterException(ex);
                     activity.IsAllDataRequested = false;
                     return;
                 }
-            }
-            catch (Exception ex)
-            {
-                HttpInstrumentationEventSource.Log.RequestFilterException(ex);
-                activity.IsAllDataRequested = false;
-                return;
-            }
 
-            activity.DisplayName = HttpTagHelper.GetOperationNameForHttpMethod(request.Method);
+                activity.DisplayName = HttpTagHelper.GetOperationNameForHttpMethod(request.Method);
 
-            ActivityInstrumentationHelper.SetActivitySourceProperty(activity, ActivitySource);
-            ActivityInstrumentationHelper.SetKindProperty(activity, ActivityKind.Client);
+                ActivityInstrumentationHelper.SetActivitySourceProperty(activity, ActivitySource);
+                ActivityInstrumentationHelper.SetKindProperty(activity, ActivityKind.Client);
 
-            if (activity.IsAllDataRequested)
-            {
                 activity.SetTag(SemanticConventions.AttributeHttpMethod, HttpTagHelper.GetNameForHttpMethod(request.Method));
                 activity.SetTag(SemanticConventions.AttributeHttpHost, HttpTagHelper.GetHostTagValueFromRequestUri(request.RequestUri));
                 activity.SetTag(SemanticConventions.AttributeHttpUrl, request.RequestUri.OriginalString);

diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.netcore31.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.netcore31.cs
@@ -213,6 +213,7 @@ public async Task HttpClientInstrumentation_AddViaFactory_HttpInstrumentation_Co
         [Fact]
         public async Task HttpClientInstrumentationBacksOffIfAlreadyInstrumented()
         {
+            // TODO: Investigate why this feature is required.
             var processor = new Mock<BaseProcessor<Activity>>();
 
             var request = new HttpRequestMessage