Closed rynowak closed 5 years ago
Marking for up for grabs. If you're interested in this please continue the discussion here first so we can work out details.
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.
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.
Ah, @martincostello beat me to the punch! Clearly we're both watching notifications from this repo!! :-)
I’m sure multiple heads thinking about it are better than one!
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.
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.
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
.
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.
@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
};
});
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.
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?
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.
@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 :).
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.
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.
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
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.
@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.
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)
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.
This will be included in preview 6. https://github.com/aspnet/AspNetCore/pull/9793
Thanks @alefranz for doing 99% of the work for this!
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.
@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.
@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)?
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