Header propagation sample and UX improvements

[rynowak]

Creating a sample for the header propagation package and some various
misc UX improvements based on building the sample.

A small list:

• allow duplicate inbound names
• de-dupe based on outbound names
• add sugar for configuration
• simplify pattern for transforming values
• add error message for missing middleware

Also a few small perf things.

I started this out by wanting to remove the following from the
configuration pattern:

options.Headers.Add("X-TraceId", null);

This pattern with null is undiscoverable, but we didn't provide
something simpler. The most common case was to add a custom collection
type so we can define sugar methods.

The next realization is that in practical case (dist tracing sample) you
either way to key off of the same inbound header twice, or you don't
have an inbound header at all, and you will synthesize the value every
time. This means that the way we're treating inbound header names is a
bit wrong. We don't want inbound header names to be unique, we want
outbound header names to be unique.

Next, I want to consolidate DefaultValue and ValueFactory. The problems
I saw with this:

• DefaultValue is a trap. It's rare to use a static value.
• ValueFactory really wants the header name and value

I think what's there now is much more terse to work with.

/cc @Tratcher @davidfowl @alefranz

[alefranz]

Nice Ryan! Much easier to use it now!

[alefranz]

I see you have now switched to store the header with the outbound header name instead of the inbound one.
If you remember I was torn on this in the original PR and I probably switch a couple times.

