Merge branch 'main' into yunl/gRPC

src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md

@@ -2,6 +2,10 @@
 
 ## Unreleased
 
+* Fixed an issue where activity started within middleware was modified by
+  instrumentation library.
+  ([#3498](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3498))
+
 * Updated to use Activity native support from `System.Diagnostics.DiagnosticSource`
   to set activity status.
   ([#3118](https://github.com/open-telemetry/opentelemetry-dotnet/issues/3118))

src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs

@@ -258,6 +258,32 @@ public override void OnCustom(string name, Activity activity, object payload)
         {
             if (name == "Microsoft.AspNetCore.Mvc.BeforeAction")
             {
+                // We cannot rely on Activity.Current here
+                // There could be activities started by middleware
+                // after activity started by framework resulting in different Activity.Current.
+                // so, we need to first find the activity started by Asp.Net Core.
+                // For .net6.0 onwards we could use IHttpActivityFeature to get the activity created by framework
+                // var httpActivityFeature = context.Features.Get<IHttpActivityFeature>();
+                // activity = httpActivityFeature.Activity;
+                // However, this will not work as in case of custom propagator
+                // we start a new activity during onStart event which is a sibling to the activity created by framework
+                // So, in that case we need to get the activity created by us here.
+                // we can do so only by looping through activity.Parent chain.
+                while (activity != null)
+                {
+                    if (string.Equals(activity.OperationName, ActivityOperationName, StringComparison.Ordinal))
+                    {
+                        break;
+                    }
+
+                    activity = activity.Parent;
+                }
+
+                if (activity == null)
+                {
+                    return;
+                }
+
                 if (activity.IsAllDataRequested)
                 {
                     // See https://github.com/aspnet/Mvc/blob/2414db256f32a047770326d14d8b0e2afd49ba49/src/Microsoft.AspNetCore.Mvc.Core/MvcCoreDiagnosticSourceExtensions.cs#L36-L44

test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs

@@ -541,8 +541,54 @@ void ConfigureTestServices(IServiceCollection services)
             Assert.Equal(shouldEnrichBeCalled, enrichCalled);
         }
 
-        [Fact(Skip = "Changes pending on instrumentation")]
-        public async Task ActivitiesStartedInMiddlewareShouldNotBeUpdatedByInstrumentation()
+        [Fact]
+        public async Task ActivitiesStartedInMiddlewareShouldNotBeUpdated()
+        {
+            var exportedItems = new List<Activity>();
+
+            var activitySourceName = "TestMiddlewareActivitySource";
+            var activityName = "TestMiddlewareActivity";
+
+            void ConfigureTestServices(IServiceCollection services)
+            {
+                services.AddSingleton<ActivityMiddleware.ActivityMiddlewareImpl>(new TestActivityMiddlewareImpl(activitySourceName, activityName));
+                this.tracerProvider = Sdk.CreateTracerProviderBuilder()
+                    .AddAspNetCoreInstrumentation()
+                    .AddSource(activitySourceName)
+                    .AddInMemoryExporter(exportedItems)
+                    .Build();
+            }
+
+            // Arrange
+            using (var client = this.factory
+                .WithWebHostBuilder(builder =>
+                    builder.ConfigureTestServices(ConfigureTestServices))
+                .CreateClient())
+            {
+                var response = await client.GetAsync("/api/values/2");
+                response.EnsureSuccessStatusCode();
+                WaitForActivityExport(exportedItems, 2);
+            }
+
+            Assert.Equal(2, exportedItems.Count);
+
+            var middlewareActivity = exportedItems[0];
+
+            var aspnetcoreframeworkactivity = exportedItems[1];
+
+            // Middleware activity name should not be changed
+            Assert.Equal(ActivityKind.Internal, middlewareActivity.Kind);
+            Assert.Equal(activityName, middlewareActivity.OperationName);
+            Assert.Equal(activityName, middlewareActivity.DisplayName);
+
+            // tag http.route should be added on activity started by asp.net core
+            Assert.Equal("api/Values/{id}", aspnetcoreframeworkactivity.GetTagValue(SemanticConventions.AttributeHttpRoute) as string);
+            Assert.Equal("Microsoft.AspNetCore.Hosting.HttpRequestIn", aspnetcoreframeworkactivity.OperationName);
+            Assert.Equal("api/Values/{id}", aspnetcoreframeworkactivity.DisplayName);
+        }
+
+        [Fact]
+        public async Task ActivitiesStartedInMiddlewareBySettingHostActivityToNullShouldNotBeUpdated()
         {
             var exportedItems = new List<Activity>();
 
@@ -554,7 +600,7 @@ public async Task ActivitiesStartedInMiddlewareShouldNotBeUpdatedByInstrumentati
                 .WithWebHostBuilder(builder =>
                     builder.ConfigureTestServices((IServiceCollection services) =>
                     {
-                        services.AddSingleton<ActivityMiddleware.ActivityMiddlewareImpl>(new TestActivityMiddlewareImpl(activitySourceName, activityName));
+                        services.AddSingleton<ActivityMiddleware.ActivityMiddlewareImpl>(new TestNullHostActivityMiddlewareImpl(activitySourceName, activityName));
                         services.AddOpenTelemetryTracing((builder) => builder.AddAspNetCoreInstrumentation()
                         .AddSource(activitySourceName)
                         .AddInMemoryExporter(exportedItems));
@@ -570,8 +616,17 @@ public async Task ActivitiesStartedInMiddlewareShouldNotBeUpdatedByInstrumentati
 
             var middlewareActivity = exportedItems[0];
 
+            var aspnetcoreframeworkactivity = exportedItems[1];
+
             // Middleware activity name should not be changed
+            Assert.Equal(ActivityKind.Internal, middlewareActivity.Kind);
+            Assert.Equal(activityName, middlewareActivity.OperationName);
             Assert.Equal(activityName, middlewareActivity.DisplayName);
+
+            // tag http.route should not be added on activity started by asp.net core as it will not be found during oncustom event
+            Assert.DoesNotContain(aspnetcoreframeworkactivity.TagObjects, t => t.Key == SemanticConventions.AttributeHttpRoute);
+            Assert.Equal("Microsoft.AspNetCore.Hosting.HttpRequestIn", aspnetcoreframeworkactivity.OperationName);
+            Assert.Equal("/api/values/2", aspnetcoreframeworkactivity.DisplayName);
         }
 
 #if NET7_0_OR_GREATER
@@ -732,6 +787,34 @@ public override void OnStopActivity(Activity activity, object payload)
             }
         }
 
+        private class TestNullHostActivityMiddlewareImpl : ActivityMiddleware.ActivityMiddlewareImpl
+        {
+            private ActivitySource activitySource;
+            private Activity activity;
+            private string activityName;
+
+            public TestNullHostActivityMiddlewareImpl(string activitySourceName, string activityName)
+            {
+                this.activitySource = new ActivitySource(activitySourceName);
+                this.activityName = activityName;
+            }
+
+            public override void PreProcess(HttpContext context)
+            {
+                // Setting the host activity i.e. activity started by asp.net core
+                // to null here will have no impact on middleware activity.
+                // This also means that asp.net core activity will not be found
+                // during OnCustom event.
+                Activity.Current = null;
+                this.activity = this.activitySource.StartActivity(this.activityName);
+            }
+
+            public override void PostProcess(HttpContext context)
+            {
+                this.activity?.Stop();
+            }
+        }
+
         private class TestActivityMiddlewareImpl : ActivityMiddleware.ActivityMiddlewareImpl
         {
             private ActivitySource activitySource;