fix(instrumentation-fetch): handle HeadersInit tuple arrays in _addHeaders

[overbalance]

Reconstruction of #6194

Which problem is this PR solving?

When HeadersInit is passed as an array of tuples:

fetch('/api', { headers: [['Content-Type', 'application/json']] })

The _addHeaders method corrupts headers into {"0": "Content-Type", "1": "application/json"} because Object.assign() treats arrays as objects with numeric keys.

Short description of the changes

• Use Headers constructor internally to correctly parse all HeadersInit types (undefined, Headers, Record, and tuple arrays)
• Convert back to plain object to maintain backward compatibility with requestHook users

Type of change

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

How Has This Been Tested?

• Added unit tests for tuple array headers (with and without propagator)
• All existing tests pass

Checklist:

• Followed the style guidelines of this project
• Unit tests have been added

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.50%. Comparing base (fcafab5) to head (c186b9d).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6341      +/-   ##
==========================================
- Coverage   95.50%   95.50%   -0.01%     
==========================================
  Files         365      365              
  Lines       11609    11604       -5     
  Branches     2677     2673       -4     
==========================================
- Hits        11087    11082       -5     
  Misses        522      522              

Files with missing lines	Coverage Δ
...s/opentelemetry-instrumentation-fetch/src/fetch.ts	90.03% <100.00%> (-0.19%)	⬇️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[trentm]

fetch('/api', { headers: [['Content-Type', 'application/json']] })

Is that an allowed usage for headers? https://developer.mozilla.org/en-US/docs/Web/API/RequestInit#headers suggests it is not legal.

[trentm]

From trying to parse the fetch spec (e.g. https://fetch.spec.whatwg.org/#typedefdef-headersinit) my head is spinning and I don't know what is meant to be legal.

[trentm]

undici's fetch() appears to allow it -- from reading its types.

[trentm]

And using fetch() in the Firefox dev console on www.google.com seems happy with this form too:

[overbalance]

@trentm The tuple is bizarre but it works. I've never seen it in the wild though 🧐

[david-luna]

this reminds me of open-telemetry/opentelemetry-js-contrib#2875

[david-luna]

There is no reference on the spec of what should take precedence. See the spec issue that was motivated by the same situation in undici instrumentation.

I do not have a preference in which one takes precedence because each option has a use case. At least we should be consistent and do not override the headers in the if block above.

[trentm]

Crazy how complex this gets. Sorry for the wordiness, I'm thinking out loud.

• The instr was already inconsistent in the precedence it was giving. The existing options instanceof Request and options.headers instanceof Headers and options.headers instanceof Map branches were already giving precedence to the new propagation.injected headers. The final else block was givin precedence to the user-provided headers.
• The fetch spec (or at least from reading MDN) saw that Request.heaaders is read-only, so the first if (options instanceof Request) block has to behave as it currently is (precedence to the injected headers). (Well, this could all be refactored to completely replace the whole Request, but that's probably crazy and has its own issues.) So, if consistency in the precedence is a concern, then I think we should have all the inject'ing blocks just add to the existing headers (precedence to the injected headers).
• In all cases, except options instanceof Request where options.headers property is read-only, it might be nice to avoid mutating the given headers object. Instrumentation-http avoids mutating the headers, because of AWS S3 behavior changes when using the auto instrumentation opentelemetry-js-contrib#1609 (comment) For this reason, I like the idea of creating a new Headers object.
• (It might be nice to avoid mutating the whole options object, in case user code is re-using it, but I think that would be for a separate issue/PR.)
• Why a Headers object instead of not just an object? Because it is native to fetch() and I assume will do the best job of compatibly handing whatever weird thing is passed as options.headers.

Doing ^^ gives a change that is almost exactly #6194

Then there is whether to "break" requestHook, because now request.headers is a Headers instance where it was typically (but not always) an Object instance.

• Returning a Headers instance still fits the FetchRequestHookFunction type, FWIW.
• I noticed an interesting thing in instrumentation-undici: It explicitly runs the requestHook before injection, so "no hook can tamper" with the injected headers. https://github.com/open-telemetry/opentelemetry-js-contrib/blob/97d9ef01008d47df6e2f56eb06908cda23ba6fe6/packages/instrumentation-undici/src/undici.ts#L329-L340

Options:

1. Break requestHook usage that was assuming request.headers would be an Object -- typically because it was an object on the way in, so somewhat reasonable assumption.
2. Run requestHook before injection so workaround the issue.
3. Do the extra work of converting back to an Object. Note that this will also possibly break a user that assumed options.headers would be a Headers if it passed a Headers in, or a Map if it passed a Map in.

I'm leaning towards option 2, because that seems a weaselly way out. :)

[overbalance]

I think it should match undici behavior by calling requestHook before propagation injection. I implemented option 2 and kept the Headers type rather than converting to a plain object since it's a valid HeadersInit type. Thoughts?