#3490 follow up (Fix asp.net core middleware activity modification issue)

[vishweshbankwar]

Follow up to #3490

Changes

Please provide a brief description of the changes here.

For significant contributions please make sure you have completed the following items:

• Appropriate CHANGELOG.md updated for non-trivial changes
• Design discussion issue #
• Changes in public API reviewed

[codecov]

Codecov Report

Merging #3498 (5038cc9) into main (99b49b6) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3498      +/-   ##
==========================================
- Coverage   87.46%   87.42%   -0.04%     
==========================================
  Files         278      278              
  Lines       10082    10087       +5     
==========================================
+ Hits         8818     8819       +1     
- Misses       1264     1268       +4     

Impacted Files	Coverage Δ
...tation.AspNetCore/Implementation/HttpInListener.cs	90.18% <100.00%> (+0.31%)	⬆️
...Listener/Internal/PrometheusExporterEventSource.cs	16.66% <0.00%> (-11.12%)	⬇️
...ZPages/Implementation/ZPagesExporterEventSource.cs	56.25% <0.00%> (-6.25%)	⬇️
....Prometheus.HttpListener/PrometheusHttpListener.cs	78.66% <0.00%> (-4.00%)	⬇️
...ter.ZPages/Implementation/ZPagesActivityTracker.cs	97.14% <0.00%> (-2.86%)	⬇️
src/OpenTelemetry/Logs/OpenTelemetryLogger.cs	88.88% <0.00%> (+2.22%)	⬆️
...tation/OpenTelemetryProtocolExporterEventSource.cs	85.00% <0.00%> (+10.00%)	⬆️

[alanwest]

Are we sure that activity.Parent != null will always be the activity we're looking for? I'm trying to think of a scenario where an activity could be created before the ASP.NET Core created activity... though I haven't studied the flow of ASP.NET Core enough to think of one.

Also, seems pretty unlikely, but could this also be at risk of an infinite loop? Like A is the parent of B is the parent of A.

[alanwest]

Was just reviewing the conversation on the PR for grpc-dotnet that does the same thing you're doing. grpc/grpc-dotnet#341 (comment).

Seems if infinite loop was a concern it would have been a concern for them as well. However, I'm not finding a good reason why they're looking for the activity by OperationName versus the simpler thing you're doing here.

[vishweshbankwar]

Also, seems pretty unlikely, but could this also be at risk of an infinite loop? Like A is the parent of B is the parent of A.

-- Do you know any specific case when this could be possible? not able to come up with a specific case.

I'm trying to think of a scenario where an activity could be created before the ASP.NET Core created activity... though I haven't studied the flow of ASP.NET Core enough to think of one.

-- I am not completely sure but here is what I was looking at - ASP.NET Core extracts parent from remote traceparent and sets the parent properties on activity it starts. If there was a possibility of the activity being created before asp.net core one it would have some check here?

[vishweshbankwar]

Was just reviewing the conversation on the PR for grpc-dotnet that does the same thing you're doing. grpc/grpc-dotnet#341 (comment).

Seems if infinite loop was a concern it would have been a concern for them as well. However, I'm not finding a good reason why they're looking for the activity by OperationName versus the simpler thing you're doing here.

did not see this ^ before my previous comment

why they're looking for the activity by OperationName versus the simpler thing you're doing here

I have same question too :)

[cijothomas]

 private Activity? GetHostActivity()
        {
            var activity = Activity.Current;
            while (activity != null)
            {
                // We only want to add gRPC metadata to the host activity
                // Search parent activities in case a new activity was started in middleware before gRPC endpoint is invoked
                if (string.Equals(activity.OperationName, GrpcServerConstants.HostActivityName, StringComparison.Ordinal))
                {
                    return activity;
                }

                activity = activity.Parent;
            }

            return null;
        }

https://github.com/grpc/grpc-dotnet/pull/341/files#diff-2f74a9233c4e6d1aa1bee085bb9c98881e6bb2c2c30f62132c5423d9f4642d02R384 This seems the right one. We need to find the activity created by asp.net hosting, and we are willing to walk the activity parent chain until we find it or we give up when we no longer have a parent.

[vishweshbankwar]

@cijothomas - My assumption is the root activity will always be the asp.net core activity. Is there any case this will not be true?

[alanwest]

I'm not familiar enough with the use case for why someone would do this, but it looks like you can configure the service with a custom IHttpContextFactory:

https://github.com/dotnet/aspnetcore/blob/93d88ac001b54db169aec5c088a8469309aaf30e/src/Hosting/Hosting/src/Internal/WebHost.cs#L154-L155

It's used to create the context here - which happens before the ASP.NET Core activity is created.

https://github.com/dotnet/aspnetcore/blob/93d88ac001b54db169aec5c088a8469309aaf30e/src/Hosting/Hosting/src/Internal/HostingApplication.cs#L76

[alanwest]

Ok so maybe this is super unusual, but I was able to produce this situation.. if you modify your test setup with

builder.ConfigureTestServices((IServiceCollection services) =>
{
    services.AddSingleton<IHttpContextFactory, TestHttpContextFactory>();
    services.AddSingleton<ActivityMiddleware.ActivityMiddlewareImpl>(new TestActivityMiddlewareImpl(activitySourceName, activityName));
    services.AddOpenTelemetryTracing((builder) => builder.AddAspNetCoreInstrumentation()
        .AddSource(activitySourceName)
        .AddSource("HttpContextFactoryActivitySource")
        .AddInMemoryExporter(exportedItems));
}))

Here's what TestHttpContextFactory looks like

 private class TestHttpContextFactory : IHttpContextFactory
 {
    private IHttpContextFactory innerFactory;
    private ActivitySource activitySource;

    public TestHttpContextFactory(IServiceProvider serviceProvider)
    {
        this.innerFactory = new DefaultHttpContextFactory(serviceProvider);
        this.activitySource = new ActivitySource("HttpContextFactoryActivitySource");
    }

    public HttpContext Create(IFeatureCollection featureCollection)
    {
        var activity = activitySource.StartActivity("ActivityFromHttpContextFactory");
        var httpContext = this.innerFactory.Create(featureCollection);
        httpContext.Items["MyActivity"] = activity;
        return httpContext;
    }

    public void Dispose(HttpContext httpContext)
    {
        (httpContext.Items["MyActivity"] as Activity).Stop();
        this.innerFactory.Dispose(httpContext);
    }
}

[vishweshbankwar]

Interesting!. I have updated the logic to now check for operation name. Thanks for helping with clarification.