PR feedback

Author: JamesNK

src/Shared/Diagnostics/ActivityCreator.cs

@@ -20,15 +20,18 @@ internal static class ActivityCreator
         IEnumerable<ActivityLink>? links,
         bool diagnosticsOrLoggingEnabled)
     {
-        propagator.ExtractTraceIdAndState(
-            distributedContextCarrier,
-            propagatorGetter,
-            out var requestId,
-            out var traceState);
-
         Activity? activity = null;
+        string? requestId = null;
+        string? traceState = null;
+
         if (activitySource.HasListeners())
         {
+            propagator.ExtractTraceIdAndState(
+                distributedContextCarrier,
+                propagatorGetter,
+                out requestId,
+                out traceState);
+
             if (ActivityContext.TryParse(requestId, traceState, isRemote: true, out ActivityContext context))
             {
                 // The requestId used the W3C ID format. Unfortunately, the ActivitySource.CreateActivity overload that
@@ -40,7 +43,7 @@ internal static class ActivityCreator
             else
             {
                 // Pass in the ID we got from the headers if there was one.
-                activity = activitySource.CreateActivity(activityName, ActivityKind.Server, string.IsNullOrEmpty(requestId) ? null! : requestId, tags: tags, links: links);
+                activity = activitySource.CreateActivity(activityName, ActivityKind.Server, string.IsNullOrEmpty(requestId) ? null : requestId, tags: tags, links: links);
             }
         }
 
@@ -50,6 +53,19 @@ internal static class ActivityCreator
             // case when there are no listeners). Let's create it here if needed.
             if (diagnosticsOrLoggingEnabled)
             {
+                // Note that there is a very small chance that propagator has already been called.
+                // Requires that the activity source had listened, but it didn't create an activity.
+                // Can only happen if there is a race between HasListeners and CreateActivity calls,
+                // and someone removing the listener.
+                //
+                // The only negative of calling the propagator twice is a small performance hit, but
+                // it's small and unlikely so it's not worth trying to optimize.
+                propagator.ExtractTraceIdAndState(
+                    distributedContextCarrier,
+                    propagatorGetter,
+                    out requestId,
+                    out traceState);
+
                 activity = new Activity(activityName);
                 if (!string.IsNullOrEmpty(requestId))
                 {

src/SignalR/clients/csharp/Client/test/FunctionalTests/HubConnectionTests.cs

@@ -140,16 +140,43 @@ public async Task InvokeAsync_SendTraceHeader(string protocolName, HttpTransport
 
             var connection = connectionBuilder.Build();
 
-            using var clientActivity = new Activity("ClientActivity");
-            clientActivity.Start();
-
+            Activity clientActivity1 = null;
+            Activity clientActivity2 = null;
             try
             {
                 await connection.StartAsync().DefaultTimeout();
 
-                var result = await connection.InvokeAsync<string>(nameof(TestHub.HelloWorld)).DefaultTimeout();
+                // Invocation 1
+                try
+                {
+                    clientActivity1 = new Activity("ClientActivity1");
+                    clientActivity1.AddBaggage("baggage-1", "value-1");
+                    clientActivity1.Start();
 
-                Assert.Equal("Hello World!", result);
+                    var result = await connection.InvokeAsync<string>(nameof(TestHub.HelloWorld)).DefaultTimeout();
+
+                    Assert.Equal("Hello World!", result);
+                }
+                finally
+                {
+                    clientActivity1?.Stop();
+                }
+
+                // Invocation 2
+                try
+                {
+                    clientActivity2 = new Activity("ClientActivity2");
+                    clientActivity2.AddBaggage("baggage-2", "value-2");
+                    clientActivity2.Start();
+
+                    var result = await connection.InvokeAsync<string>(nameof(TestHub.HelloWorld)).DefaultTimeout();
+
+                    Assert.Equal("Hello World!", result);
+                }
+                finally
+                {
+                    clientActivity2?.Stop();
+                }
             }
             catch (Exception ex)
             {
@@ -158,11 +185,10 @@ public async Task InvokeAsync_SendTraceHeader(string protocolName, HttpTransport
             }
             finally
             {
-                clientActivity.Stop();
                 await connection.DisposeAsync().DefaultTimeout();
             }
 
-            var activities = await channel.Reader.ReadAtLeastAsync(minimumCount: 3).DefaultTimeout();
+            var activities = await channel.Reader.ReadAtLeastAsync(minimumCount: 4).DefaultTimeout();
 
             var hubName = path switch
             {
@@ -178,18 +204,38 @@ public async Task InvokeAsync_SendTraceHeader(string protocolName, HttpTransport
                     Assert.Equal($"{hubName}/OnConnectedAsync", a.OperationName);
                     Assert.Equal("Microsoft.AspNetCore.Hosting.HttpRequestIn", a.Parent.OperationName);
                     Assert.False(a.HasRemoteParent);
+                    Assert.Empty(a.Baggage);
                 },
                 a =>
                 {
                     Assert.Equal($"{hubName}/HelloWorld", a.OperationName);
+                    Assert.Equal(clientActivity1.Id, a.ParentId);
                     Assert.True(a.HasRemoteParent);
-                    Assert.Equal(clientActivity.Id, a.ParentId);
+                    Assert.Collection(a.Baggage,
+                        b =>
+                        {
+                            Assert.Equal("baggage-1", b.Key);
+                            Assert.Equal("value-1", b.Value);
+                        });
+                },
+                a =>
+                {
+                    Assert.Equal($"{hubName}/HelloWorld", a.OperationName);
+                    Assert.Equal(clientActivity2.Id, a.ParentId);
+                    Assert.True(a.HasRemoteParent);
+                    Assert.Collection(a.Baggage,
+                        b =>
+                        {
+                            Assert.Equal("baggage-2", b.Key);
+                            Assert.Equal("value-2", b.Value);
+                        });
                 },
                 a =>
                 {
                     Assert.Equal($"{hubName}/OnDisconnectedAsync", a.OperationName);
                     Assert.Equal("Microsoft.AspNetCore.Hosting.HttpRequestIn", a.Parent.OperationName);
                     Assert.False(a.HasRemoteParent);
+                    Assert.Empty(a.Baggage);
                 });
         }
     }
@@ -570,12 +616,15 @@ public async Task StreamAsyncCore_SendTraceHeader(string protocolName, HttpTrans
 
             var connection = CreateHubConnection(server.Url, path, transportType, protocol, LoggerFactory);
 
-            using var clientActivity = new Activity("ClientActivity");
-            clientActivity.Start();
-
+            Activity clientActivity = null;
             try
             {
                 await connection.StartAsync().DefaultTimeout();
+
+                clientActivity = new Activity("ClientActivity");
+                clientActivity.AddBaggage("baggage-1", "value-1");
+                clientActivity.Start();
+
                 var expectedValue = 0;
                 var streamTo = 5;
                 var asyncEnumerable = connection.StreamAsyncCore<int>("Stream", new object[] { streamTo });
@@ -594,7 +643,7 @@ public async Task StreamAsyncCore_SendTraceHeader(string protocolName, HttpTrans
             }
             finally
             {
-                clientActivity.Stop();
+                clientActivity?.Stop();
                 await connection.DisposeAsync().DefaultTimeout();
             }
 
@@ -614,18 +663,26 @@ public async Task StreamAsyncCore_SendTraceHeader(string protocolName, HttpTrans
                     Assert.Equal($"{hubName}/OnConnectedAsync", a.OperationName);
                     Assert.Equal("Microsoft.AspNetCore.Hosting.HttpRequestIn", a.Parent.OperationName);
                     Assert.False(a.HasRemoteParent);
+                    Assert.Empty(a.Baggage);
                 },
                 a =>
                 {
                     Assert.Equal($"{hubName}/Stream", a.OperationName);
-                    Assert.True(a.HasRemoteParent);
                     Assert.Equal(clientActivity.Id, a.ParentId);
+                    Assert.True(a.HasRemoteParent);
+                    Assert.Collection(a.Baggage,
+                        b =>
+                        {
+                            Assert.Equal("baggage-1", b.Key);
+                            Assert.Equal("value-1", b.Value);
+                        });
                 },
                 a =>
                 {
                     Assert.Equal($"{hubName}/OnDisconnectedAsync", a.OperationName);
                     Assert.Equal("Microsoft.AspNetCore.Hosting.HttpRequestIn", a.Parent.OperationName);
                     Assert.False(a.HasRemoteParent);
+                    Assert.Empty(a.Baggage);
                 });
         }
     }