Skip to content

Add System.Diagnostics.TextMapPropagator support #33777

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Jul 15, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ public GenericWebHostBuilder(IHostBuilder builder, WebHostBuilderOptions options
services.TryAddSingleton(sp => new DiagnosticListener("Microsoft.AspNetCore"));
services.TryAddSingleton<DiagnosticSource>(sp => sp.GetRequiredService<DiagnosticListener>());
services.TryAddSingleton(sp => new ActivitySource("Microsoft.AspNetCore"));
services.TryAddSingleton(DistributedContextPropagator.Current);

services.TryAddSingleton<IHttpContextFactory, DefaultHttpContextFactory>();
services.TryAddScoped<IMiddlewareFactory, MiddlewareFactory>();
Expand Down
26 changes: 14 additions & 12 deletions src/Hosting/Hosting/src/GenericHost/GenericWebHostedService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -21,24 +21,25 @@ namespace Microsoft.AspNetCore.Hosting
{
internal sealed partial class GenericWebHostService : IHostedService
{
public GenericWebHostService(
IOptions<GenericWebHostServiceOptions> options,
IServer server,
ILoggerFactory loggerFactory,
DiagnosticListener diagnosticListener,
ActivitySource activitySource,
IHttpContextFactory httpContextFactory,
IApplicationBuilderFactory applicationBuilderFactory,
IEnumerable<IStartupFilter> startupFilters,
IConfiguration configuration,
IWebHostEnvironment hostingEnvironment)
public GenericWebHostService(IOptions<GenericWebHostServiceOptions> options,
IServer server,
ILoggerFactory loggerFactory,
DiagnosticListener diagnosticListener,
ActivitySource activitySource,
DistributedContextPropagator propagator,
IHttpContextFactory httpContextFactory,
IApplicationBuilderFactory applicationBuilderFactory,
IEnumerable<IStartupFilter> startupFilters,
IConfiguration configuration,
IWebHostEnvironment hostingEnvironment)
{
Options = options.Value;
Server = server;
Logger = loggerFactory.CreateLogger("Microsoft.AspNetCore.Hosting.Diagnostics");
LifetimeLogger = loggerFactory.CreateLogger("Microsoft.Hosting.Lifetime");
DiagnosticListener = diagnosticListener;
ActivitySource = activitySource;
Propagator = propagator;
HttpContextFactory = httpContextFactory;
ApplicationBuilderFactory = applicationBuilderFactory;
StartupFilters = startupFilters;
Expand All @@ -53,6 +54,7 @@ public GenericWebHostService(
public ILogger LifetimeLogger { get; }
public DiagnosticListener DiagnosticListener { get; }
public ActivitySource ActivitySource { get; }
public DistributedContextPropagator Propagator { get; }
public IHttpContextFactory HttpContextFactory { get; }
public IApplicationBuilderFactory ApplicationBuilderFactory { get; }
public IEnumerable<IStartupFilter> StartupFilters { get; }
Expand Down Expand Up @@ -116,7 +118,7 @@ public async Task StartAsync(CancellationToken cancellationToken)
application = ErrorPageBuilder.BuildErrorPageApplication(HostingEnvironment.ContentRootFileProvider, Logger, showDetailedErrors, ex);
}

var httpApplication = new HostingApplication(application, Logger, DiagnosticListener, ActivitySource, HttpContextFactory);
var httpApplication = new HostingApplication(application, Logger, DiagnosticListener, ActivitySource, Propagator, HttpContextFactory);

await Server.StartAsync(httpApplication, cancellationToken);

Expand Down
3 changes: 2 additions & 1 deletion src/Hosting/Hosting/src/Internal/HostingApplication.cs
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,11 @@ public HostingApplication(
ILogger logger,
DiagnosticListener diagnosticSource,
ActivitySource activitySource,
DistributedContextPropagator propagator,
IHttpContextFactory httpContextFactory)
{
_application = application;
_diagnostics = new HostingApplicationDiagnostics(logger, diagnosticSource, activitySource);
_diagnostics = new HostingApplicationDiagnostics(logger, diagnosticSource, activitySource, propagator);
if (httpContextFactory is DefaultHttpContextFactory factory)
{
_defaultHttpContextFactory = factory;
Expand Down
54 changes: 28 additions & 26 deletions src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,9 @@
using System.Diagnostics;
using System.Globalization;
using System.Runtime.CompilerServices;
using System.Web;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Http.Features;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Primitives;
using Microsoft.Net.Http.Headers;

namespace Microsoft.AspNetCore.Hosting
{
Expand All @@ -31,13 +28,19 @@ internal class HostingApplicationDiagnostics

private readonly ActivitySource _activitySource;
private readonly DiagnosticListener _diagnosticListener;
private readonly DistributedContextPropagator _propagator;
private readonly ILogger _logger;

public HostingApplicationDiagnostics(ILogger logger, DiagnosticListener diagnosticListener, ActivitySource activitySource)
public HostingApplicationDiagnostics(
ILogger logger,
DiagnosticListener diagnosticListener,
ActivitySource activitySource,
DistributedContextPropagator propagator)
{
_logger = logger;
_diagnosticListener = diagnosticListener;
_activitySource = activitySource;
_propagator = propagator;
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
Expand Down Expand Up @@ -279,38 +282,37 @@ private static void RecordRequestStartEventLog(HttpContext httpContext)
{
return null;
}

var headers = httpContext.Request.Headers;
var requestId = headers.TraceParent;
if (requestId.Count == 0)
{
requestId = headers.RequestId;
}

if (!StringValues.IsNullOrEmpty(requestId))
_propagator.ExtractTraceIdAndState(headers,
static (object? carrier, string fieldName, out string? fieldValue, out IEnumerable<string>? fieldValues) =>
{
fieldValues = default;
var headers = (IHeaderDictionary)carrier!;
fieldValue = headers[fieldName];
},
out var requestId,
out var traceState);

if (!string.IsNullOrEmpty(requestId))
{
activity.SetParentId(requestId);
var traceState = headers.TraceState;
if (traceState.Count > 0)
if (!string.IsNullOrEmpty(traceState))
{
activity.TraceStateString = traceState;
}

// We expect baggage to be empty by default
// Only very advanced users will be using it in near future, we encourage them to keep baggage small (few items)
var baggage = headers.GetCommaSeparatedValues(HeaderNames.Baggage);
if (baggage.Length == 0)
var baggage = _propagator.ExtractBaggage(headers, static (object? carrier, string fieldName, out string? fieldValue, out IEnumerable<string>? fieldValues) =>
{
baggage = headers.GetCommaSeparatedValues(HeaderNames.CorrelationContext);
}
fieldValues = default;
var headers = (IHeaderDictionary)carrier!;
fieldValue = headers[fieldName];
});

// 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
// An order could be important if baggage has two items with the same key (that is allowed by the contract)
for (var i = baggage.Length - 1; i >= 0; i--)
// Order could be important if baggage has two items with the same key (that is allowed by the contract)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You used to add these in reverse, does ExtractBaggage handle the reversal? If so then include that in the comment. Otherwise this comment about order sensitivity doesn't make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ll send a follow-up PR to fix the comment. I want to merge this when the PR checks are green in case AzDo has a bad day again

if (baggage is not null)
{
if (NameValueHeaderValue.TryParse(baggage[i], out var baggageItem))
foreach (var baggageItem in baggage)
{
activity.AddBaggage(baggageItem.Name.ToString(), HttpUtility.UrlDecode(baggageItem.Value.ToString()));
activity.AddBaggage(baggageItem.Key, baggageItem.Value);
}
}
}
Expand Down
3 changes: 2 additions & 1 deletion src/Hosting/Hosting/src/Internal/WebHost.cs
Original file line number Diff line number Diff line change
Expand Up @@ -157,8 +157,9 @@ public async Task StartAsync(CancellationToken cancellationToken = default)

var diagnosticSource = _applicationServices.GetRequiredService<DiagnosticListener>();
var activitySource = _applicationServices.GetRequiredService<ActivitySource>();
var propagator = _applicationServices.GetRequiredService<DistributedContextPropagator>();
var httpContextFactory = _applicationServices.GetRequiredService<IHttpContextFactory>();
var hostingApp = new HostingApplication(application, _logger, diagnosticSource, activitySource, httpContextFactory);
var hostingApp = new HostingApplication(application, _logger, diagnosticSource, activitySource, propagator, httpContextFactory);
await Server.StartAsync(hostingApp, cancellationToken).ConfigureAwait(false);
_startedServer = true;

Expand Down
1 change: 1 addition & 0 deletions src/Hosting/Hosting/src/WebHostBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,7 @@ private IServiceCollection BuildCommonServices(out AggregateException? hostingSt
services.TryAddSingleton(sp => new DiagnosticListener("Microsoft.AspNetCore"));
services.TryAddSingleton<DiagnosticSource>(sp => sp.GetRequiredService<DiagnosticListener>());
services.TryAddSingleton(sp => new ActivitySource("Microsoft.AspNetCore"));
services.TryAddSingleton(DistributedContextPropagator.Current);

services.AddTransient<IApplicationBuilderFactory, ApplicationBuilderFactory>();
services.AddTransient<IHttpContextFactory, DefaultHttpContextFactory>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -397,7 +397,7 @@ public void ActivityBaggagePreservesItemsOrder()
KeyValuePair.Create("Key1","value3")
};

Assert.Equal(expectedBaggage, Activity.Current.Baggage);
Assert.Equal(expectedBaggage, Activity.Current.Baggage.ToArray());
}

[Fact]
Expand Down Expand Up @@ -552,6 +552,7 @@ private static HostingApplication CreateApplication(out FeatureCollection featur
logger ?? new NullScopeLogger(),
diagnosticListener ?? new NoopDiagnosticListener(),
activitySource ?? new ActivitySource("Microsoft.AspNetCore"),
DistributedContextPropagator.CreateDefaultPropagator(),
httpContextFactory.Object);

return hostingApplication;
Expand Down
1 change: 1 addition & 0 deletions src/Hosting/Hosting/test/HostingApplicationTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,7 @@ private static HostingApplication CreateApplication(IHttpContextFactory httpCont
NullLogger.Instance,
new DiagnosticListener("Microsoft.AspNetCore"),
activitySource ?? new ActivitySource("Microsoft.AspNetCore"),
DistributedContextPropagator.CreateDefaultPropagator(),
httpContextFactory);

return hostingApplication;
Expand Down