Skip to content

Commit cfc30b1

Browse files
JamesNKJamesNK
committed
Propagate trace parent to SignalR hub invocations
1 parent 98ee502 commit cfc30b1

File tree

13 files changed

+653
-113
lines changed

13 files changed

+653
-113
lines changed

‎src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs

+10-66
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
using Microsoft.AspNetCore.Http;
1010
using Microsoft.AspNetCore.Http.Features;
1111
using Microsoft.AspNetCore.Http.Metadata;
12+
using Microsoft.AspNetCore.Shared;
1213
using Microsoft.Extensions.Logging;
1314

1415
namespace Microsoft.AspNetCore.Hosting;
@@ -389,80 +390,23 @@ private void RecordRequestStartMetrics(HttpContext httpContext)
389390
hasDiagnosticListener = false;
390391

391392
var headers = httpContext.Request.Headers;
392-
_propagator.ExtractTraceIdAndState(headers,
393+
var activity = ActivityCreator.CreateFromRemote(
394+
_activitySource,
395+
_propagator,
396+
headers,
393397
static (object? carrier, string fieldName, out string? fieldValue, out IEnumerable<string>? fieldValues) =>
394398
{
395399
fieldValues = default;
396400
var headers = (IHeaderDictionary)carrier!;
397401
fieldValue = headers[fieldName];
398402
},
399-
out var requestId,
400-
out var traceState);
401-
402-
Activity? activity = null;
403-
if (_activitySource.HasListeners())
404-
{
405-
if (ActivityContext.TryParse(requestId, traceState, isRemote: true, out ActivityContext context))
406-
{
407-
// The requestId used the W3C ID format. Unfortunately, the ActivitySource.CreateActivity overload that
408-
// takes a string parentId never sets HasRemoteParent to true. We work around that by calling the
409-
// ActivityContext overload instead which sets HasRemoteParent to parentContext.IsRemote.
410-
// https://github.com/dotnet/aspnetcore/pull/41568#discussion_r868733305
411-
activity = _activitySource.CreateActivity(ActivityName, ActivityKind.Server, context);
412-
}
413-
else
414-
{
415-
// Pass in the ID we got from the headers if there was one.
416-
activity = _activitySource.CreateActivity(ActivityName, ActivityKind.Server, string.IsNullOrEmpty(requestId) ? null! : requestId);
417-
}
418-
}
419-
403+
ActivityName,
404+
tags: null,
405+
links: null,
406+
loggingEnabled || diagnosticListenerActivityCreationEnabled);
420407
if (activity is null)
421408
{
422-
// CreateActivity didn't create an Activity (this is an optimization for the
423-
// case when there are no listeners). Let's create it here if needed.
424-
if (loggingEnabled || diagnosticListenerActivityCreationEnabled)
425-
{
426-
activity = new Activity(ActivityName);
427-
if (!string.IsNullOrEmpty(requestId))
428-
{
429-
activity.SetParentId(requestId);
430-
}
431-
}
432-
else
433-
{
434-
return null;
435-
}
436-
}
437-
438-
// The trace id was successfully extracted, so we can set the trace state
439-
// https://www.w3.org/TR/trace-context/#tracestate-header
440-
if (!string.IsNullOrEmpty(requestId))
441-
{
442-
if (!string.IsNullOrEmpty(traceState))
443-
{
444-
activity.TraceStateString = traceState;
445-
}
446-
}
447-
448-
// Baggage can be used regardless of whether a distributed trace id was present on the inbound request.
449-
// https://www.w3.org/TR/baggage/#abstract
450-
var baggage = _propagator.ExtractBaggage(headers, static (object? carrier, string fieldName, out string? fieldValue, out IEnumerable<string>? fieldValues) =>
451-
{
452-
fieldValues = default;
453-
var headers = (IHeaderDictionary)carrier!;
454-
fieldValue = headers[fieldName];
455-
});
456-
457-
// AddBaggage adds items at the beginning of the list, so we need to add them in reverse to keep the same order as the client
458-
// By contract, the propagator has already reversed the order of items so we need not reverse it again
459-
// Order could be important if baggage has two items with the same key (that is allowed by the contract)
460-
if (baggage is not null)
461-
{
462-
foreach (var baggageItem in baggage)
463-
{
464-
activity.AddBaggage(baggageItem.Key, baggageItem.Value);
465-
}
409+
return null;
466410
}
467411

