HttpClient automatically adds Request-Id HTTP header

[shravan2x]

I have an application that sends HTTP requests via HttpClient from an Asp.NET Core project. For unknown reasons, all sent requests seem to have a Request-Id HTTP header. I'm not able to repro this in a regular console application. The docs don't have any information on disabling it.

They seem to be added by the framework https://github.com/dotnet/runtime/search?q=RequestIdHeaderName&unscoped_q=RequestIdHeaderName .

• Is there a temporary fix for this issue?
• What is the right way to fix this issue in the long-term?
• Can we treat changes to the default HTTP headers sent as breaking changes? Issues like HttpRequestMessage does not respect TryAddWithoutValidation #35335 and this silently cause hard-to-debug issues.

[ghost]

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

[davidsh]

For unknown reasons, all sent requests seem to have a Request-Id HTTP header.

HttpClient API does not add "Request-Id" HTTP request header.

How did you confirm that HttpClient was adding this request header? Was it by inspecting request headers received by the server? It's quite possible that a front-end proxy etc. is adding these headers on your behalf.

[stephentoub]

HttpClient API does not add "Request-Id" HTTP request header.

Actually, DiagnosticHandler does:

runtime/src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHandler.cs

Line 271 in 4168844

request.Headers.TryAddWithoutValidation(DiagnosticsHandlerLoggingStrings.RequestIdHeaderName, currentActivity.Id);

but only if the diagnostics are enabled:

runtime/src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.cs

Line 220 in 4b03513

return DiagnosticsHandler.IsEnabled() ?

[davidfowl]

See this thread for clarity dotnet/aspnetcore#21012 (comment)

[davidfowl]

The docs are lacking in this area cc @noahfalk @shirhatti

[shravan2x]

@davidsh You can see how surprised I was when I realized the framework was doing this, and yet again just now when I heard that this is intentional behavior, and that there are no plans to fix it.

@davidfowl

That's fair, but we went with an opt-out model so that the "it just works when I call my other ASP.NET Core app or backend that supports the new W3C trace context" is on by default.

I strongly disagree with this. Not only is it sending behavior modifying headers without explicitly opting-in, it leaks private information about my HTTP client.

In what universe is it OK to post data that I didn't explicitly ask for to a remote location????

As much I hate to say it, I agree with @karlra's strongly worded views on this.

Even if it had been well documented, surely we don't expect every user of .NET to use a proxy, identify the weird header, google it, and then add code to opt-out? Why can't we ask users to add a line of code in their Startup.cs if they actually want such behavior?

I doubt the vast majority of Asp.NET Core users know or care about features like distributed tracing. Such a feature should have been designed to be opt-in. I'm confident you'll hear the same opinion if a public poll was conducted for this.

[davidsh]

@karelz @stephentoub

[davidfowl]

At this point if we decide to revert the behavior the question will be:

• What is the gesture to turn this on for an outbound calls?
• What about scenarios where you don't control how the HttpClient is constructed?

cc @SergeyKanzhelev @lmolkova

[shravan2x]

What is the gesture to turn this on for an outbound calls?

Following the usual pattern, something along the lines of services.AddDiagnostics(options => options.IsActivityPropagationEnabled = true); seems expected.

What about scenarios where you don't control how the HttpClient is constructed?

I'm unsure what such a scenario might be. However, I think having it be controlled by a flag like the example above is reasonable.

[davidfowl]

Following the usual pattern, something along the lines of services.AddDiagnostics(options => options.IsActivityPropagationEnabled = true); seems expected.

This would be HttpClientFactory specific? What would it be applied to? Which HttpClients?

I'm unsure what such a scenario might be. However, I think having it be controlled by a flag like the example above is reasonable.

A global static? Where do I set it, which HttpClients does it affect? Today the HttpClients that are affected are ones that run within the current activity. That's why it can be disabled on a granular basis.

[karlra]

I strongly disagree with this. Not only is it sending behavior modifying headers without explicitly opting-in, it leaks private information about my HTTP client.

I obviously also disagree strongly with this. Imagine the number of issues that will be raised on here about it.....The concatenation of the header that takes place if you try to set this header yourself is even worse.

In what universe is it OK to post data that I didn't explicitly ask for to a remote location????

As much I hate to say it, I agree with @karlra's strongly worded views on this.

Why, you hold a particular dislike for me? :)

[davidfowl]

The concatenation of the header that takes place if you try to set this header yourself is even worse.

I forgot about that. @karlr can you file another issue for that?

[karlra]

I included it in my original ticket, but do you want a completely separate ticket concerning just that one thing @davidfowl ?

[davidfowl]

Yes in this repository with an isolated repro.

[shravan2x]

A global static? Where do I set it, which HttpClients does it affect? Today the HttpClients that are affected are ones that run within the current activity. That's why it can be disabled on a granular basis.

@davidfowl From the doc you linked, we currently use code similar to the one below. OnIncomingRequest runs within the context of a web server, presumably in an Asp.NET Core middleware. When creating a new Activity instance, we could simply check the value of IsActivityPropagationEnabled and set it as a field on the Activity instance itself like activity.SetPropagationEnabled(options.IsActivityPropagationEnabled). We then check the value of activity.IsPropagationEnabled in the HttpClient instance before sending a request.

Keep in mind that HttpClient instances created in the context of an (incoming) web request might outlive the web request itself. So this should be a field that is checked before an (outgoing) HTTP request is sent, not when an HttpClient is created.