The reason why I settled on using the inbound name was to play better with the ability to specify the headers per client which we discussed to add later (but unfortunately I haven't had time to work on it yet but I would like to discuss it later so I can work on it).

So the idea is that given an header I receive (e.g. Accept-Language, I'm intentionally not going to use tracing as example :) ) I want to propagate it to my dependencies which unfortunately have different conventions (one wants X-Language, the other Lang) so the output name will be overridable and being able to then configure the clients in terms of the original name accepted by my API/website (as well as being able to find it in the HeaderPropagationValues for logging as mentioned in the other comment) seems the best option.

What do you think? what was your thought process that led to use the outbound name as key?

Thank you

[rynowak]

So the idea is that given an header I receive (e.g. Accept-Language, I'm intentionally not going to use tracing as example :) ) I want to propagate it to my dependencies which unfortunately have different conventions (one wants X-Language, the other Lang) so the output name will be overridable and being able to then configure the clients in terms of the original name accepted by my API/website (as well as being able to find it in the HeaderPropagationValues for logging as mentioned in the other comment) seems the best option.

I'm glad that you're bringing this up because I wanted to get your opinion as well. I'll come back to your example because I think my changes open up an easy path to what you're looking for.

I think storing the header values by their inbound names raises a few questions that are hard to answer, or have unsatisfying answers:

• What 'inbound name' do I use for a header that is always generated? 🤷‍♂
• Can I map the same inbound header to two outbound names? No 😢
• Can I map two different headers to the same outbound name? Yes, but the behaviour is undefined 😢

I started contemplating this example while writing the sample here and these problems bit me. I have both a generated header X-SpanId and a header that's mapped to a different outbound name X-SpanId -> X-ParentSpanId.

Since I'm mapping X-SpanId to X-ParentSpanId then that mapping needs to use X-SpanId as its inbound name.

So then what inbound name do I associate with my generated X-SpanId. Well it could be anything but one of the other header names. I originally started with Add("__ignoreme", "X-SpanId", ....);

This just made me feel sad and like the concepts aren't working for us. Since we're responsible for documentation and guidance, it never feels good to tell a user "make up a name, it doesn't matter" - believe it or not that really confuses and upsets people.

So I went back to requirements. What are the actual constraints of the system? The actual constraint is that outbound headers must not contain duplicates when used with a single HttpClient.

So what if we lean into that?? From that I derived what's in the PR, and added the (quite elegant IMO) ability to add multiple mappings that run in order. This is a nice little convenience so that you can avoid writing complicated code in a lambda.

Another option that I considered is to associate a non-semantic "logical name" with each mapping. Then you have no restrictions other than no-duplicated-logical-names. I don't want to do this because it really increases the complexity, and I don't think there's a need to go that far.

---

Let's go back to your example.

Does it make sense to forward Accept-Language as Accept-Language sometimes and X-Lang sometimes? Sure it might for your scenario - and it's not blocked by this change.

A different question? Does it make sense to map two different headers to the same outbound name inside a single request? I don't think you would do that - because then you're not really propagating headers - you're doing something else. This is the use case that is blocked by keying on the outbound header name.

I think there's a simple solution to what you proposed here, which is to pass a filter predicate to each message handler with an allow-list of filters to include. So your code would look like:

services.AddHeaderPropagation(options =>
{
    options.Headers.Add("Accept-Language");
    options.Headers.Add("Accept-Language", "X-Lang");
});

services.AddHttpClient("a").AddHeaderPropagation(h => h == "Accept-Language");
services.AddHttpClient("b").AddHeaderPropagation(h => h == "X-Lang");

Hopefully this all makes sense. I'm fairly convinced this is the right thing to do, but still interested in your thoughts.

[alefranz]

tl;dr - scroll down to the conclusion

Hi Ryan,
I appreciate your interest in my opinion.

Let me clarify from the beginning that I am not arguing this approach is incorrect, as usual there are compromises to make and probably it is the best compromise. I just want to make sure that some scenarios, which in my experience are pretty common, are not overlooked.
These are not ideal scenario to have to deal with but many times APIs/websites are aggregating multiple APIs that are out of your control (or they are but are expensive to change) and all use different conventions to gather the same input.

• What 'inbound name' do I use for a header that is always generated? 🤷‍♂
  agree it will be not ideal to have to make up a name, but I believe if's ok-ish if we consider it a logical name. The problem is that even if using the output header name, if this header need to be passed to multiple APIs with different names, you would still need the ability to rename it and using one of these names as key as pros and cons comparing to use a logical name.
  e.g. (in made up new syntax, just to be self explanatory)

services.AddHeaderPropagation(o =>o.Headers.Capture("X-SpanId")
                                           .OrUseIfMissing( x=>Guid.NewGuid()));

services.AddHttpClient("a").AddHeaderPropagation(o => o.Propagate("X-SpanId"));
services.AddHttpClient("b").AddHeaderPropagation(o => o.Propagate("X-SpanId")
                                                       .Named("TransactionId"));

• Can I map the same inbound header to two outbound names? No 😢
  I don't get this one. To me it feels similar to the above: you capture inbound header, optionally specify a default outbound header, and later when you use it you can override it.

services.AddHeaderPropagation(o =>o.Headers.Capture("Accept-Language")
                                           .Named("X-Lang"));

services.AddHttpClient("a").AddHeaderPropagation(o => o.Propagate("Lang")
                                                       .Named("Language")); // Here I override the name
services.AddHttpClient("b").AddHeaderPropagation(o => o.Propagate("X-Lang"));
services.AddHttpClient("c").AddHeaderPropagation(o => o.Propagate("X-Lang")));

Probably having the ability to specify a default outbound header name can add complexity as you don't really see it when you setup the client, so your approach would be more obvious (as the identifier of the header is what will be used as name if I don't override it). It has the drawback that when all that when all the dependencies have a different names you either have to pick one or make up a logical name and override in all. I think I'm happy with this

services.AddHeaderPropagation(o =>o.Headers.Capture("Accept-Language").Named("Lang"));
// outbound name is Lang but I always override it so it is used only as logical name

services.AddHttpClient("a").AddHeaderPropagation(o => o.Propagate("Lang")
                                            .Named("Language"));
services.AddHttpClient("b").AddHeaderPropagation(o => o.Propagate("Lang")
                                            .Named("X-Lang"));

• Can I map two different headers to the same outbound name? Yes, but the behaviour is undefined 😢

Do you mean the value can be passed in either one or the other header and you pick one?
e.g. your API can receive the language either in Accept-Language or on a deprecated X-Lang header?
Yes this is awkward if you use the inbound header name.
I think this scenario is quite edge case as you later mention:

Does it make sense to map two different headers to the same outbound name inside a single request? I don't think you would do that - because then you're not really propagating headers - you're doing something else. This is the use case that is blocked by keying on the outbound header name.

So I guess you meant something different in one place or the other?

Also because to me if feels if using the outbound header name it actually works

services.AddHeaderPropagation(o =>o.Headers
                                   .CaptureFromFactory(/* Take from context Accept-Language or, if missing, take X-Lang*/)
                                   .Named("Lang"));  // it will be stored as Lang and passed to clients as Lang

---

Conclusion

In conclusion what really matter, in my option, is the ability to specify the headers you want to pass per client, not only as a filter but with the ability to specify a different name.
I don't think using the inbound name block any functionality, but this updated approach looks more obvious when you set it up per client (as the name you see it in your filter is the name that by default will be used ad outbound name in the client).
I think we should actually see it as a logical name used to store the header, which by default it is used as outbound name unless it is overridden in the specific client.

---

If it's ok with you, after this get merged, I can submit a proposal (or PR) to configure the clients. I haven't figured out a way to specify the configuration of each HttpClient in config, but I guess we can start with configuration in code only.

Someone will then argue: what if one of my dependencies needs that value propagated as parameter in query string? I guess this would be a different feature proposal that risk going too far out of scope to a more generic "parameters propagation thingy™️"

Sorry for the long reply, here's a 🥔 .

Thank you,
Alessio

[rynowak]

If it's ok with you, after this get merged, I can submit a proposal (or PR) to configure the clients. I haven't figured out a way to specify the configuration of each HttpClient in config, but I guess we can start with configuration in code only.

Sure, I'd love to see more from you on this. I think what's in the PR is a positive change, and I'm open to more design changes up to the 3.0 release if we're adding more features.

[alefranz]

Thanks. I'll try to open a PR later today.

[rynowak]

Updated this

[Tratcher]

Ref assemblies