468412
_diagnosticListener.OnActivityImport(activity, httpContext);

‎src/Hosting/Hosting/src/Microsoft.AspNetCore.Hosting.csproj

+1
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
<Compile Include="$(SharedSourceRoot)StaticWebAssets\**\*.cs" LinkBase="StaticWebAssets" />
1919
<Compile Include="$(SharedSourceRoot)Metrics\MetricsExtensions.cs" />
2020
<Compile Include="$(SharedSourceRoot)Metrics\MetricsConstants.cs" />
21+
<Compile Include="$(SharedSourceRoot)Diagnostics\ActivityCreator.cs" />
2122
</ItemGroup>
2223

2324
<ItemGroup>

‎src/Shared/Diagnostics/ActivityCreator.cs

+106
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
4+
using System.Diagnostics;
5+
6+
namespace Microsoft.AspNetCore.Shared;
7+
8+
internal static class ActivityCreator
9+
{
10+
/// <summary>
11+
/// Create an activity with details received from a remote source.
12+
/// </summary>
13+
public static Activity? CreateFromRemote(
14+
ActivitySource activitySource,
15+
DistributedContextPropagator propagator,
16+
object distributedContextCarrier,
17+
DistributedContextPropagator.PropagatorGetterCallback propagatorGetter,
18+
string activityName,
19+
IEnumerable<KeyValuePair<string, object?>>? tags,
20+
IEnumerable<ActivityLink>? links,
21+
bool diagnosticsOrLoggingEnabled)
22+
{
23+
propagator.ExtractTraceIdAndState(
24+
distributedContextCarrier,
25+
propagatorGetter,
26+
out var requestId,
27+
out var traceState);
28+
29+
Activity? activity = null;
30+
if (activitySource.HasListeners())
31+
{
32+
if (ActivityContext.TryParse(requestId, traceState, isRemote: true, out ActivityContext context))
33+
{
34+
// The requestId used the W3C ID format. Unfortunately, the ActivitySource.CreateActivity overload that
35+
// takes a string parentId never sets HasRemoteParent to true. We work around that by calling the
36+
// ActivityContext overload instead which sets HasRemoteParent to parentContext.IsRemote.
37+
// https://github.com/dotnet/aspnetcore/pull/41568#discussion_r868733305
38+
activity = activitySource.CreateActivity(activityName, ActivityKind.Server, context, tags: tags, links: links);
39+
}
40+
else
41+
{
42+
// Pass in the ID we got from the headers if there was one.
43+
activity = activitySource.CreateActivity(activityName, ActivityKind.Server, string.IsNullOrEmpty(requestId) ? null! : requestId, tags: tags, links: links);
44+
}
45+
}
46+
47+
if (activity is null)
48+
{
49+
// CreateActivity didn't create an Activity (this is an optimization for the
50+
// case when there are no listeners). Let's create it here if needed.
51+
if (diagnosticsOrLoggingEnabled)
52+
{
53+
activity = new Activity(activityName);
54+
if (!string.IsNullOrEmpty(requestId))
55+
{
56+
activity.SetParentId(requestId);
57+
}
58+
if (tags != null)
59+
{
60+
foreach (var tag in tags)
61+
{
62+
activity.AddTag(tag.Key, tag.Value);
63+
}
64+
}
65+
if (links != null)
66+
{
67+
foreach (var link in links)
68+
{
69+
activity.AddLink(link);
70+
}
71+
}
72+
}
73+
else
74+
{
75+
return null;
76+
}
77+
}
78+
79+
// The trace id was successfully extracted, so we can set the trace state
80+
// https://www.w3.org/TR/trace-context/#tracestate-header
81+
if (!string.IsNullOrEmpty(requestId))
82+
{
83+
if (!string.IsNullOrEmpty(traceState))
84+
{
85+
activity.TraceStateString = traceState;
86+
}
87+
}
88+
89+
// Baggage can be used regardless of whether a distributed trace id was present on the inbound request.
90+
// https://www.w3.org/TR/baggage/#abstract
91+
var baggage = propagator.ExtractBaggage(distributedContextCarrier, propagatorGetter);
92+
93+
// AddBaggage adds items at the beginning of the list, so we need to add them in reverse to keep the same order as the client
94+
// By contract, the propagator has already reversed the order of items so we need not reverse it again
95+
// Order could be important if baggage has two items with the same key (that is allowed by the contract)
96+
if (baggage is not null)
97+
{
98+
foreach (var baggageItem in baggage)
99+
{
100+
activity.AddBaggage(baggageItem.Key, baggageItem.Value);
101+
}
102+
}
103+
104+
return activity;
105+
}
106+
}