This way, we avoid creating a global static for this. Even if some user had multiple webservers running in the same program, each of their individual activity propagation preferences would be respected.

public void OnIncomingRequest(DiagnosticListener httpListener, HttpContext context)
{
    if (httpListener.IsEnabled("Http_In"))
    {
        Activity activity = new Activity("Http_In");

        //add tags, baggage, etc.
        activity.SetParentId(context.Request.headers["Request-id"])
        foreach (var pair in context.Request.Headers["Correlation-Context"])
        {
            var baggageItem = NameValueHeaderValue.Parse(pair);
            activity.AddBaggage(baggageItem.Key, baggageItem.Value);
        }     
        httpListener.StartActivity(activity, new  {context});
        try {
            //process request ...
        } finally {
            //stop activity
            httpListener.StopActivity(activity, new {context} );
        }       
    }
}

I think having 10% of users enable a feature that they actually want is much more preferable to having the other 90% disable a feature they don't.

@karlra

Why, you hold a particular dislike for me? :)

Hah not at all! I just meant that your opinion was rather strongly worded. But still, it was apt for the issue at hand - this is very unexpected behavior for a framework.

[karlra]

Yes in this repository with an isolated repro.

#35357

[shravan2x]

Just wanted to add that I originally identified this issue when debugging why a library wasn't working (so there are real-world issues that this raises). The extra Request-Id header was causing the webserver (non-ASP) to provide a different response than it otherwise would. Luckily in my case, the library was internal and I was able to update it to resolve the issue.

If possible, let's treat this issue as slightly higher priority since others may not be able to debug at the level that I or @karlra did. And since without debugging they wouldn't know about the Request-Id header, they wouldn't be able to find this issue or the suggested fix either.

[karelz]

Related to #31261

[karelz]

Triage:

• Does not seem to be in super-high demand
• There is workaround to use SocketsHttpHandler directly (maybe HttpClientFactory should expose a knob for convenience?)
• BCL API should be design with the opposite scenario (opt-in) in mind -- see Customized SocketsHttpHandler does not support diagnostics tracking #31261

[shravan2x]

@karelz Completely agree that any diagnostic feature like this should be opt-in.

Could you clarify what more info we need to proceed with improving this API?

[karelz]

I wasn't suggesting it should be opt-in. Given it is in place for few years and this is the first complain, I think opt-out is ok.

We should first wait for #31261 to be designed. Then design this API to be consistent / aligned.

[shravan2x]

@karelz

I wasn't suggesting it should be opt-in. Given it is in place for few years and this is the first complain, I think opt-out is ok.

Oh I misunderstood you. I believe that a big reason this is the first complaint is that very few people have noticed it so far. Even for those people, I strongly suspect they would consider this a privacy concern if they were explicitly asked if they accept this behavior.

[karlra]

I disagree strongly with making this opt-out for so many reasons, but if that is the decision, could you at least please change it so that if you set the request-id header explicitly it counts as opting out?

#35357

[shravan2x]

@karlra But then wouldn't the request-id header still be sent?

[karlra]

@shravan2x I just mean that if the decision is that .NET will by default continue setting the request-id header for tracking purposes and expecting the developer to opt out, then explicitly setting the value of the header must completely overwrite the header instead of how it works now.

[karelz]

@karlra that is discussed in the issue you linked, and our triage #35357 (comment) agreed with the behavior - let's keep the discussion there, separate from this one.

[kklldog]

I cant believe this is opt-in !!!! This waste 2 days for me , to find out why my request be ban . /cry

[MihaZupan]

While we are working on a proper solution for 6.0, if you don't care about distributed tracing at all, you can completely disable the DiagnosticsHandler for the process by setting an AppContext switch (may be useful for those using versions < 6 in the future).

AppContext.SetSwitch("System.Net.Http.EnableActivityPropagation", false);

[shravan2x]

@MihaZupan Does this work in .NET 5.0? Are there any side effects I should know about?

[MihaZupan]

Yes the switch has been there for quite a while.

It will completely disable the DiagnosticsHandler.
The effect is that the DiagnosticListener will never fire events from DiagnosticsHandler, no activity will be created inside http requests, and we won't touch any context headers (such as request id, tracestate, correlation context..).

All of these would only matter if you care about being a part of the distributed trace.

[shravan2x]

Glad to hear! I strongly doubt any average user would want to opt-in to an analytics/tracking system like this without fully understanding it (and they probably haven't even heard of it).

[MihaZupan]

I don't think having it be on by default was a bad choice since realistically it's helping users that care about it while not really affecting those who don't.
If you don't care about it you risk running into very strict services that disallow extra headers, but that seems to me like the service is being too aggressive.

It isn't leaking private information - these are just long random numbers for the most part.

[shravan2x]

If I understood the feature correctly, it leaks which other servers your program made a request to. This is especially concerning with large cloud solutions like AWS or Azure because they can tie different child requests together.

[MihaZupan]

#50658 added a global setting that can be used to disable context propagation process-wide.
With #55392, the HttpClient stack honours that setting:

DistributedContextPropagator.Current = DistributedContextPropagator.CreateNoOutputPropagator();

We are also exposing a property on SocketsHttpHandler (#55556) that can be used to override behavior for just that instance:

using var client = new HttpClient(new SocketsHttpHandler
{
    ActivityHeadersPropagator = null
});

For .NET 5 (and 3.1), see the workaround mentioned in #35337 (comment)