fix(http): remove outgoing headers normalization

[marcinjahn]

Which problem is this PR solving?

The http auto-instrumentation should not modify the request in any other way than context injection. Currently, the library does modify the casing of request headers. Even though in theory that should not matter, there are servers that require incoming headers to have some predefined casing, e.g. all letters uppercase.

Fixes #3556

Short description of the changes

Remove the code that "normalizes" the request headers.

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

To test this, send a request with headers that are not lowercase. The target server should receive them as they were sent.

Checklist:

• Followed the style guidelines of this project
• Documentation has been updated

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: marcinjahn / name: Marcin Jahn (4397853)

[codecov]

Codecov Report

Merging #3557 (92c5af9) into main (d1f9594) will decrease coverage by 0.01%.
The diff coverage is n/a.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3557      +/-   ##
==========================================
- Coverage   93.88%   93.88%   -0.01%     
==========================================
  Files         255      255              
  Lines        7757     7752       -5     
  Branches     1612     1611       -1     
==========================================
- Hits         7283     7278       -5     
  Misses        474      474              

Impacted Files	Coverage Δ
...es/opentelemetry-instrumentation-http/src/utils.ts	99.17% <ø> (-0.02%)	⬇️
...emetry-core/src/platform/node/RandomIdGenerator.ts	87.50% <0.00%> (-6.25%)	⬇️
...ges/opentelemetry-instrumentation-http/src/http.ts	94.21% <0.00%> (+0.32%)	⬆️

[dyladan]

Strongly agree. Why was this here to begin with?

[dyladan]

Looks like this was added by @legendecas in 21fc8b5#diff-2a4dbbc8d99791ab2180a7b9c036458fa20b0e8175d677b7cd28099ed2ebf100R294-R298

@legendecas is there a reason to keep this code or was it just an oversight?

[Flarna]

I assume it's used to ease request suppression in outgoing hook based on request headers. for incoming node http normalizes all headers to lowercase. For outgoing one gets what application uses so any sort of casing.

Another topic is maybe to ensure that we don't end up in having TraceParent form application next to traceparent added by instrumentation.

[marcinjahn]

I assume it's used to ease request suppression in outgoing hook based on request headers. for incoming node http normalizes all headers to lowercase. For outgoing one gets what application uses so any sort of casing.

Another topic is maybe to ensure that we don't end up in having TraceParent form application next to traceparent added by instrumentation.

The second point definitely should be addressed, probably in a different PR though. The question is, what do do in such a case? Should the library prioritize the traceparent supplied with the request, or should it prioritize the one that it'd normally inject?

Btw, the build started to fail in an unexpected place, I looked through the commits that came in since I opened this PR, and none of them should be the reason. I'm not sure why builds fail.

[Flarna]

Btw, the build started to fail in an unexpected place, I looked through the commits that came in since I opened this PR, and none of them should be the reason. I'm not sure why builds fail.

#3567 should fix this

[dyladan]

I assume it's used to ease request suppression in outgoing hook based on request headers. for incoming node http normalizes all headers to lowercase. For outgoing one gets what application uses so any sort of casing.

it's possible to normalize for the hook without modifying the request. @marcinjahn what would you think about updating the PR to do that?

Another topic is maybe to ensure that we don't end up in having TraceParent form application next to traceparent added by instrumentation.

Lots of layers to this. I think questions around this are most likely best solved in the spec or even in the w3c spec before implementations start tackling them inconsistently.

[marcinjahn]

it's possible to normalize for the hook without modifying the request. @marcinjahn what would you think about updating the PR to do that?

I could do that, only I am unsure if that's a good idea. If some client sends requests with non-lowercased headers, possibly there's some reason behind it, and the casing matters. Possibly, in the hook, the app would want to use these headers for something else (e.g., just logging). If lower-case headers are needed, it's not difficult for the app to lowercase them.

I say, let's not make such decisions for the users of the library. Doing the lower-casing on the library side is irreversible for the users, while leaving the headers in their original form gives them all the possibilities to do whatever they need to do.

Having said that, since you are the maintainers of the library, I'll add the lowercasing in the hook if you still think it's the right thing to do. Btw, do you mean the HttpInstrumentationConfig's requestHook? The config interfaca has quite a few hooks there :)

[marcinjahn]

@dyladan Is there anything more to do or can this PR be merged?

[dyladan]

If some client sends requests with non-lowercased headers, possibly there's some reason behind it, and the casing matters. Possibly, in the hook, the app would want to use these headers for something else (e.g., just logging). If lower-case headers are needed, it's not difficult for the app to lowercase them.

I say, let's not make such decisions for the users of the library. Doing the lower-casing on the library side is irreversible for the users, while leaving the headers in their original form gives them all the possibilities to do whatever they need to do.

Having said that, since you are the maintainers of the library, I'll add the lowercasing in the hook if you still think it's the right thing to do. Btw, do you mean the HttpInstrumentationConfig's requestHook? The config interfaca has quite a few hooks there :)

I was thinking about it mostly from a backwards compatibility perspective. There may be users depending on the lowercase behavior that we will break by undoing it. For example, they may be reading the 'authorization' key when their app is adding it as Authorization. Apps should not be differentiating between headers by case and should not be depending on casing in headers as they are specified to be case insensitive.

I guess I can go either way. I'm curious what the other @open-telemetry/javascript-maintainers think about this, particularly @legendecas but he is on vacation until the end of the month.

[marcinjahn]

@dyladan Right, I agree with your perspective. Completely removing lower-casing might be breaking for some (although still, it might be undesirable for new users). To keep it working for existing users, I'll add the hook normalization. Again, would it be the requestHook that you were thinking about?

[dyladan]

That's the one I was thinking of but I admit there may be others that I don't think of immediately.

[marcinjahn]

Alright, let's await for @legendecas's input.

[legendecas]

This was added to avoid header case-sensitivity duplications in the hooks. As defined in https://www.rfc-editor.org/rfc/rfc7230#section-3.2, HTTP header keys are case-insensitive. And Node.js already lowe-cased all incoming message's header keys so we don't have to perform anything particular. But this is not the case for the outgoing message since the header object can be an arbitrary object.

However, it is true that we should not prevent users from sending arbitrary case headers. I think the alternative here is using the original headers object to make the actual request and calling the hooks with the parsed one.

[Flarna]

Client response and server request hooks have lowercase headers because node normalizes headers on receive.
Client request and server response get the data from user.

[dyladan]

I think the alternative here is using the original headers object to make the actual request and calling the hooks with the parsed one.

That was my original recommendation as well

Client response and server request hooks have lowercase headers because node normalizes headers on receive.
Client request and server response get the data from user.

In the case of client response and server request hooks, we don't have to lowercase them because they're already lowercased.

In the case of client request and server response hooks, we should not modify the original request but it may be beneficial to provide normalized headers to the hooks. This satisfies backwards compatibility and also provides a consistent experience across all hooks.

---

Thinking about this a bit more though I am not so sure we should normalize the headers at all. The main drawback I can think of is that we may normalize in a different way than the runtime. If there is an outgoing request with MyHeader and myheader, one of these will "win" the normalization but the runtime may (probably will) treat them entirely differently. I don't think node does any modification of the outgoing headers, which means we may provide different headers to the hook than are actually included in the request.

[dyladan]

I actually think we should keep the PR the way it is now. See my above comment for reason

[marcinjahn]

Do you know why the node-windows unit tests are failing?

[legendecas]

Do you know why the node-windows unit tests are failing?

The test is not stable. Being tracked at #3572. I've restarted the run.