Only create a sibling activity when trace id, span id or trace state …

…differ (#5136) Co-authored-by: Cijo Thomas <[email protected]> Co-authored-by: Mikel Blanchard <[email protected]> Co-authored-by: Alan West <[email protected]>
open-telemetry · Jan 16, 2024 · 18ccd87 · 18ccd87
1 parent 6d88762
commit 18ccd87
Show file tree

Hide file tree

Showing 6 changed files with 59 additions and 3 deletions.
diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs
@@ -105,7 +105,7 @@ public void OnStartActivity(Activity activity, object payload)
         // By this time, samplers have already run and
         // activity.IsAllDataRequested populated accordingly.
 
-        HttpContext context = payload as HttpContext;
+        var context = payload as HttpContext;
         if (context == null)
         {
             AspNetCoreInstrumentationEventSource.Log.NullPayload(nameof(HttpInListener), nameof(this.OnStartActivity), activity.OperationName);
@@ -118,9 +118,10 @@ public void OnStartActivity(Activity activity, object payload)
         if (textMapPropagator is not TraceContextPropagator)
         {
             var ctx = textMapPropagator.Extract(default, request, HttpRequestHeaderValuesGetter);
-
             if (ctx.ActivityContext.IsValid()
-                && ctx.ActivityContext != new ActivityContext(activity.TraceId, activity.ParentSpanId, activity.ActivityTraceFlags, activity.TraceStateString, true))
+                && !((ctx.ActivityContext.TraceId == activity.TraceId)
+                    && (ctx.ActivityContext.SpanId == activity.ParentSpanId)
+                    && (ctx.ActivityContext.TraceState == activity.TraceStateString)))
             {
                 // Create a new activity with its parent set from the extracted context.
                 // This makes the new activity as a "sibling" of the activity created by

diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md
@@ -9,6 +9,12 @@
   upgrading. For details see:
   [#5169](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5169)
 
+* Fixed an issue where the created activity from ASP.NET Core was replaced
+  with a new one. This replacement should only happen when the activity context
+  from the used propagator has a different trace id, parent span id or trace
+  state compared to the current activity. For details see:
+  [#5136](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5136)
+
 ## 1.7.0
 
 Released 2023-Dec-08

diff --git a/test/Directory.Packages.props b/test/Directory.Packages.props
@@ -4,5 +4,6 @@
     <PackageVersion Update="System.Text.Json" Version="7.0.1" />
     <PackageVersion Include="System.Runtime.InteropServices.RuntimeInformation" Version="4.3.0" />
     <PackageVersion Include="Microsoft.Coyote" Version="1.7.10" />
+    <PackageVersion Include="Microsoft.Extensions.Features" Version="8.0.0" />
   </ItemGroup>
 </Project>
diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs
@@ -1024,6 +1024,43 @@ public async Task DiagnosticSourceExceptionCallBackIsNotReceivedForExceptionsHan
         Assert.True(exceptionHandled);
     }
 
+#if NET6_0_OR_GREATER
+    [Fact]
+    public async Task NoSiblingActivityCreatedWhenTraceFlagsNone()
+    {
+        this.tracerProvider = Sdk.CreateTracerProviderBuilder()
+            .SetSampler(new AlwaysOnSampler())
+            .AddAspNetCoreInstrumentation()
+            .Build();
+
+        using var testFactory = this.factory
+            .WithWebHostBuilder(builder =>
+            {
+                builder.ConfigureTestServices(services =>
+                {
+                    this.tracerProvider = Sdk.CreateTracerProviderBuilder()
+                    .AddAspNetCoreInstrumentation()
+                    .Build();
+                });
+
+                builder.ConfigureLogging(loggingBuilder => loggingBuilder.ClearProviders());
+            });
+        using var client = testFactory.CreateClient();
+        var request = new HttpRequestMessage(HttpMethod.Get, "/api/GetActivityEquality");
+        var traceId = ActivityTraceId.CreateRandom();
+        var spanId = ActivitySpanId.CreateRandom();
+        request.Headers.Add("traceparent", $"00-{traceId}-{spanId}-00");
+
+        var response = await client.SendAsync(request);
+        var result = bool.Parse(await response.Content.ReadAsStringAsync());
+
+        Assert.True(response.IsSuccessStatusCode);
+
+        // Confirm that Activity.Current and IHttpActivityFeature activity are same
+        Assert.True(result);
+    }
+#endif
+
     public void Dispose()
     {
         this.tracerProvider?.Dispose();

diff --git a/test/TestApp.AspNetCore/Controllers/ChildActivityController.cs b/test/TestApp.AspNetCore/Controllers/ChildActivityController.cs
@@ -2,6 +2,7 @@
 // SPDX-License-Identifier: Apache-2.0
 
 using System.Diagnostics;
+using Microsoft.AspNetCore.Http.Features;
 using Microsoft.AspNetCore.Mvc;
 using OpenTelemetry;
 
@@ -34,4 +35,13 @@ public IReadOnlyDictionary<string, string> GetChildActivityBaggageContext()
         var result = Baggage.Current.GetBaggage();
         return result;
     }
+
+    [HttpGet]
+    [Route("api/GetActivityEquality")]
+    public bool GetActivityEquality()
+    {
+        var activity = this.HttpContext.Features.GetRequiredFeature<IHttpActivityFeature>().Activity;
+        var equal = Activity.Current == activity;
+        return equal;
+    }
 }
diff --git a/test/TestApp.AspNetCore/TestApp.AspNetCore.csproj b/test/TestApp.AspNetCore/TestApp.AspNetCore.csproj
@@ -5,6 +5,7 @@
   </PropertyGroup>
 
   <ItemGroup>
+    <PackageReference Include="Microsoft.Extensions.Features" />
     <PackageReference Include="Swashbuckle.AspNetCore" />
   </ItemGroup>