Fix HeaderCarrier changing header casing

[robertmarsal]

At the moment the HeaderCarrier will normalise HTTP headers and capitalise them. This way traceparent becomes Traceparent and is sent as an uppercase header. This is against the W3C recommendation as far as I understand as we should be sending it as lowercase.

TraceContext seems to have trouble consuming the header as uppercase but that seems like a separate issue.

In this PR I've used the underlying type of http.Header to ensure headers are kept and retrieved as it

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: robertmarsal / name: Robert Marsal (0cee8cdae6006605c33b130f40fcce7392d53ae4)

[robertmarsal]

Thank you for the thorough review @MrAlias. I've incorporated your suggestions and had a go at resolving the issue of consuming the headers in any case by lowercasing them when we set them as this is the format for both of the supported headers. I've also expanded the test cases to check for this. Can you please have another look when you get a chance?

Edit: please hold on the review as I discovered some issues with this approach.

[MrAlias]

Edit: please hold on the review as I discovered some issues with this approach.

Sounds good.

It's not required, but feel free to change this to a draft to help signal this. Otherwise I'll wait for the WIP prefix to change before I take another look.

Thanks for the diligence and the contribution :smile:

[robertmarsal]

I think I've actually hit a stumbling block with this at the moment. Because the underlying type of HeaderCarrier is http.Header users can build the HeaderCarrier using the native http.Header Set and that bypasses my attempts to avoid normalising like in this example from the tests:

req, _ := http.NewRequest("GET", "http://example.com", nil)
req.Header.Set("baggage", tt.header)

expected := tt.has.Baggage(t)
ctx := baggage.ContextWithBaggage(context.Background(), expected)
ctx = prop.Extract(ctx, propagation.HeaderCarrier(req.Header))

While I can fix this by checking for both the normalised and non normalised version when retrieving if the user does something like this:

req, _ := http.NewRequest("GET", "http://example.com", nil)
ctx := baggage.ContextWithBaggage(context.Background(), tt.mems.Baggage(t))
propagator.Inject(ctx, propagation.HeaderCarrier(req.Header))

got := strings.Split(req.Header.Get("baggage"), ",")

then the value will not be found because in the map it will be stored in lowercase.

It feels like this can't be accomplished with the current implementation and I don't think it's worth making big changes just for this. Unless there's something I am missing I think we can close this for now. Thank you

[robertmarsal]

@MrAlias to add more context to this, the way I discovered this was using a library that will receive the traceparent info from different sources like HTTP headers or other types of headers for example Kafka headers. This info was then converted to a text map and then extracted into a context. However this failed when the source was HTTP headers due to Go canonicalising the headers and TextMap being case-sensitive.

Basically the TextMapCarrier and HeaderCarrier implement the same interface however behave in different way and are not compatible with each other. Based on my research above there's no easy fix for this without changing the API but maybe we can improve the docs in this regard?

Thanks

[MrAlias]

Yeah, this sounds a bit tricky. My initial thought is to add an additional method to the HeaderCarrier, but I don't think this is necessarily the best approach. I'll have to think a bit about it.

@Aneurysm9 thoughts?