Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Removed static activity source from hosting #31483

Merged
merged 2 commits into from
Apr 2, 2021
Merged

Removed static activity source from hosting #31483

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
293f778
Removed static activity source from hosting
davidfowl Apr 2, 2021
8fa7f7b
Fixed test
davidfowl Apr 2, 2021
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
1 change: 1 addition & 0 deletions src/Hosting/Hosting/src/GenericHost/GenericWebHostBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ public GenericWebHostBuilder(IHostBuilder builder, WebHostBuilderOptions options
// We need to flow this differently
services.TryAddSingleton(sp => new DiagnosticListener("Microsoft.AspNetCore"));
services.TryAddSingleton<DiagnosticSource>(sp => sp.GetRequiredService<DiagnosticListener>());
services.TryAddSingleton(sp => new ActivitySource("Microsoft.AspNetCore"));
Copy link
Member

Choose a reason for hiding this comment

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

Why did the value change from "Microsoft.AspNetCore.Hosting"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Matches the diagnostic listener. @shirhatti are they used in a similar way?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is technically a breaking change. You've changed the ActivitySource name. Most subscribers only care about the Activity name, but I'd prefer not to have this change

Copy link
Member Author

Choose a reason for hiding this comment

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

Didn't we make this change in 6.0?

Copy link
Contributor

Choose a reason for hiding this comment

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

The original ActivitySource guidance was to have something like ActivitySource Name = "Microsoft.AspNetCore.Hosting" and Activity Name = "BeginRequest", that is don't use a fully qualified name for the Activity. However, it wasn't possible to do that without breaking existing consumers, so we just changed the guidance for Activity 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, new in 6.0

Copy link
Member Author

Choose a reason for hiding this comment

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

Then it's not a breaking change. We just need the "right" naming.


services.TryAddSingleton<IHttpContextFactory, DefaultHttpContextFactory>();
services.TryAddScoped<IMiddlewareFactory, MiddlewareFactory>();
Expand Down
5 changes: 4 additions & 1 deletion src/Hosting/Hosting/src/GenericHost/GenericWebHostedService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ public GenericWebHostService(IOptions<GenericWebHostServiceOptions> options,
IServer server,
ILoggerFactory loggerFactory,
DiagnosticListener diagnosticListener,
ActivitySource activitySource,
IHttpContextFactory httpContextFactory,
IApplicationBuilderFactory applicationBuilderFactory,
IEnumerable<IStartupFilter> startupFilters,
Expand All @@ -35,6 +36,7 @@ public GenericWebHostService(IOptions<GenericWebHostServiceOptions> options,
Logger = loggerFactory.CreateLogger("Microsoft.AspNetCore.Hosting.Diagnostics");
LifetimeLogger = loggerFactory.CreateLogger("Microsoft.Hosting.Lifetime");
DiagnosticListener = diagnosticListener;
ActivitySource = activitySource;
HttpContextFactory = httpContextFactory;
ApplicationBuilderFactory = applicationBuilderFactory;
StartupFilters = startupFilters;
Expand All @@ -48,6 +50,7 @@ public GenericWebHostService(IOptions<GenericWebHostServiceOptions> options,
// Only for high level lifetime events
public ILogger LifetimeLogger { get; }
public DiagnosticListener DiagnosticListener { get; }
public ActivitySource ActivitySource { get; }
public IHttpContextFactory HttpContextFactory { get; }
public IApplicationBuilderFactory ApplicationBuilderFactory { get; }
public IEnumerable<IStartupFilter> StartupFilters { get; }
Expand Down Expand Up @@ -111,7 +114,7 @@ public async Task StartAsync(CancellationToken cancellationToken)
application = ErrorPageBuilder.BuildErrorPageApplication(HostingEnvironment.ContentRootFileProvider, Logger, showDetailedErrors, ex);
}

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

await Server.StartAsync(httpApplication, cancellationToken);

Expand Down
3 changes: 2 additions & 1 deletion src/Hosting/Hosting/src/Internal/HostingApplication.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,11 @@ public HostingApplication(
RequestDelegate application,
ILogger logger,
DiagnosticListener diagnosticSource,
ActivitySource activitySource,
IHttpContextFactory httpContextFactory)
{
_application = application;
_diagnostics = new HostingApplicationDiagnostics(logger, diagnosticSource);
_diagnostics = new HostingApplicationDiagnostics(logger, diagnosticSource, activitySource);
if (httpContextFactory is DefaultHttpContextFactory factory)
{
_defaultHttpContextFactory = factory;
Expand Down
7 changes: 3 additions & 4 deletions src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,16 +24,15 @@ internal class HostingApplicationDiagnostics
private const string DeprecatedDiagnosticsEndRequestKey = "Microsoft.AspNetCore.Hosting.EndRequest";
private const string DiagnosticsUnhandledExceptionKey = "Microsoft.AspNetCore.Hosting.UnhandledException";

private const string ActivitySourceName = "Microsoft.AspNetCore.Hosting";
private static readonly ActivitySource _activitySource = new ActivitySource(ActivitySourceName);

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

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

[MethodImpl(MethodImplOptions.AggressiveInlining)]
Expand Down
3 changes: 2 additions & 1 deletion src/Hosting/Hosting/src/Internal/WebHost.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,8 +155,9 @@ public virtual async Task StartAsync(CancellationToken cancellationToken = defau
await _hostedServiceExecutor.StartAsync(cancellationToken).ConfigureAwait(false);

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

Expand Down
4 changes: 4 additions & 0 deletions src/Hosting/Hosting/src/WebHostBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,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.AddTransient<IApplicationBuilderFactory, ApplicationBuilderFactory>();
services.AddTransient<IHttpContextFactory, DefaultHttpContextFactory>();
Expand Down Expand Up @@ -346,6 +347,9 @@ private void AddApplicationServices(IServiceCollection services, IServiceProvide
var listener = hostingServiceProvider.GetService<DiagnosticListener>();
services.Replace(ServiceDescriptor.Singleton(typeof(DiagnosticListener), listener!));
services.Replace(ServiceDescriptor.Singleton(typeof(DiagnosticSource), listener!));

var activitySource = hostingServiceProvider.GetService<ActivitySource>();
services.Replace(ServiceDescriptor.Singleton(typeof(ActivitySource), activitySource!));
}

private string ResolveContentRootPath(string contentRootPath, string basePath)
Expand Down
12 changes: 8 additions & 4 deletions src/Hosting/Hosting/test/HostingApplicationDiagnosticsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,7 @@ public void ActivityBaggagePreservesItemsOrder()
hostingApplication.CreateContext(features);
Assert.Equal("Microsoft.AspNetCore.Hosting.HttpRequestIn", Activity.Current.OperationName);

var expectedBaggage = new []
var expectedBaggage = new[]
{
KeyValuePair.Create("Key1","value1"),
KeyValuePair.Create("Key2","value2"),
Expand Down Expand Up @@ -493,17 +493,18 @@ public void ActivityOnImportHookIsCalled()
Assert.True(Activity.Current.Recorded);
}

[Fact(Skip = "https://github.com/dotnet/aspnetcore/issues/30582")]
[Fact]
public void ActivityListenersAreCalled()
{
var hostingApplication = CreateApplication(out var features);
var parentSpanId = "";
using var listener = new ActivityListener
{
ShouldListenTo = activitySource => true,
Sample = (ref ActivityCreationOptions<ActivityContext> _) => ActivitySamplingResult.AllData,
ActivityStarted = activity =>
{
Assert.Equal("0123456789abcdef", Activity.Current.ParentSpanId.ToHexString());
parentSpanId = Activity.Current.ParentSpanId.ToHexString();
}
};

Expand All @@ -518,7 +519,9 @@ public void ActivityListenersAreCalled()
{"baggage", "Key1=value1, Key2=value2"}
}
});

hostingApplication.CreateContext(features);
Assert.Equal("0123456789abcdef", parentSpanId);
}


Expand All @@ -533,7 +536,7 @@ private static void AssertProperty<T>(object o, string name)
}

private static HostingApplication CreateApplication(out FeatureCollection features,
DiagnosticListener diagnosticListener = null, ILogger logger = null, Action<DefaultHttpContext> configure = null)
DiagnosticListener diagnosticListener = null, ActivitySource activitySource = null, ILogger logger = null, Action<DefaultHttpContext> configure = null)
{
var httpContextFactory = new Mock<IHttpContextFactory>();

Expand All @@ -548,6 +551,7 @@ private static HostingApplication CreateApplication(out FeatureCollection featur
ctx => Task.CompletedTask,
logger ?? new NullScopeLogger(),
diagnosticListener ?? new NoopDiagnosticListener(),
activitySource ?? new ActivitySource("Microsoft.AspNetCore"),
httpContextFactory.Object);

return hostingApplication;
Expand Down
1 change: 1 addition & 0 deletions src/Hosting/Hosting/test/HostingApplicationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ private static HostingApplication CreateApplication(IHttpContextFactory httpCont
ctx => Task.CompletedTask,
NullLogger.Instance,
new DiagnosticListener("Microsoft.AspNetCore"),
new ActivitySource("Microsoft.AspNetCore"),
httpContextFactory);

return hostingApplication;
Expand Down