From 00b6a849d7b1de6f9e575da1ee2403ede0fc2499 Mon Sep 17 00:00:00 2001 From: Denis Ivanov Date: Mon, 21 Mar 2022 18:48:24 -0700 Subject: [PATCH] Excluding span links based on decisions made in https://github.com/open-telemetry/opentelemetry-specification/pull/2078 --- .../Internal/SpanAttributeConstants.cs | 2 - .../HttpHandlerDiagnosticListener.cs | 37 --------- .../HttpWebRequestActivitySource.netfx.cs | 45 +--------- .../HttpClientTests.Basic.netcore31.cs | 82 ------------------- .../HttpWebRequestTests.Basic.netfx.cs | 9 +- ...elemetry.Instrumentation.Http.Tests.csproj | 4 +- 6 files changed, 4 insertions(+), 175 deletions(-) diff --git a/src/OpenTelemetry.Api/Internal/SpanAttributeConstants.cs b/src/OpenTelemetry.Api/Internal/SpanAttributeConstants.cs index c5b6a49dbc7..5640fd3decb 100644 --- a/src/OpenTelemetry.Api/Internal/SpanAttributeConstants.cs +++ b/src/OpenTelemetry.Api/Internal/SpanAttributeConstants.cs @@ -23,8 +23,6 @@ internal static class SpanAttributeConstants { public const string StatusCodeKey = "otel.status_code"; public const string StatusDescriptionKey = "otel.status_description"; - public const string PreviousTryContextKey = "otel.previous_try_context"; - public const string RetryCountKey = "http.retry_count"; public const string DatabaseStatementTypeKey = "db.statement_type"; } } diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs index d8a01f76cd6..3989bf8c6af 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs @@ -35,8 +35,6 @@ internal sealed class HttpHandlerDiagnosticListener : ListenerHandler internal static readonly Version Version = AssemblyName.Version; internal static readonly ActivitySource ActivitySource = new(ActivitySourceName, Version.ToString()); - private const string RestartedActivityKey = "dotnet.restarted_activity"; - private static readonly Regex CoreAppMajorVersionCheckRegex = new("^\\.NETCoreApp,Version=v(\\d+)\\.", RegexOptions.Compiled | RegexOptions.IgnoreCase); private readonly PropertyFetcher startRequestFetcher = new("Request"); @@ -90,32 +88,6 @@ public override void OnStartActivity(Activity activity, object payload) return; } - var context = Propagators.DefaultTextMapPropagator.Extract(default, request, HttpRequestMessageContextPropagation.HeaderValuesGetter); - if (context != default && request.Properties.TryGetValue(SpanAttributeConstants.PreviousTryContextKey, out var previousContext)) - { - // Handling request retry or redirect. - var retryCount = 1; - if (request.Properties.TryGetValue(SpanAttributeConstants.RetryCountKey, out var previousRetryCount)) - { - retryCount = (int)previousRetryCount + 1; - } - - // Suppressing activity started by HttpClient DiagnosticsHandler. - activity.IsAllDataRequested = false; - activity.Stop(); - - activity = ActivitySource.StartActivity(activity.Kind, context.ActivityContext, links: new[] { new ActivityLink((ActivityContext)previousContext) }); - Activity.Current = activity; - - request.Properties[RestartedActivityKey] = activity; - - activity.SetTag(SpanAttributeConstants.RetryCountKey, retryCount); - request.Properties[SpanAttributeConstants.RetryCountKey] = retryCount; - } - - // Store activity context for the next possible try. - request.Properties[SpanAttributeConstants.PreviousTryContextKey] = activity.Context; - // Propagate context irrespective of sampling decision var textMapPropagator = Propagators.DefaultTextMapPropagator; if (!(this.httpClientSupportsW3C && textMapPropagator is TraceContextPropagator)) @@ -215,15 +187,6 @@ public override void OnStopActivity(Activity activity, object payload) HttpInstrumentationEventSource.Log.EnrichmentException(ex); } } - - if (this.startRequestFetcher.TryFetch(payload, out HttpRequestMessage request) && request != null) - { - if (request.Properties.TryGetValue(RestartedActivityKey, out object restartedActivityInstance)) - { - var restartedActivity = (Activity)restartedActivityInstance; - restartedActivity.Stop(); - } - } } } diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs index b70b449a5f8..5c7385d9300 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs @@ -213,40 +213,8 @@ private static void ProcessRequest(HttpWebRequest request) return; } - if (RequestProperties.Instance == null) - { - RequestProperties.Instance = new Dictionary(); - } - - Activity activity; - var context = Propagators.DefaultTextMapPropagator.Extract(default, request, HttpWebRequestHeaderValuesGetter); - if (context != default && RequestProperties.Instance.TryGetValue(SpanAttributeConstants.PreviousTryContextKey, out var previousContext)) - { - // This request was instrumented by previous - // ProcessRequest, such is the case with retries or redirect responses where the same request is sent again. - - var retryCount = 1; - if (RequestProperties.Instance.TryGetValue(SpanAttributeConstants.RetryCountKey, out var previousRetryCount)) - { - retryCount = (int)previousRetryCount + 1; - } - - activity = WebRequestActivitySource.StartActivity(ActivityName, ActivityKind.Client, context.ActivityContext, links: new[] { new ActivityLink((ActivityContext)previousContext) }); - - activity?.SetTag(SpanAttributeConstants.RetryCountKey, retryCount); - RequestProperties.Instance[SpanAttributeConstants.RetryCountKey] = retryCount; - } - else - { - activity = WebRequestActivitySource.StartActivity(ActivityName, ActivityKind.Client); - } - + var activity = WebRequestActivitySource.StartActivity(ActivityName, ActivityKind.Client); var activityContext = Activity.Current?.Context ?? default; - if (activityContext != default) - { - // Store activity context for the next possible try. - RequestProperties.Instance[SpanAttributeConstants.PreviousTryContextKey] = activityContext; - } // Propagation must still be done in all cases, to allow // downstream services to continue from parent context, if any. @@ -658,17 +626,6 @@ private static Func CreateTypeInstance(ConstructorInfo construct return (Func)setterMethod.CreateDelegate(typeof(Func)); } - private static class RequestProperties - { - private static readonly AsyncLocal> Properties = new AsyncLocal>(); - - public static Dictionary Instance - { - get => Properties.Value; - set => Properties.Value = value; - } - } - private class HashtableWrapper : Hashtable, IEnumerable { private readonly Hashtable table; diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.netcore31.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.netcore31.cs index 94ebb4e8396..e58cab62804 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.netcore31.cs +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.netcore31.cs @@ -303,55 +303,6 @@ public async Task HttpClientInstrumentationWorksIfAlreadyInstrumented() Assert.NotEqual(parentSpanId, activity.ParentSpanId.ToString()); } - /// - /// Test request connection retry: reflection hook does not throw. - /// - [Theory] - [InlineData("GET")] - [InlineData("POST")] - public async Task TestSecureTransportRetryFailureRequest(string method) - { - var processor = new Mock>(); - - var uriBuilder = new UriBuilder(this.url) { Scheme = "https" }; - - using (Sdk.CreateTracerProviderBuilder() - .AddHttpClientInstrumentation() - .AddProcessor(processor.Object) - .Build()) - { - using (var client = new HttpClient(new SimpleRetryHandler(new HttpClientHandler(), maxRetryCount: 1))) - { - var ex = await Assert.ThrowsAnyAsync(() => - { - return method == "GET" - ? client.GetAsync(uriBuilder.Uri) - : client.PostAsync(uriBuilder.Uri, new StringContent("hello world")); - }); - Assert.True(ex is InvalidOperationException); - } - } - - Assert.Equal(8, processor.Invocations.Count); // SetParentProvider/OnStart/OnEnd/OnStart/OnStart/OnEnd/OnShutdown/Dispose called. - var firstTry = (Activity)processor.Invocations[2].Arguments[0]; - var secondTry = (Activity)processor.Invocations[5].Arguments[0]; - - Assert.Equal(ActivityKind.Client, firstTry.Kind); - Assert.NotEqual(default, firstTry.Context.SpanId); - - Assert.Equal(ActivityKind.Client, secondTry.Kind); - Assert.NotEqual(default, secondTry.Context.SpanId); - - ActivityLink activityLink = secondTry.Links.FirstOrDefault(); - Assert.NotEqual(default, activityLink); - Assert.Equal(firstTry.Context.TraceId, activityLink.Context.TraceId); - Assert.Equal(firstTry.Context.SpanId, activityLink.Context.SpanId); - - var retryCount = secondTry.GetTagItem("http.retry_count"); - Assert.NotNull(retryCount); - Assert.Equal(1, (int)retryCount); - } - [Fact] public async void RequestNotCollectedWhenInstrumentationFilterApplied() { @@ -491,39 +442,6 @@ private static void ActivityEnrichment(Activity activity, string method, object break; } } - - private sealed class SimpleRetryHandler : DelegatingHandler - { - private readonly int maxRetryCount; - - public SimpleRetryHandler(HttpMessageHandler innerHandler, int maxRetryCount = 3) - : base(innerHandler) - { - this.maxRetryCount = maxRetryCount; - } - - protected override async Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) - { - HttpResponseMessage response = null; - for (int i = 0; i < this.maxRetryCount + 1; i++) - { - try - { - response = await base.SendAsync(request, cancellationToken); - if (response.IsSuccessStatusCode) - { - return response; - } - } - catch - { - // ignored - } - } - - return response; - } - } } } #endif diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestTests.Basic.netfx.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestTests.Basic.netfx.cs index 9f5bfb89ac8..30dbd89ef46 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestTests.Basic.netfx.cs +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestTests.Basic.netfx.cs @@ -218,14 +218,7 @@ public async Task HttpWebRequestInstrumentationWorksIfAlreadyInstrumented() using var c = new HttpClient(); await c.SendAsync(request); - Assert.Equal(3, activityProcessor.Invocations.Count); // SetParentProvider/Begin/End called - var activity = (Activity)activityProcessor.Invocations[2].Arguments[0]; - - Assert.Equal(ActivityKind.Client, activity.Kind); - Assert.NotEqual(default, activity.Context.SpanId); - Assert.NotEqual(traceId, activity.TraceId.ToString()); - Assert.NotEqual(parentSpanId, activity.SpanId.ToString()); - Assert.NotEqual(parentSpanId, activity.ParentSpanId.ToString()); + Assert.Equal(1, activityProcessor.Invocations.Count); } [Fact] diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/OpenTelemetry.Instrumentation.Http.Tests.csproj b/test/OpenTelemetry.Instrumentation.Http.Tests/OpenTelemetry.Instrumentation.Http.Tests.csproj index f6a8e15b318..3ca663d762a 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/OpenTelemetry.Instrumentation.Http.Tests.csproj +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/OpenTelemetry.Instrumentation.Http.Tests.csproj @@ -2,8 +2,8 @@ Unit test project for OpenTelemetry HTTP instrumentations - net6.0;net5.0;netcoreapp3.1 - $(TargetFrameworks);net461 + net6.0;net5.0;netcoreapp3.1;net461 +