fetch: design of wrapper makes instrumenting miss the first request

[bizob2828]

What is the problem this feature will solve?

fetch shipped in 18.x which is a wrapper around undici. I work on the New Relic Node.js agent and we noticed an issue where we weren't properly propagating state through a fetch command. When I tried registering our undici instrumentation, I noticed the first request is missed.

This is due to the lazy loading of the internal undici. Also since this is an internal module load I couldn't rely on our CJS instrumentation to key off of a require of a file path.

const diagnostics = require('diagnostics_channel')
const originalFetch = global.fetch

global.fetch = function wrappedFetch() {
  const fetchRequest = originalFetch.apply(this, arguments)
  // add your code now that undici has been loaded and can subscribe to the undici:* events.  
  diagnosticsChannel.channel('undici:request:create').subscribe(({ request }) => {
    // do stuff with the undici request
  })
  return fetchRequest
}

What is the feature you are proposing to solve the problem?

Change how undici is getting loaded as an internal to make it possible to properly register channels before the first fetch is ever called. Also as aside, I may be wrong but making fetch async is creating an unnecessary promise right?

  async function fetch(input, init = undefined) {
    emitExperimentalWarning('The Fetch API');
    return lazyUndici().fetch(input, init);
  }

What alternatives have you considered?

The above code snippet feels like the only option which will miss the first request as the subscriptions are not present. I guess an alternative would be users to preemptively subscribe to channels but I get how that could be problematic and lead to hijacking of name spaces.

[bizob2828]

@targos I see you added the lazy loading in 4944ad0b9ed. Any thoughts on making this more malleable for instrumentation vendors? Ideally it'd be nice to always load at runtime so we can properly subscribe to the undici channels before any fetch requests have been made

[targos]

I'm sorry, I don't understand the problem enough to suggest a solution. Why can't you subscribe to the diagnostics channels before undici is loaded?

[bizob2828]

I'm sorry, I don't understand the problem enough to suggest a solution. Why can't you subscribe to the diagnostics channels before undici is loaded?

@targos I'm not 100% certain but maybe because undici is getting required through the internal loader and the WeakReference is lost? If I create the subscriptions before undici is loaded I see the channels in the diagnostics channel but the ref is undefined when undici calls diagnostics_channel.channel so all previous subscriptions are missing. It seems like you cannot preemptively subscribe to channels until they are defined in certain cases. I even provided a small repro below and if you run with debugger you can see the channel reference is undefined so it creates a new one.

// channel.js:
'use strict'
const diagnosticChannel = require('diagnostics_channel')
diagnosticChannel.channel('diag-test').subscribe((name) => {
  console.log(`Hello ${name}`)
})

// main.js
'use strict'
require('./channel')
const diagnosticsChannel = require('diagnostics_channel')
function doStuff() {
  console.log('doing stuff')
}

debugger
const channel = diagnosticsChannel.channel('diag-test')
if (channel.hasSubscribers) {
  channel.publish('Bob')
}

[bizob2828]

With all this being said, this appears to be a registration order of instrumentation within our agent. I've yet to find root cause but I'm going to close this for now.

[targos]

I'd still like to bring this to the attention of @nodejs/diagnostics

[Qard]

Use diagnostics_channel.subscribe(name, handler). The channel.subscribe(handler) API is deprecated.

If you really must use the channel object subscriber, you should make sure the channel object reference is held somewhere as it will otherwise get garbage collected immediately after use due to being a weak reference within diagnostics_channel.

[bizob2828]

Thanks for your feedback @Qard I will update our instrumentation and save references to ensure GC doesn't clean up.

[bizob2828]

@Qard which version of Node.js is channel.subscribe(handler) deprecated? I'm in 18.x and diagnostics_channel.subscribe(name, handler) isn't even available. I'm also not seeing any of what you're describing in any public docs.

[bizob2828]

Also FWIW I did this and it still didn't work

If you really must use the channel object subscriber, you should make sure the channel object reference is held somewhere as it will otherwise get garbage collected immediately after use due to being a weak reference within diagnostics_channel.

I basically set something up like this

const channels = {
  requestCreate: diagnostics_channel.channel('undici:request:create')
}

channels.requestCreate.subscribe(() => // handler code )

But when undici is internally required and when it creates the channels, the weak references are gone for my subscription handlers. As I mentioned here, something in our bootstrap process is affecting this. I was hoping doing what you suggested by saving references to channels would help but it appears it did not

[targos]

The new API isn't available in a release yet. It was added in #42714

[Qard]

Ah, yeah, it was merged awhile ago now, but I guess no new releases with it yet.

As for the channel getting collected, the way the original API was supposed to be used was by declaring the channels immediately after the requires at the top of the file and storing them somewhere that can be reached by wherever the publish or subscribe calls will happen. This turned out to be a bit problematic because subscribers sometimes also would happen at top-level, which resulted in people chaining the subscribes which would let the whole thing fall out-of-scope immediately. This is why the new API was introduced which explicitly marks the link as strong.

[bizob2828]

I'm revisiting this and I think it's the same reason detailed out in #42170