[Http] Fix propagation issues

src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md

@@ -2,7 +2,10 @@
 
 ## Unreleased
 
-* *Breaking change** The `Enrich` callback option has been removed. For better
+* Added back `netstandard2.0` target.
+  ([#3787](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3787))
+
+* **Breaking change**: The `Enrich` callback option has been removed. For better
   usability, it has been replaced by three separate options: In case of
   `HttpClient` the new options are `EnrichWithHttpRequestMessage`,
   `EnrichWithHttpResponseMessage` and `EnrichWithException` and in case of
@@ -15,11 +18,13 @@
   `HttpClient` and `HttpWebRequest`,`HttpWebResponse` and `Exception` in case of
   `HttpWebRequest`. The separate callbacks make it clear what event triggers
   them and there is no longer the need to cast the argument to the expected
-type.
-([#3792](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3792))
+  type.
+  ([#3792](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3792))
 
-* Added back `netstandard2.0` target.
-([#3787](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3787))
+* Fixed an issue which may prevent custom propagators from being called and may
+  cause broken traces when making outgoing calls using `HttpClient` on .NET &
+  .NET Core runtimes.
+  ([#3828](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3828))
 
 ## 1.0.0-rc9.8
 

src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs

@@ -100,11 +100,7 @@ public void OnStartActivity(Activity activity, object payload)
             // By this time, samplers have already run and
             // activity.IsAllDataRequested populated accordingly.
 
-            // For .NET7.0 or higher versions, activity is created using activity source
-            // However, the framework will fallback to creating activity if the sampler's decision is to drop and there is a active diagnostic listener.
-            // To prevent processing such activities we first check the source name to confirm if it was created using
-            // activity source or not.
-            if (Sdk.SuppressInstrumentation || (IsNet7OrGreater && string.IsNullOrEmpty(activity.Source.Name)))
+            if (Sdk.SuppressInstrumentation)
             {
                 return;
             }
@@ -115,16 +111,52 @@ public void OnStartActivity(Activity activity, object payload)
                 return;
             }
 
+            bool isNullActivitySource = string.IsNullOrEmpty(activity.Source.Name);
+
             // Propagate context irrespective of sampling decision
             var textMapPropagator = Propagators.DefaultTextMapPropagator;
-            if (textMapPropagator is not TraceContextPropagator)
+
+            var context = activity.Context;
+
+            // Note: isNullActivitySource is true for all .NET 6 and prior
+            // spans. isNullActivitySource is true for .NET 7+ only when
+            // sample decision was drop.
+            Debug.Assert(
+                isNullActivitySource || IsNet7OrGreater,
+                "activity source was invalid");
+
+            // Note: When sample decision is drop...
+            //  * On .NET 7 activity has empty source and is NOT updated by
+            //    the legacy logic. Recorded is just mirrored from the
+            //    parent.
+            //
+            //  * On .NET 6 activity is updated by the legacy logic and Recorded can be trusted.
+            if (isNullActivitySource
+                && (IsNet7OrGreater || !activity.Recorded))
             {
-                textMapPropagator.Inject(new PropagationContext(activity.Context, Baggage.Current), request, HttpRequestMessageContextPropagation.HeaderValueSetter);
+                var parent = activity.Parent;
+                if (parent != null)
+                {
+                    // If the runtime created a span regardless of sampling
+                    // decision send the parent (root) if we have one so
+                    // that we don't create broken traces.
+                    context = parent.Context;
+
+                    Debug.Assert(parent.Parent == null, "parent was not root");
+                }
             }
 
+            textMapPropagator.Inject(new PropagationContext(context, Baggage.Current), request, HttpRequestMessageContextPropagation.HeaderValueSetter);
+
+            // For .NET7.0 or higher versions, activity is created using activity source
+            // However, the framework will fallback to creating activity if the sampler's decision is to drop and there is a active diagnostic listener.
+            // To prevent processing such activities we first check the source name to confirm if it was created using
+            // activity source or not.
+            bool isNet7LegacyActivity = IsNet7OrGreater && isNullActivitySource;
+
             // enrich Activity from payload only if sampling decision
             // is favorable.
-            if (activity.IsAllDataRequested)
+            if (activity.IsAllDataRequested && !isNet7LegacyActivity)
             {
                 try
                 {

test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs

@@ -41,7 +41,13 @@ public HttpClientTests()
             this.serverLifeTime = TestHttpServer.RunServer(
                 (ctx) =>
                 {
-                    if (ctx.Request.Url.PathAndQuery.Contains("500"))
+                    string expectedTraceparent = ctx.Request.QueryString["expectedTraceparent"];
+                    if (!string.IsNullOrEmpty(expectedTraceparent) && ctx.Request.Headers["traceparent"] != expectedTraceparent)
+                    {
+                        ctx.Response.StatusCode = 500;
+                        ctx.Response.StatusDescription = "Traceparent mismatch";
+                    }
+                    else if (ctx.Request.Url.PathAndQuery.Contains("500"))
                     {
                         ctx.Response.StatusCode = 500;
                     }
@@ -584,6 +590,105 @@ public async Task HttpClientInstrumentationDoesNotReportExceptionEventOnErrorRes
             Assert.Empty(exportedItems[0].Events);
         }
 
+        [Theory]
+        [InlineData(true, true)]
+        [InlineData(true, false)]
+        [InlineData(false, true)]
+        [InlineData(false, false)]
+        public async Task CustomPropagatorCalled(bool sample, bool createParentActivity)
+        {
+            ActivityContext parentContext = default;
+            ActivityContext contextFromPropagator = default;
+
+            var propagator = new Mock<TextMapPropagator>();
+            propagator.Setup(m => m.Inject(It.IsAny<PropagationContext>(), It.IsAny<HttpRequestMessage>(), It.IsAny<Action<HttpRequestMessage, string, string>>()))
+                .Callback<PropagationContext, HttpRequestMessage, Action<HttpRequestMessage, string, string>>((context, message, action) =>
+                {
+                    contextFromPropagator = context.ActivityContext;
+
+                    string traceparent = $"00/{context.ActivityContext.TraceId}/{context.ActivityContext.SpanId}/01";
+
+                    message.RequestUri = new Uri(message.RequestUri.OriginalString + $"?expectedTraceparent={traceparent}");
+
+                    action(message, "traceparent", traceparent);
+                    action(message, "tracestate", Activity.Current.TraceStateString);
+                });
+
+            Sdk.SetDefaultTextMapPropagator(propagator.Object);
+
+            var exportedItems = new List<Activity>();
+
+            using (var traceprovider = Sdk.CreateTracerProviderBuilder()
+               .AddHttpClientInstrumentation()
+               .AddInMemoryExporter(exportedItems)
+               .SetSampler(sample ? new ParentBasedSampler(new AlwaysOnSampler()) : new AlwaysOffSampler())
+               .Build())
+            {
+                Activity parent = null;
+                if (createParentActivity)
+                {
+                    parent = new Activity("parent")
+                        .SetIdFormat(ActivityIdFormat.W3C)
+                        .Start();
+
+                    parent.TraceStateString = "k1=v1,k2=v2";
+                    parent.ActivityTraceFlags = ActivityTraceFlags.Recorded;
+
+                    parentContext = parent.Context;
+                }
+
+                var request = new HttpRequestMessage
+                {
+                    RequestUri = new Uri(this.url),
+                    Method = new HttpMethod("GET"),
+                };
+
+                using var c = new HttpClient();
+                await c.SendAsync(request);
+
+                parent?.Stop();
+            }
+
+            if (!sample)
+            {
+                Assert.Empty(exportedItems);
+            }
+            else
+            {
+                Assert.Single(exportedItems);
+            }
+
+            if (createParentActivity)
+            {
+                if (!sample)
+                {
+                    // If we created a parent, make sure that is what was injected.
+                    // Not the .NET 7 legacy activity.
+                    Assert.True(parentContext == contextFromPropagator);
+                }
+                else
+                {
+                    Assert.True(contextFromPropagator != parentContext);
+                    Assert.Equal(contextFromPropagator, exportedItems[0].Context);
+                }
+            }
+            else
+            {
+                // Make sure custom propagator was called.
+                Assert.True(contextFromPropagator != default);
+                if (sample)
+                {
+                    Assert.Equal(contextFromPropagator, exportedItems[0].Context);
+                }
+            }
+
+            Sdk.SetDefaultTextMapPropagator(new CompositeTextMapPropagator(new TextMapPropagator[]
+            {
+                new TraceContextPropagator(),
+                new BaggagePropagator(),
+            }));
+        }
+
         public void Dispose()
         {
             this.serverLifeTime?.Dispose();