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

Add a header propagation middleware and message handler #526

Closed
rynowak opened this issue Oct 12, 2018 · 25 comments
Closed

Add a header propagation middleware and message handler #526

rynowak opened this issue Oct 12, 2018 · 25 comments
Assignees
Labels
help wanted Up for grabs. We would accept a PR to help resolve this issue

Comments

@rynowak
Copy link
Member

rynowak commented Oct 12, 2018

We should consider providing an in-the-box solution for propagating headers from the current request to an outgoing http request.

This would consist of a middleware that can be configured to stamp certain headers on a feature of the current request. Then a message handler uses IHttpContextAccessor to grab the feature and apply the headers to outgoing requests.

Maybe similar to:
https://github.com/stevejgordon/CorrelationId/tree/dev/src/CorrelationId from @stevejgordon or
(sample @davidfowl made)

Also consider using Activity

@rynowak
Copy link
Member Author

rynowak commented Oct 12, 2018

Marking for up for grabs. If you're interested in this please continue the discussion here first so we can work out details.

@martincostello
Copy link
Member

I’d be potentially interested in helping out with this. We currently have some in-house bespoke handlers to do header forwarding, so a configurable, built-in way to do the same thing would be great.

@stevejgordon
Copy link

Hi @rynowak - Depending on timescales I'd quite like to throw in on this one since you may be after similar to my library. I've been meaning to dig into the Activity bits at some point too. I've just recently added a handler within an internal project that applies the correlation ID using my library which works quite nicely.

@stevejgordon
Copy link

Ah, @martincostello beat me to the punch! Clearly we're both watching notifications from this repo!! :-)

@martincostello
Copy link
Member

I’m sure multiple heads thinking about it are better than one!

@rynowak
Copy link
Member Author

rynowak commented Oct 12, 2018

cool - I'd definitely look forwards to some contributions from you both. I think I'd also like to get some input from @davidfowl and @glennc and use this issue to discuss design proposals as well as scenarios.

@davidfowl
Copy link
Member

Here's the sample I wrote https://gist.github.com/davidfowl/c34633f1ddc519f030a1c0c5abe8e867. This is just generic header propagation not tied to any specific implementation of activities or id generation etc.

@martincostello
Copy link
Member

Would simple transformations be on the table for this feature as well? We do a few things internally where we map one header to another name when we pass it on.

An example would be something like User-Agent being passed on
as X-Forwarded-User-Agent.

@stevejgordon
Copy link

I was thinking the exact same thing as @martincostello. It's something we recently needed to do and a few people asked about adding that to the CorrelationId library too. Also, some people ask about being able to control this per endpoint. In that respect, something that can be added per logical client on the HttpClientBuilder would be useful too.

@davidfowl
Copy link
Member

@stevejgordon

Also, some people ask about being able to control this per endpoint. In that respect, something that can be added per logical client on the HttpClientBuilder would be useful too.

See https://gist.github.com/davidfowl/c34633f1ddc519f030a1c0c5abe8e867#file-headerpropagation-cs-L24

Would simple transformations be on the table for this feature as well? We do a few things internally where we map one header to another name when we pass it on.

As for the mapping I'd like to see a couple of real examples. I believe there's a need there but I don't know the extent of the mapping capabilities required. Is it a simple Func<IncomingHeader, OutgoingHeader>? Or do you need more context? Maybe there's an "I want to take over" mode where you get to write the headers yourself:

services.AddHeaderPropagation(options =>
{
        options.Mapping = (httpContext, httpRequestMessage) =>
        {
                // Write code to propagate headers here
        };
});

@martincostello
Copy link
Member

I like this idea of it sort of being "layered" so you can go low-level and custom if you really want to.

Something super-simple could then be powered off that with a helper overload, something kinda like:

public static IServiceProvider AddHeaderPropagation(this IServiceProvider services, NameValueCollection mapping)
{
    return services.AddHeaderPropagation(options =>
    {
        options.Mapping = (context, request) =>
        {
            foreach (string key in mapping.Keys)
            {
                var headerValue = context.Request.Headers[key];

                if (StringValues.IsNullOrEmpty(headerValue))
                {
                    continue;
                }

                request.Headers.TryAddWithoutValidation(mapping[key], (string[])headerValue);
            }
        };
    });
}

Then in the simple case, a user can do something like the below:

var mapping = new NameValueCollection()
{
    ["Referer"] = "Referer",
    ["User-Agent"] = "X-Forwarded-User-Agent",
    ["X-Correlation-Id"] = "X-Correlation-Id",
};

services.AddHeaderPropagation(mapping);

But for a more advanced implementation, the user can just bring their own implementation and be advanced as they like using the Action<HttpContext, HttpRequestMessage> delegate.

@glennc
Copy link

glennc commented Oct 17, 2018

I want to get some real examples of what people use the mapping code for, is User-Agent to X-Forward-User-Agent a real-world thing? Are there others the people your talking to are explicitly wanting to do?

@martincostello
Copy link
Member

So internally we map headers so that we can trace things in our Kibana logs so we can see where traffic originated as it goes through the layers of our application, such as from our DDoS edge, to HAProxy, to our web tier, down to our microservices/APIs.

In one case we have a header called x-je-conversation which we always pass through if present that is sort of a end-to-end correlation ID, which would be covered by the simple propagation case.

