Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 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 @@ -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;

// https://github.com/dotnet/aspnetcore/blob/bf3352f2422bf16fa3ca49021f0e31961ce525eb/src/Hosting/Hosting/src/WebHostBuilder.cs#L291
internal static readonly string FrameworkActivitySourceName = "Microsoft.AspNetCore";
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 Down Expand Up @@ -96,8 +112,14 @@ 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 = activity.Source.CreateActivity(ActivityOperationName, ActivityKind.Server, ctx.ActivityContext);
#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 +157,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
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
// </copyright>

using System;
using System.Diagnostics;
using Microsoft.Extensions.DependencyInjection;
using OpenTelemetry.Instrumentation.AspNetCore;
using OpenTelemetry.Instrumentation.AspNetCore.Implementation;
using OpenTelemetry.Internal;
Expand Down Expand Up @@ -42,7 +44,7 @@ public static TracerProviderBuilder AddAspNetCoreInstrumentation(
{
return deferredTracerProviderBuilder.Configure((sp, builder) =>
{
AddAspNetCoreInstrumentation(builder, sp.GetOptions<AspNetCoreInstrumentationOptions>(), configureAspNetCoreInstrumentationOptions);
AddAspNetCoreInstrumentation(builder, sp.GetOptions<AspNetCoreInstrumentationOptions>(), configureAspNetCoreInstrumentationOptions, sp);
});
}

Expand All @@ -51,22 +53,45 @@ public static TracerProviderBuilder AddAspNetCoreInstrumentation(

internal static TracerProviderBuilder AddAspNetCoreInstrumentation(
this TracerProviderBuilder builder,
AspNetCoreInstrumentation instrumentation)
AspNetCoreInstrumentation instrumentation,
IServiceProvider serviceProvider = null)
{
builder.AddSource(HttpInListener.ActivitySourceName);
builder.AddLegacySource(HttpInListener.ActivityOperationName); // for the activities created by AspNetCore
// For .NET7.0 onwards activity will be created using activitySource.
// https://github.com/dotnet/aspnetcore/blob/bf3352f2422bf16fa3ca49021f0e31961ce525eb/src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs#L327
// For .NET6.0 and below, we will continue to use legacy way.
if (HttpInListener.IsNet7OrGreater)
{
var activitySourceService = serviceProvider?.GetService<ActivitySource>();
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if this is safe. Anyone can add ActivitySource to DI, and how do we know if this is the one added by asp.net core?

Copy link
Member Author

@vishweshbankwar vishweshbankwar Jul 15, 2022

Choose a reason for hiding this comment

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

if (activitySourceService != null)
{
builder.AddSource(activitySourceService.Name);
}
else
{
// For users not using hosting package?
builder.AddSource(HttpInListener.FrameworkActivitySourceName);
}
}
else
{
builder.AddSource(HttpInListener.ActivitySourceName);
builder.AddLegacySource(HttpInListener.ActivityOperationName); // for the activities created by AspNetCore
}

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

private static TracerProviderBuilder AddAspNetCoreInstrumentation(
TracerProviderBuilder builder,
AspNetCoreInstrumentationOptions options,
Action<AspNetCoreInstrumentationOptions> configure = null)
Action<AspNetCoreInstrumentationOptions> configure = null,
IServiceProvider serviceProvider = null)
{
configure?.Invoke(options);
return AddAspNetCoreInstrumentation(
builder,
new AspNetCoreInstrumentation(new HttpInListener(options)));
new AspNetCoreInstrumentation(new HttpInListener(options)),
serviceProvider);
}
}
}
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);
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);
#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