‎src/Shared/SignalR/InProcessTestServer.cs

+4
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@ public abstract class InProcessTestServer : IAsyncDisposable
2727

2828
public abstract string Url { get; }
2929

30+
public abstract IServiceProvider Services { get; }
31+
3032
public abstract ValueTask DisposeAsync();
3133
}
3234

@@ -54,6 +56,8 @@ internal override event Action<LogRecord> ServerLogged
5456

5557
public override string Url => _url;
5658

59+
public override IServiceProvider Services => _host.Services;
60+
5761
public static async Task<InProcessTestServer<TStartup>> StartServer(ILoggerFactory loggerFactory, Action<KestrelServerOptions> configureKestrelServerOptions = null, IDisposable disposable = null)
5862
{
5963
var server = new InProcessTestServer<TStartup>(loggerFactory, configureKestrelServerOptions, disposable);

‎src/SignalR/clients/csharp/Client.Core/src/HubConnection.cs

+21
Original file line numberDiff line numberDiff line change
@@ -1037,6 +1037,7 @@ private async Task InvokeCore(ConnectionState connectionState, string methodName
10371037

10381038
// Client invocations are always blocking
10391039
var invocationMessage = new InvocationMessage(irq.InvocationId, methodName, args, streams);
1040+
InjectHeaders(invocationMessage);
10401041

10411042
Log.RegisteringInvocation(_logger, irq.InvocationId);
10421043
connectionState.AddInvocation(irq);
@@ -1063,6 +1064,7 @@ private async Task InvokeStreamCore(ConnectionState connectionState, string meth
10631064
Log.PreparingStreamingInvocation(_logger, irq.InvocationId, methodName, irq.ResultType.FullName!, args.Length);
10641065

10651066
var invocationMessage = new StreamInvocationMessage(irq.InvocationId, methodName, args, streams);
1067+
InjectHeaders(invocationMessage);
10661068

10671069
Log.RegisteringInvocation(_logger, irq.InvocationId);
10681070

@@ -1083,6 +1085,25 @@ private async Task InvokeStreamCore(ConnectionState connectionState, string meth
10831085
}
10841086
}
10851087

1088+
private static void InjectHeaders(HubInvocationMessage invocationMessage)
1089+
{
1090+
// TODO:
1091+
// This sends info about the current activity, regardless of the activity source, to the SignalR server.
1092+
// When SignalR client supports client activities this logic should be updated to only send headers
1093+
// if the SignalR client activity is created. The goal is to match the behavior of distributed tracing in HttpClient.
1094+
if (Activity.Current is { } currentActivity)
1095+
{
1096+
DistributedContextPropagator.Current.Inject(currentActivity, invocationMessage, static (carrier, key, value) =>
1097+
{
1098+
if (carrier is HubInvocationMessage invocationMessage)
1099+
{
1100+
invocationMessage.Headers ??= new Dictionary<string, string>();
1101+
invocationMessage.Headers[key] = value;
1102+
}
1103+
});
1104+
}
1105+
}
1106+
10861107
private async Task SendHubMessage(ConnectionState connectionState, HubMessage hubMessage, CancellationToken cancellationToken = default)
10871108
{
10881109
_state.AssertConnectionValid();

0 commit comments

Comments
 (0)