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.

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

[.NET7.0] AspNetCore ActivitySource Migration #3391

Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,33 @@ internal class AspNetCoreInstrumentation : IDisposable
{
private readonly DiagnosticSourceSubscriber diagnosticSourceSubscriber;

private readonly Func<string, object, object, bool> isEnabled = (activityName, obj1, obj2) =>
{
if (activityName.Equals("Microsoft.AspNetCore.Hosting.HttpRequestIn"))
{
return false;
}

return true;
};

public AspNetCoreInstrumentation(HttpInListener httpInListener)
{
this.diagnosticSourceSubscriber = new DiagnosticSourceSubscriber(httpInListener, null);
// For .NET7.0 activity will be created using activitySource.
// https://github.com/dotnet/aspnetcore/blob/main/src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs#L327
// However, in case when activity creation returns null (due to sampling)
vishweshbankwar marked this conversation as resolved.
Show resolved Hide resolved
// the framework will fall back to creating activity anyways due to active diagnostic source listener
// To prevent this, isEnabled is implemented which will return false always
// so that the sampler's decision is respected.
if (HttpInListener.IsNet7OrGreater)
{
this.diagnosticSourceSubscriber = new DiagnosticSourceSubscriber(httpInListener, this.isEnabled);
}
else
{
this.diagnosticSourceSubscriber = new DiagnosticSourceSubscriber(httpInListener, null);
}

this.diagnosticSourceSubscriber.Subscribe();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@ namespace OpenTelemetry.Instrumentation.AspNetCore.Implementation
internal class HttpInListener : ListenerHandler
{
internal const string ActivityOperationName = "Microsoft.AspNetCore.Hosting.HttpRequestIn";
internal static readonly bool IsNet7OrGreater;
vishweshbankwar marked this conversation as resolved.
Show resolved Hide resolved

// https://github.com/dotnet/aspnetcore/blob/main/src/Hosting/Hosting/src/WebHostBuilder.cs#L291
internal static readonly string FrameworkActivitySourceName = "Microsoft.AspNetCore";
alanwest marked this conversation as resolved.
Show resolved Hide resolved
vishweshbankwar marked this conversation as resolved.
Show resolved Hide resolved
internal static readonly AssemblyName AssemblyName = typeof(HttpInListener).Assembly.GetName();
internal static readonly string ActivitySourceName = AssemblyName.Name;
internal static readonly Version Version = AssemblyName.Version;
Expand All @@ -50,6 +54,18 @@ internal class HttpInListener : ListenerHandler
private readonly PropertyFetcher<string> beforeActionTemplateFetcher = new("Template");
private readonly AspNetCoreInstrumentationOptions options;

static HttpInListener()
{
try
{
IsNet7OrGreater = typeof(HttpRequest).Assembly.GetName().Version.Major >= 7;
}
catch (Exception)
{
IsNet7OrGreater = false;
}
}

public HttpInListener(AspNetCoreInstrumentationOptions options)
: base(DiagnosticSourceName)
{
Expand All @@ -71,7 +87,7 @@ public override void OnStartActivity(Activity activity, object payload)
// By this time, samplers have already run and
// activity.IsAllDataRequested populated accordingly.

if (Sdk.SuppressInstrumentation)
if (Sdk.SuppressInstrumentation || (IsNet7OrGreater && string.IsNullOrEmpty(activity.Source.Name)))
{
return;
}
Expand All @@ -96,8 +112,15 @@ public override void OnStartActivity(Activity activity, object payload)
// Create a new activity with its parent set from the extracted context.
// This makes the new activity as a "sibling" of the activity created by
// Asp.Net Core.
#if NET7_0_OR_GREATER
// For NET7.0 onwards activity is created using ActivitySource so,
// we will use the source of the activity to create the new one.
Activity newOne;
newOne = activity.Source.CreateActivity(ActivityOperationName, ActivityKind.Server, ctx.ActivityContext);
vishweshbankwar marked this conversation as resolved.
Show resolved Hide resolved
#else
Activity newOne = new Activity(ActivityOperationName);
newOne.SetParentId(ctx.ActivityContext.TraceId, ctx.ActivityContext.SpanId, ctx.ActivityContext.TraceFlags);
#endif
newOne.TraceStateString = ctx.ActivityContext.TraceState;

newOne.SetTag("IsCreatedByInstrumentation", bool.TrueString);
Expand Down Expand Up @@ -135,8 +158,11 @@ public override void OnStartActivity(Activity activity, object payload)
return;
}

ActivityInstrumentationHelper.SetActivitySourceProperty(activity, ActivitySource);
ActivityInstrumentationHelper.SetKindProperty(activity, ActivityKind.Server);
if (!IsNet7OrGreater)
Copy link
Member

Choose a reason for hiding this comment

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

why we have used pre-processor directives (NET7_0_OR_GREATER) in some places and the static variable in other place?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point - we should use preprocessor directive everywhere. let me know if there is any case where it wont work

{
ActivityInstrumentationHelper.SetActivitySourceProperty(activity, ActivitySource);
ActivityInstrumentationHelper.SetKindProperty(activity, ActivityKind.Server);
}

var path = (request.PathBase.HasValue || request.Path.HasValue) ? (request.PathBase + request.Path).ToString() : "/";
activity.DisplayName = path;
Expand Down Expand Up @@ -177,6 +203,11 @@ public override void OnStartActivity(Activity activity, object payload)

public override void OnStopActivity(Activity activity, object payload)
{
if (IsNet7OrGreater && string.IsNullOrEmpty(activity.Source.Name))
vishweshbankwar marked this conversation as resolved.
Show resolved Hide resolved
{
return;
}

if (activity.IsAllDataRequested)
{
_ = this.stopContextFetcher.TryFetch(payload, out HttpContext context);
Expand Down Expand Up @@ -244,6 +275,11 @@ public override void OnStopActivity(Activity activity, object payload)

public override void OnCustom(string name, Activity activity, object payload)
{
if (IsNet7OrGreater && string.IsNullOrEmpty(activity.Source.Name))
{
return;
}

if (name == "Microsoft.AspNetCore.Mvc.BeforeAction")
{
if (activity.IsAllDataRequested)
Expand Down Expand Up @@ -273,6 +309,11 @@ public override void OnCustom(string name, Activity activity, object payload)

public override void OnException(Activity activity, object payload)
{
if (IsNet7OrGreater && string.IsNullOrEmpty(activity.Source.Name))
{
return;
}

if (activity.IsAllDataRequested)
{
if (!this.stopExceptionFetcher.TryFetch(payload, out Exception exc) || exc == null)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,16 @@ internal static TracerProviderBuilder AddAspNetCoreInstrumentation(
this TracerProviderBuilder builder,
AspNetCoreInstrumentation instrumentation)
{
builder.AddSource(HttpInListener.ActivitySourceName);
builder.AddLegacySource(HttpInListener.ActivityOperationName); // for the activities created by AspNetCore
if (HttpInListener.IsNet7OrGreater)
{
builder.AddSource(HttpInListener.FrameworkActivitySourceName);
vishweshbankwar marked this conversation as resolved.
Show resolved Hide resolved
}
else
{
builder.AddSource(HttpInListener.ActivitySourceName);
builder.AddLegacySource(HttpInListener.ActivityOperationName); // for the activities created by AspNetCore
}

return builder.AddInstrumentation(() => instrumentation);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,7 @@ public async Task ExtractContextIrrespectiveOfSamplingDecision(SamplingDecision
var expectedTraceId = ActivityTraceId.CreateRandom();
var expectedParentSpanId = ActivitySpanId.CreateRandom();
var expectedTraceState = "rojo=1,congo=2";
var activityContext = new ActivityContext(expectedTraceId, expectedParentSpanId, ActivityTraceFlags.Recorded, expectedTraceState);
var activityContext = new ActivityContext(expectedTraceId, expectedParentSpanId, ActivityTraceFlags.Recorded, expectedTraceState, true);
vishweshbankwar marked this conversation as resolved.
Show resolved Hide resolved
var expectedBaggage = Baggage.SetBaggage("key1", "value1").SetBaggage("key2", "value2");
Sdk.SetDefaultTextMapPropagator(new ExtractOnlyPropagator(activityContext, expectedBaggage));

Expand Down Expand Up @@ -575,8 +575,13 @@ private static void WaitForActivityExport(List<Activity> exportedItems, int coun
private static void ValidateAspNetCoreActivity(Activity activityToValidate, string expectedHttpPath)
{
Assert.Equal(ActivityKind.Server, activityToValidate.Kind);
#if NET7_0_OR_GREATER
Assert.Equal(HttpInListener.FrameworkActivitySourceName, activityToValidate.Source.Name);
Assert.Empty(activityToValidate.Source.Version);
vishweshbankwar marked this conversation as resolved.
Show resolved Hide resolved
#else
Assert.Equal(HttpInListener.ActivitySourceName, activityToValidate.Source.Name);
Assert.Equal(HttpInListener.Version.ToString(), activityToValidate.Source.Version);
#endif
Assert.Equal(expectedHttpPath, activityToValidate.GetTagValue(SemanticConventions.AttributeHttpTarget) as string);
}

Expand Down