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

TestServer.CreateClient() produces an HttpClient that does not auto-inject distributed tracing when Activity is used #24633

Open
danile-ms opened this issue Aug 6, 2020 · 5 comments
Labels
affected-very-few This issue impacts very few customers area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions bug This issue describes a behavior which is not expected - a bug. severity-nice-to-have This label is used by an internal tool
Milestone

Comments

@danile-ms
Copy link

Describe the bug

CreateClient() produces an HttpClient object that does not auto-inject correlation data when a System.Diagnostics.Activity is instantiated and started (using the Start() method).

Background information about how to use Activity to send correlation data can be found here: https://devblogs.microsoft.com/aspnet/improvements-in-net-core-3-0-for-troubleshooting-and-monitoring-distributed-apps/ (refer to the "Initiate distributed trace in .NET Core 3.0 app" section).

Though the (docs.microsoft.com)"Test ASP.NET Core middleware" guidance states that TestServer "does not try to replicate all HttpClient behavior", having TestServer create an HttpClient object such that propagating any correlation data that may be found in the HTTP request header would make functional/integration tests of middleware more complete and righteous as distributed tracing & correlation support is significant to the story of developing & testing ASP.NET Core apps.

To Reproduce

Suppose we had a test method like the sample one in (docs.microsoft.com)"Send requests with HttpClient". In addition, we'll wrap the call with an Activity:

[Fact]
public async Task MiddlewareTest_ReturnsNotFoundForRequest()
{
    using var host = await new HostBuilder()
        .ConfigureWebHost(webBuilder =>
        {
            webBuilder
                .UseTestServer()
                .ConfigureServices(services =>
                {
                    services.AddMyServices();
                })
                .Configure(app =>
                {
                    app.UseMiddleware<MyMiddleware>();
                });
        })
        .StartAsync();

    var activity = new Activity("CallToBackend").SetIdFormat(ActivityIdFormat.W3C).Start();
    {
        var response = await host.GetTestServer().CreateClient().GetAsync("/");
    }
    finally
    {
        activity.Stop();
    }

    // ...
}

I'd expect that when the registered middleware code is reached, the "traceparent" header would be valued (since the Activity ID's format was set to the W3C standard above). However, when the test is run, the expected "traceparent" header does not exist in the request.

    public class CorrelationValidationMiddleware
    {
        // ...

        public async Task InvokeAsync(HttpContext context)
        {
            if (context.Request.Headers.TryGetValue("traceparent", out StringValues headerValue) == false)
            {
                context.Response.StatusCode = (int)HttpStatusCode.BadRequest;
                await context.Response.WriteAsync($"The 'traceparent' header is missing.").ConfigureAwait(false);
                return;
            }

            // ...

            // Call the next delegate/middleware in the pipeline
            await this.next(context).ConfigureAwait(false);
        }

        // ...
    }

If you were to instantiate a new System.Net.Http.HttpClient (i.e. HttpClient httpClient = new HttpClient()) in the unit test (not from TestServer.CreateClient()), the "traceparent" header would be present along with the generated value.

Just for your information, my team has developed a workaround so that our instantiated TestServer objects in our functional/integration tests produces an HttpClient that supports distributed tracing (just as if you were instantiating a new ``System.Net.Http.HttpClientobject outside theTestServer`) using an extension method:

    public static class TestServerExtensions
    {
        /// <summary>
        /// Creates an HTTP client.
        /// </summary>
        /// <remarks>
        /// <para>
        /// This method creates an <see cref="HttpClient" /> containing a <c>DiagnosticsHandler</c> so that the
        /// distributed tracing from an <see cref="Activity" /> can automatically be injected into the request
        /// header.
        /// </para>
        /// <para>
        /// This extension method is a workaround solution as the created <see cref="HttpClient" /> from the
        /// <see cref="TestServer.CreateClient" /> method does not inject distributed tracing into the request
        /// header from an instantiated <see cref="Activity" /> as expected.
        /// </para>
        /// </remarks>
        /// <param name="testServer">This <see cref="TestServer" />.</param>
        /// <returns>The HTTP client.</returns>
        public static HttpClient CreateClientWithDiagnosticsHandler(this TestServer testServer)
        {
            Assembly assembly = Assembly.Load("System.Net.Http");
            Type diagnosticsHandlerType = assembly.GetType("System.Net.Http.DiagnosticsHandler");

            HttpMessageHandler handler =
              (HttpMessageHandler)Activator.CreateInstance(diagnosticsHandlerType, testServer.CreateHandler());
            return new HttpClient(handler) { BaseAddress = testServer.BaseAddress };
        }
    }

Exceptions (if any)

Further technical details

  • ASP.NET Core version
  • Include the output of dotnet --info
  • The IDE (VS / VS Code/ VS4Mac) you're running on, and it's version
@danile-ms danile-ms changed the title CreateClient() produces an HttpClient that does not auto-inject distributed tracing when Activity is used TestServer.CreateClient() produces an HttpClient that does not auto-inject distributed tracing when Activity is used Aug 6, 2020
@Tratcher
Copy link
Member

Tratcher commented Aug 7, 2020

Activities aren't something I'd normally expect in tests, but I can see wanting tools for explicitly testing remote tracing.

This is similar to dotnet/runtime#31261, let's see what workaround they come up with.

@BrennanConroy BrennanConroy added this to the Backlog milestone Aug 7, 2020
@ghost
Copy link

ghost commented Aug 7, 2020

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@Tratcher Tratcher added affected-very-few This issue impacts very few customers bug This issue describes a behavior which is not expected - a bug. severity-nice-to-have This label is used by an internal tool labels Oct 6, 2020 — with ASP.NET Core Issue Ranking
@bronumski
Copy link

This would be handy for doing some integration testing.

@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 24, 2023
@manio143
Copy link

Looks like dotnet/runtime#31261 was completed and it would be nice to revisit this issue.

@joshmouch
Copy link

Yes, please revisit this. Why is DiagnosticsHandler not public?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affected-very-few This issue impacts very few customers area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions bug This issue describes a behavior which is not expected - a bug. severity-nice-to-have This label is used by an internal tool
Projects
None yet
Development

No branches or pull requests

8 participants