Repository navigation
addHeader removed in Undici request #3043
Description
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
Activity
ronag commentedon Apr 2, 2024
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 commentedon Apr 2, 2024
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 commentedon Apr 2, 2024
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 commentedon Apr 2, 2024
And you can't achieve that with a dispatcher? Can you provide me with a full example?
bizob2828 commentedon Apr 2, 2024
This propagation use case is a W3C standard. These were the docs I'm referring to as how to achieve this.
bizob2828 commentedon Apr 2, 2024
Here's our code to propagate headers
bizob2828 commentedon Apr 2, 2024
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 commentedon Apr 2, 2024
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 commentedon Apr 2, 2024
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 requestronag commentedon Apr 2, 2024
Please don't do that. That should/will not work either. Diagnostic channels should not mutate the request state.
ronag commentedon Apr 2, 2024
We are having a meetup in London this week and I'll bring your use case up for discussion.
bizob2828 commentedon Apr 2, 2024
@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 commentedon Apr 2, 2024
if that's the thinking, then it needs to be revisited. That's a major use case for observability vendors
ronag commentedon Apr 2, 2024
@mcollina wdyt? Revert the remove addHeader part and defer to a semver major?
mcollina commentedon Apr 2, 2024
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 commentedon Apr 2, 2024
@ronag the use case of APMs is to be able to do these kind of manipulations.
bizob2828 commentedon Apr 2, 2024
Thanks @mcollina 🙏