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

Remove ActivitySourceAdapter from AspNetCore #1654

Closed
wants to merge 2 commits into from
Closed

Remove ActivitySourceAdapter from AspNetCore #1654

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
627a878
Remove ActivitySourceAdapter from AspNetCore
eddynaka Dec 10, 2020
7dcb6f3
fixing operation name
eddynaka Dec 11, 2020
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
7 changes: 3 additions & 4 deletions src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreInstrumentation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,9 @@
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>

using System;
using OpenTelemetry.Instrumentation.AspNetCore.Implementation;
using OpenTelemetry.Trace;

namespace OpenTelemetry.Instrumentation.AspNetCore
{
Expand All @@ -29,11 +29,10 @@ internal class AspNetCoreInstrumentation : IDisposable
/// <summary>
/// Initializes a new instance of the <see cref="AspNetCoreInstrumentation"/> class.
/// </summary>
/// <param name="activitySource">ActivitySource adapter instance.</param>
/// <param name="options">Configuration options for ASP.NET Core instrumentation.</param>
public AspNetCoreInstrumentation(ActivitySourceAdapter activitySource, AspNetCoreInstrumentationOptions options)
public AspNetCoreInstrumentation(AspNetCoreInstrumentationOptions options)
{
this.diagnosticSourceSubscriber = new DiagnosticSourceSubscriber(new HttpInListener("Microsoft.AspNetCore", options, activitySource), null);
this.diagnosticSourceSubscriber = new DiagnosticSourceSubscriber(new HttpInListener("Microsoft.AspNetCore", options), null);
this.diagnosticSourceSubscriber.Subscribe();
}

Expand Down
43 changes: 20 additions & 23 deletions src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,12 @@ internal class HttpInListener : ListenerHandler
private readonly PropertyFetcher<string> beforeActionTemplateFetcher = new PropertyFetcher<string>("Template");
private readonly bool hostingSupportsW3C;
private readonly AspNetCoreInstrumentationOptions options;
private readonly ActivitySourceAdapter activitySource;

public HttpInListener(string name, AspNetCoreInstrumentationOptions options, ActivitySourceAdapter activitySource)
public HttpInListener(string name, AspNetCoreInstrumentationOptions options)
: base(name)
{
this.hostingSupportsW3C = typeof(HttpRequest).Assembly.GetName().Version.Major >= 3;
this.options = options ?? throw new ArgumentNullException(nameof(options));
this.activitySource = activitySource;
}

[System.Diagnostics.CodeAnalysis.SuppressMessage("Reliability", "CA2000:Dispose objects before losing scope", Justification = "The objects should not be disposed.")]
Expand Down Expand Up @@ -81,36 +79,37 @@ public override void OnStartActivity(Activity activity, object payload)
return;
}

ActivityContext activityContext = default;
Copy link
Member

Choose a reason for hiding this comment

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

rename to parentActivityContextOfOriginalActivity if you don't mind looong names.

Activity originalActivityCurrent = Activity.Current;
if (activity.ParentSpanId == default)
{
Activity.Current = null;
}
else
{
activityContext = new ActivityContext(activity.TraceId, activity.ParentSpanId, activity.ActivityTraceFlags, activity.TraceStateString, true);
}

var request = context.Request;
var textMapPropagator = Propagators.DefaultTextMapPropagator;
if (!this.hostingSupportsW3C || !(textMapPropagator is TraceContextPropagator))
{
var ctx = textMapPropagator.Extract(default, request, HttpRequestHeaderValuesGetter);

if (ctx.ActivityContext.IsValid()
&& ctx.ActivityContext != new ActivityContext(activity.TraceId, activity.ParentSpanId, activity.ActivityTraceFlags, activity.TraceStateString, true))
{
// 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.
Activity newOne = new Activity(ActivityNameByHttpInListener);
newOne.SetParentId(ctx.ActivityContext.TraceId, ctx.ActivityContext.SpanId, ctx.ActivityContext.TraceFlags);
newOne.TraceStateString = ctx.ActivityContext.TraceState;

// Starting the new activity make it the Activity.Current one.
newOne.Start();
activity = newOne;
}

activityContext = ctx.ActivityContext;
if (ctx.Baggage != default)
{
Baggage.Current = ctx.Baggage;
}
}

this.activitySource.Start(activity, ActivityKind.Server, ActivitySource);
activity = ActivitySource.StartActivity(ActivityNameByHttpInListener, ActivityKind.Server, activityContext);

if (activity.IsAllDataRequested)
if (activity == null)
{
Activity.Current = originalActivityCurrent;
}

if (activity?.IsAllDataRequested ?? false)
{
var path = (request.PathBase.HasValue || request.Path.HasValue) ? (request.PathBase + request.Path).ToString() : "/";
activity.DisplayName = path;
Expand Down Expand Up @@ -208,8 +207,6 @@ public override void OnStopActivity(Activity activity, object payload)
// the one created by the instrumentation.
// And retrieve it here, and set it to Current.
}

this.activitySource.Stop(activity);
Copy link
Member

Choose a reason for hiding this comment

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

