addHeader removed in Undici request

[bizob2828]

Bug Description

Looks like addHeader was removed in https://github.com/nodejs/undici/pull/3024/files. The New Relic Node.js agent relies on this to propagate headers in our instrumentation.

Reproducible By

const requestHooks = diagnosticsChannel.channel('undici:request:create')
requestHook.subscribe(function requestCreate({ request }) {
  request.addHeader('test-header', 'value')
})

Expected Behavior

Adds test-header with value of value

Logs & Screenshots

TypeError: request.addHeader is not a function
    at addDTHeaders (undici.js:104:13)
    at requestCreateHook (undici.js:162:5)
    at Channel.publish (node:diagnostics_channel:142:9)
    at new Request (request.js:192:23)
    at Client.[dispatch] (client.js:304:21)
    at Intercept (redirect-interceptor.js:11:16)
    at Client.[Intercepted Dispatch] (dispatcher-base.js:158:12)
    at Client.dispatch (dispatcher-base.js:179:40)
    at Pool.[dispatch] (pool-base.js:143:28)

Environment

Any, undici 6.11.0

[ronag]

That method is unsafe and pretty much undefined in terms of behavior. A request object should not be mutated once queued.

What are you trying to achieve? Maybe we can find a better way to deal with it.

[bizob2828]

Our agent needs to propagate some headers before making an outbound request. This was a documented use case and removed in a minor release

[ronag]

This was a documented use case and removed in a minor release

Diagnostics channels are experimental. Hence any breaking change to them is not qualitifed as semver major. I'm not opposed to deferring it as such but it was in this case decided that it's not necessary.

What was/is it documented? We might need to fix that as well.

[ronag]

Our agent needs to propagate some headers before making an outbound request.

And you can't achieve that with a dispatcher? Can you provide me with a full example?

[bizob2828]

This propagation use case is a W3C standard. These were the docs I'm referring to as how to achieve this.

[bizob2828]

Here's our code to propagate headers

[bizob2828]

And you can't achieve that with a dispatcher?

Maybe, I'm just not that familiar with the API. At the time of our instrumentation we just followed the docs that were removed in a semver minor. Experimental or not, this was quite abrupt

[bizob2828]

Looking at dispatchers, it's not the appropriate use case as we're an agent. The diagnostics channel seems more apropos for this use case

[bizob2828]

I can fix our instrumentation to just do request.headers.push(<key>, <value>) but this appears to only work in newer versions so i'll have to fork the logic to handle different versions of undici request

[ronag]

I can fix our instrumentation to just do request.headers.push(<key>, <value>) but this appears to only work in newer versions so i'll have to fork the logic to handle different versions of undici request

Please don't do that. That should/will not work either. Diagnostic channels should not mutate the request state.

[ronag]

We are having a meetup in London this week and I'll bring your use case up for discussion.

[bizob2828]

@ronag why don't you suggest pushing headers, it works just fine and looking at the request code, that's what is happening? we also cannot wait for a meeting next week. We have 100s of customers relying on this instrumentation.

[bizob2828]

Diagnostic channels should not mutate the request state.

if that's the thinking, then it needs to be revisited. That's a major use case for observability vendors

[ronag]

@mcollina wdyt? Revert the remove addHeader part and defer to a semver major?

[mcollina]

Let's revert this. I guess we should not have been done that to begin with and modifying headers is a valid use case.

[mcollina]

@ronag the use case of APMs is to be able to do these kind of manipulations.

[bizob2828]

Thanks @mcollina 🙏