Feature Request: Undici / Global Fetch integration

[flovilmart]

Hi there!

We're starting to shift away from axios / request and wish to roll out undici in our new deployments.
Unfortunately, there is no 'official' tracing package for it.

Is it something that is being worked on? Would a PR be accepted to add an undici plugin? Should I be aware of any 🐉 ?

Thanks!

[rochdev]

Is it something that is being worked on?

We've been looking into using Undici for sending traces, but we haven't looked into writing a plugin for it yet.

Would a PR be accepted to add an undici plugin?

Definitely accepted and welcomed!

Should I be aware of any 🐉 ?

Not that I know of, it should be pretty similar to other HTTP servers/clients that are already supported.

[flovilmart]

Thanks @rochdev ; I'll draw some inspiration from the http tracing plugin.

[bengl]

@flovilmart Note also that undici has also recently added support for diagnostics channel, so we'll want to use that in versions of undici where that's available.

https://github.com/nodejs/undici/blob/main/docs/api/DiagnosticsChannel.md

[flovilmart]

That's a super neat feature those diagnostics channels!
I assume we want both implementations and patch based on the node.js and undici versions?

[rochdev]

@flovilmart It should be based on the undici version, but the Node version shouldn't matter since in older versions the module will not exist so the plugin will simply never run. Since we're in the process of rewriting a lot of how plugins work and there are no examples currently of using diagnostics channel in a plugin, I would recommend writing the plugin like any other plugin (for example HTTP) and not worry about diagnostics channel for now.

[flovilmart]

OK that works for me @rochdev , we're not even on node 16 yet! I planned to get on that next week.

[flovilmart]

Got this nodejs/undici#1053 patch done for undici for better compatibility.

Outside the request call, patching all other API's (pipeline, stream etc...) is proven to be cumbersome.

The implementation through the diag channels is very straightforward with some book-keeping.
I'll visit the implementation through patching the Client and Request objects after.

[flovilmart]

@rochdev , what do you think about this:nodejs/undici#1053 (comment)?

[rochdev]

@flovilmart Actually it's fine if the implementation in Undici is backed 100% by diagnostics channel. My thought was that we need to do the monkey-patching anyway to support older versions of Undici without the channel available. In that sense, the idea was to first implement the monkey-patching and then add support to use the channel once it lands in Undici. This doesn't mean adding any monkey-patching ability to Undici since that would be redundant when the channels are available.

So basically:

• dd-trace would support older versions of Undici with monkey-patching.
• dd-trace would support newer versions of Undici with diagnostics channel subscribers when the publishers are implemented in Undici.

In both cases, no change is needed in Undici other than adding diagnostics channel support.

[flovilmart]

@rochdev @bengl I've open the PR above to gather initial feedback around the implementation.
Notably, the next tests seem to fails because of the inclusion of the diagnostic_channels module. Any guidance on the workaround?

For now, I'm waiting for a new release of undici, which explains why the tests are currently failing.

[rochdev]

Notably, the next tests seem to fails because of the inclusion of the diagnostic_channels module. Any guidance on the workaround?

@flovilmart Not sure why these would interact. Can you try rebasing the PR? There was a change to how Next is tests a few weeks ago, maybe that change wasn't in your branch and it would fix the issue.

[flovilmart]

Interesting! I've rebased, let's see how it goes!

[JustinTRoss]

Any resolution on this since? I see in your PR that there has been some back and forth on whether to solve monkey-patching or diagnostic channels approach first.

Are we pinning this to Undici DC PR as a dependency?

[flovilmart]

@JustinTRoss I haven't had a chance to actually move forward with a new implementation; the monkey patching look very complex given undici's architecture; there are multiple APIs that leverage dispatchers differently, I haven't found any "clean" monkey patching strategy for all APIs (request, client, pipeline etc...)

[Grmiade]

Any news on this subject?
Since the last versions of NodeJS use Undici to implement the built-in Fetch API, it could be great to have this support =)

[flovilmart]

@Grmiade yep agreed - I haven't had the chance to improve on the current design and address the comments.

[rochdev]

We're actually in the process of finalizing the approach of instrumenting with async_hooks and diagnostics_channel now that we have rewritten 50+ plugins and put it to the test, so might be worth it waiting a few weeks so that this can be added in Undici directly. We're planning to implement the new API directly in Node core and ship a polyfill.

[kibertoad]

@flovilmart Maybe it might make sense to skip the complex monkey-patching altogether and only support newer versions of Undici?

[rochdev]

Quick update related to the above: we're currently finishing up the new approach which we'll be trying internally in the next few weeks, and we plan to add instrumentation to Undici directly in Node core soon after. That work can be tracked in nodejs/node#44943

[flovilmart]

Amazing work @rochdev glad your team was able to pull it through! That was not a simple endeavour!

[mrkosima]

hello there, are there any updates on Undici support?

[saimon-moore]

Looks like this nodejs/node#44943 was merged so I guess this is closer now?

[SimpleCreations]

I see that support for fetch has been added here (#3258).
Does this mean this issue can be closed now?

[cassidycodes]

We tested out undici for our gateway application and datadog didn't report any traces for backend services when we used undici. I don't think this issue can be closed until dd-trace supports undici.

We are using dd-trace-js v 4.14.0

[SimpleCreations]

I meant to say that most people want undici support because the Fetch API uses undici. But I guess there are cases when one might use undici without fetch.