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

Migrate to ActivitySource #30089

Merged
7 commits merged into from
Feb 25, 2021
Merged

Migrate to ActivitySource #30089

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0f10c14
WIP: Migrate to ActivitySource
shirhatti Feb 11, 2021
8ce8411
Add ActivityListener test
shirhatti Feb 11, 2021
d3ff481
Fix remaining tests
shirhatti Feb 12, 2021
224912b
Remove dummy listener
shirhatti Feb 23, 2021
fe50c4b
Use ActivitySource.CreateActivity APIs
shirhatti Feb 23, 2021
b2b8ee6
Undo changes to Activity.Stop
shirhatti Feb 23, 2021
606da28
Undo test change
shirhatti Feb 23, 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
28 changes: 19 additions & 9 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,6 +24,9 @@ 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 DiagnosticListener _diagnosticListener;
private readonly ILogger _logger;

Expand All @@ -46,11 +49,13 @@ public void BeginRequest(HttpContext httpContext, HostingApplication.Context con
}

var diagnosticListenerEnabled = _diagnosticListener.IsEnabled();
var diagnosticListenerActivityCreationEnabled = (diagnosticListenerEnabled && _diagnosticListener.IsEnabled(ActivityName, httpContext));
var loggingEnabled = _logger.IsEnabled(LogLevel.Critical);

if (loggingEnabled || (diagnosticListenerEnabled && _diagnosticListener.IsEnabled(ActivityName, httpContext)))

if (loggingEnabled || diagnosticListenerActivityCreationEnabled || _activitySource.HasListeners())
{
context.Activity = StartActivity(httpContext, out var hasDiagnosticListener);
context.Activity = StartActivity(httpContext, loggingEnabled, diagnosticListenerActivityCreationEnabled, out var hasDiagnosticListener);
context.HasDiagnosticListener = hasDiagnosticListener;
}

Expand Down Expand Up @@ -245,11 +250,20 @@ private static void RecordRequestStartEventLog(HttpContext httpContext)
}

[MethodImpl(MethodImplOptions.NoInlining)]
private Activity StartActivity(HttpContext httpContext, out bool hasDiagnosticListener)
private Activity? StartActivity(HttpContext httpContext, bool loggingEnabled, bool diagnosticListenerActivityCreationEnabled, out bool hasDiagnosticListener)
{
var activity = new Activity(ActivityName);
var activity = _activitySource.CreateActivity(ActivityName, ActivityKind.Server);
Copy link
Member

Choose a reason for hiding this comment

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

var [](start = 12, length = 3)

does it make sense to check _activitySource.HasListeners() before calling _activitySource.CreateActivity if you want save some cycles when there is no listeners?

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 don't think it'll make a difference. You already do the check in ActivitySource.CreateActivity- https://github.com/dotnet/runtime/blob/master/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs#L172

if (activity is null && (loggingEnabled || diagnosticListenerActivityCreationEnabled))
{
activity = new Activity(ActivityName);
}
hasDiagnosticListener = false;

if (activity is null)
{
return null;
shirhatti marked this conversation as resolved.
Show resolved Hide resolved
}

var headers = httpContext.Request.Headers;
if (!headers.TryGetValue(HeaderNames.TraceParent, out var requestId))
{
Expand Down Expand Up @@ -305,10 +319,7 @@ private void StopActivity(HttpContext httpContext, Activity activity, bool hasDi
{
StopActivity(activity, httpContext);
}
else
{
activity.Stop();
}
activity.Stop();
}

// These are versions of DiagnosticSource.Start/StopActivity that don't allocate strings per call (see https://github.com/dotnet/corefx/issues/37055)
Expand All @@ -328,7 +339,6 @@ private void StopActivity(Activity activity, HttpContext httpContext)
activity.SetEndTime(DateTime.UtcNow);
}
_diagnosticListener.Write(ActivityStopKey, httpContext);
activity.Stop(); // Resets Activity.Current (we want this after the Write)
}
}
}
35 changes: 33 additions & 2 deletions src/Hosting/Hosting/test/HostingApplicationDiagnosticsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,7 @@ public void ActivityBaggagePrefersW3CBaggageHeaderName()
Assert.Contains(Activity.Current.Baggage, pair => pair.Key == "Key2" && pair.Value == "value4");
}


[Fact]
public void ActivityBaggagePreservesItemsOrder()
{
Expand Down Expand Up @@ -465,7 +466,7 @@ public void ActivityTraceParentAndTraceStateFromHeaders()
}

[Fact]
public void ActivityOnExportHookIsCalled()
public void ActivityOnImportHookIsCalled()
shirhatti marked this conversation as resolved.
Show resolved Hide resolved
{
var diagnosticListener = new DiagnosticListener("DummySource");
var hostingApplication = CreateApplication(out var features, diagnosticListener: diagnosticListener);
Expand All @@ -477,7 +478,9 @@ public void ActivityOnExportHookIsCalled()
onActivityImport: (activity, context) =>
{
onActivityImportCalled = true;
Assert.Null(Activity.Current);
// Review: Breaking change
// Since we fire OnActivityImport after Activity.Start(), it will no longer be not null
//Assert.Null(Activity.Current);
Assert.Equal("Microsoft.AspNetCore.Hosting.HttpRequestIn", activity.OperationName);
Assert.NotNull(context);
Assert.IsAssignableFrom<HttpContext>(context);
Expand All @@ -492,6 +495,34 @@ public void ActivityOnExportHookIsCalled()
Assert.True(Activity.Current.Recorded);
}

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

ActivitySource.AddActivityListener(listener);

features.Set<IHttpRequestFeature>(new HttpRequestFeature()
{
Headers = new HeaderDictionary()
{
{"traceparent", "00-0123456789abcdef0123456789abcdef-0123456789abcdef-01"},
{"tracestate", "TraceState1"},
{"baggage", "Key1=value1, Key2=value2"}
}
});
hostingApplication.CreateContext(features);
}


private static void AssertProperty<T>(object o, string name)
{
Expand Down