It looks like AspNetCore hosting keeps a reference to the Activity it started and that's the one it will stop:

https://github.com/dotnet/aspnetcore/blob/4a7ed303fff26e4070e02e385fb22df484f6b81f/src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs#L134-L139

So my question is: Does the Activity we create in this approach actually get stopped?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@CodeBlanch , when you say "this approach", that means the old approach or this new one? For the new one, we are replacing the activity, but, if aspnetcore has a reference there, we might be creating a new one that would never be stopped.
@cijothomas to comment as well :)

Copy link
Member

Choose a reason for hiding this comment

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

@eddynaka I meant the new approach sorry 😄

For the new one, we are replacing the activity, but, if aspnetcore has a reference there, we might be creating a new one that would never be stopped.

Ya, exactly. That would be a problem!

Copy link
Member

Choose a reason for hiding this comment

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

we are stopping our own activity. aspnet core stops their activity. check line 197 for our activity being stopped.

(This is a diff. issue in AspNet, as it relies on Activity.Current, so we need to restore it for AspNet, but for AspNetCore we dont. But agree - this is a bit flaky design, and will fall apart if aspnetcore changes their implementation. We'll need to revisit this in .NET6, when they are expected to do some changes.)

Copy link
Member

Choose a reason for hiding this comment

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

@eddynaka Can you verify the one we're creating is stopped correctly? I see the activity.Stop on LN197 but we're stopping the same activity passed in OnStopActivity so it should already be stopped? 🤷

Copy link
Member

Choose a reason for hiding this comment

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

OnStopActivity gets Activity.Current, which should be the one we created, right?

Copy link
Member

Choose a reason for hiding this comment

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

Ya you're right it works fine. Just took it for a spin to verify.

}

public override void OnCustom(string name, Activity activity, object payload)
Expand Down
5 changes: 4 additions & 1 deletion src/OpenTelemetry.Instrumentation.AspNetCore/TracerProviderBuilderExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

using System;
using OpenTelemetry.Instrumentation.AspNetCore;
using OpenTelemetry.Instrumentation.AspNetCore.Implementation;

namespace OpenTelemetry.Trace
{
Expand All @@ -41,7 +42,9 @@ public static TracerProviderBuilder AddAspNetCoreInstrumentation(

var aspnetCoreOptions = new AspNetCoreInstrumentationOptions();
configureAspNetCoreInstrumentationOptions?.Invoke(aspnetCoreOptions);
builder.AddDiagnosticSourceInstrumentation((activitySource) => new AspNetCoreInstrumentation(activitySource, aspnetCoreOptions));

builder.AddSource(HttpInListener.ActivitySourceName);
builder.AddInstrumentation(() => new AspNetCoreInstrumentation(aspnetCoreOptions));

return builder;
}
Expand Down
6 changes: 0 additions & 6 deletions test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,15 +171,9 @@ public async Task SuccessfulTemplateControllerCallUsesParentContext()
Assert.Equal(3, activityProcessor.Invocations.Count); // begin and end was called
var activity = (Activity)activityProcessor.Invocations[2].Arguments[0];

#if !NETCOREAPP2_1
// ASP.NET Core after 2.x is W3C aware and hence Activity created by it
// must be used.
Assert.Equal("Microsoft.AspNetCore.Hosting.HttpRequestIn", activity.OperationName);
#else
// ASP.NET Core before 3.x is not W3C aware and hence Activity created by it
// is always ignored and new one is created by the Instrumentation
Assert.Equal("ActivityCreatedByHttpInListener", activity.OperationName);
#endif
Assert.Equal(ActivityKind.Server, activity.Kind);
Assert.Equal("api/Values/{id}", activity.DisplayName);
Assert.Equal("/api/values/2", activity.GetTagValue(SpanAttributeConstants.HttpPathKey) as string);
Expand Down
8 changes: 6 additions & 2 deletions ....Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,15 +59,18 @@ public async Task SuccessfulTemplateControllerCallGeneratesASpan(
bool recordException = false)
{
var processor = new Mock<BaseProcessor<Activity>>();
TracerProvider tracerProvider = null;

// Arrange
using (var client = this.factory
.WithWebHostBuilder(builder =>
builder.ConfigureTestServices((IServiceCollection services) =>
{
services.AddSingleton<CallbackMiddleware.CallbackMiddlewareImpl>(new TestCallbackMiddlewareImpl(statusCode, reasonPhrase));
services.AddOpenTelemetryTracing((builder) => builder.AddAspNetCoreInstrumentation(options => options.RecordException = recordException)
.AddProcessor(processor.Object));
tracerProvider = Sdk.CreateTracerProviderBuilder()
.AddAspNetCoreInstrumentation(options => options.RecordException = recordException)
.AddProcessor(processor.Object)
.Build();
}))
.CreateClient())
{
Expand Down Expand Up @@ -140,6 +143,7 @@ public async Task SuccessfulTemplateControllerCallGeneratesASpan(

activity.Dispose();
processor.Object.Dispose();
tracerProvider?.Dispose();
}

private void ValidateTagValue(Activity activity, string attribute, string expectedValue)
Expand Down