In another case we map User-Agent to x-je-user-agent so that an application can see who called it via User-Agent and who it called it on behalf of from x-je-user-agent. This would be covered by the "mapping" scenario.

We also have a third case where we pass-through a header if present, but generate a new value if not to forward on. This would be something more advanced that would either still need a custom handler today, or could use something like the advanced delegate-based mapper.

@glennc
Copy link

glennc commented Oct 23, 2018

@martincostello the first two of those are variations of distributed tracing. No? If by default we forwarded a correlation id header would that satisfy most of your requirements for the tracing/identifiers?

The distributed tracing spec that I am thinking of us supporting by default also has a concept like baggage from Zipkin, which is an arbitrary mapping of keys to values that will flow as well as the correlationId.

If you had those two features would that satisfy all the cases you know of? I have a suspicion that a distributed tracing system is what everyone is using this for and am looking for exceptions to that or limitations that would make it not work :).

@glennc
Copy link

glennc commented Oct 23, 2018

I'm not arguing that we don't do this feature. But if the majority of customer cases are satisfied by correlationIds and perhaps some baggage then we should possibly focus our efforts there first.

For example, a scenario that would need mapping outside of the tracing system is testing in production where you flow a header that informs some load balancer to flow your requests to a specific test instance of a microservice.

@martincostello
Copy link
Member

Yep, it's definitely similar to/is-kinda distributed tracing, it's just something custom and internal that's been around for years and years in our many applications that everything has to follow by convention.

At the moment for our .NET Core stuff we're doing this semi-manually with a hand-coded delegating handler we distribute via ProGet, but something built-in we could just configure would be nicer, however that manifests itself.

I think the key "want" for that scenario for us is being able to map one header already present to another name, rather than only a simple as-is propagation.

@alefranz
Copy link

I'd like to help moving this forward. Would be creating a PR the best approach?
I plan to use @davidfowl gist with minor tweaks and a small addition.

Thanks

@Symbianx
Copy link

Symbianx commented Feb 11, 2019

For people who step in this issue looking for a way to easily propagate the ISTIO tracing headers like I did, we created a library just for that, you can find it here: https://github.com/nosinovacao/istio-tracing-aspnetcore .

@alefranz it also may be of help for your PR.

@alefranz
Copy link

@Symbianx I was actually waiting for a reply to submit a PR but I had already hacked some code. I've now submitted it, IT would be great if you can have a look and let me know if this cover your needs. I will try to have a look at that library tomorrow.

@acds
Copy link

acds commented Mar 7, 2019

I’ve been following this as a micro service developer, and really appreciate this is being looked at.

My main use case is in terms of tracing and tracking but do also see value in propagating other headsets including any authentication tokens for end-to-end authentication between micros services.

It would also be useful to hook any header propagation middleware such that one can leverage the hook to push headers into properties in ones logging context in a central place (like a SeriLog enricher)

@antoinebj
Copy link

Is #1099 really viable with multithreading?

I'm currently using v2.2 with the need to propagate a header. However, I'm stuck with the old ASP.NET Web API 2, so instead of IHttpContextAccessor, I have to make do with HttpContext.Current. Obviously I ran into trouble the moment the HttpClient was being used from another thread (therefore with no associated HttpContext), because the delegating handler's SendAsync() was also running on that thread.
So I was curious to look at the implementation done here to see if I could get a solution that I could backport.

I read the IHttpContextAccessor is not thread safe, I'm assuming it has the same limitations whereby you cannot get the HttpContext if the code is not running on the request's thread.
If that is the case, I don't see where you avoid the same problem that I ran into.

Furthermore, the pooling of message handlers is great, but I'm wondering if the design isn't fundamentally flawed. Apparently it does not allow to get scoped information into the handler instance, and that creates all sorts of frustrations.
Honestly, the workaround described here is very unpleasant and kind of feels like the HttpClientFactory library becomes pointless. I was planning to write custom AddTypedClient extension methods to replace the original ones, but the latter make use of a private ReserveClient method, which I'm not able to call. I don't want to pull a thread that leads me to rewriting a significant part of the library.

@rynowak
Copy link
Member Author

rynowak commented May 7, 2019

This will be included in preview 6. dotnet/aspnetcore#9793

Thanks @alefranz for doing 99% of the work for this!

@rynowak rynowak closed this as completed May 7, 2019
@rynowak rynowak modified the milestones: Backlog, 3.0.0-preview6 May 7, 2019
@rynowak rynowak added 3 - Done and removed 1 - Ready labels May 7, 2019
@alefranz
Copy link

alefranz commented May 7, 2019

I've aslo published an unofficial backport to AspNetCore 2.1 and 2.2.
https://www.nuget.org/packages/HeaderPropagation/

I'll update it now with the latest changes.

@antoinebj
Copy link

@alefranz Thanks a lot. I understand now, the key to getting this to work across threads and through the asynchronous execution flow is to use AsyncLocal.

@acds
Copy link

acds commented May 10, 2019

@alefranz Nice!

Is there a standardized compatible with this library, to support a way to pick the current header values up, such that they could be injected into say the Serilog context via:

LogContext.PushProperty($"{headerName}", headerValue)

with each scope/request being processed such that any logging pick up those header values (where headerName is known - and defined in the services.AddHeaderPropagation config)?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
help wanted Up for grabs. We would accept a PR to help resolve this issue
Projects
None yet
Development

No branches or pull requests