From 45a02a8f0827f98c0b5dc7c499c224fb481bd918 Mon Sep 17 00:00:00 2001 From: Cijo Thomas Date: Tue, 9 Mar 2021 20:31:33 -0800 Subject: [PATCH] Modify httpclient instrumentation to take advantage of sampling decision (#1894) * Modify httpclient instrumentation to take advantage of sampling decision * changelog --- .../CHANGELOG.md | 4 ++ .../HttpHandlerDiagnosticListener.cs | 50 +++++++++++++------ .../HttpClientTests.Basic.netcore31.cs | 1 + 3 files changed, 39 insertions(+), 16 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md index 8e4fc3345d9..a7e45ed525a 100644 --- 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 index 4de16246766..5e258afd413 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs @@ -67,12 +67,28 @@ 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 @@ -80,36 +96,38 @@ public override void OnStartActivity(Activity activity, object payload) 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 index 4cc67baecea..b027bd20e1d 100644 --- 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>(); var request = new HttpRequestMessage