undici icon indicating copy to clipboard operation
undici copied to clipboard

Improve developer experience (Debuggability)

Open ThisIsMissEm opened this issue 4 years ago • 8 comments
trafficstars

Context:

In the node.js ecosystem, largely lead by express & the debug module, there is the ability to turn on lower level debugging than that which you would normally use in production environments, as to gain knowledge about requests being sent and received, similar to the NODE_DEBUG environment variable for debugging streams, or the DEBUG environment variable from the debug module that can be used to enable a lot of output in a wide variety of modules.

This would solve...

Issues where the whilst developing an client using undici, the request or response isn't what you're expecting. For instance, you're making requests that have dynamically generated urls or headers and the target server responds with an error, or a request that should result in a 200 but for some reason returns a 303.

I would consider this to be something that you'd only ever run on a development machine

The implementation should look like...

I think this could use the diagnostics channels as mentioned in #980, and just be a listener for the channels that prints to stdout, enabled by an environment variable; This should have no effect in production: if you want to capture diagnostics info in production, use an APM.

I have also considered...

Whilst I considered suggesting just using the debug module, I think diagnostic channels are probably the better way forwards. I would just like to see Undici have a bundled "APM less" module for listening to the channels, for as a developer, I don't want to manually have to re-implement all the diagnostic events just to figure out what's happening in my local environment (somewhere where using an APM isn't an option).

Additional context

None I can think of.

ThisIsMissEm avatar Aug 24 '21 22:08 ThisIsMissEm

Here is the PR that adds the diagnostics_channel integrations https://github.com/nodejs/undici/pull/1000. I think it would be a good place to add support for NODE_DEBUG.

FYI, we can just use https://nodejs.org/api/util.html#util_util_debuglog_section_callback and have it behave like Node.js core. I would recommend adding those hooks in the https://github.com/nodejs/undici/blob/main/lib/core/request.js class to begin with.

mcollina avatar Aug 25 '21 13:08 mcollina

@mcollina would you like to use the diagnostics_channel and have a subscriber to that for writing to utils.debuglog, and the subscriber is only registered if the debuglog is enabled?

ThisIsMissEm avatar Aug 25 '21 14:08 ThisIsMissEm

I would just use debuglog wherever needed, likely in the same place we call the diagn channel. We could possibly do it in another places as well!

mcollina avatar Aug 25 '21 16:08 mcollina

Wouldn't it be cleaner to use the diagnostics channels though? One thing to maintain the data production, another to report to debuglog? Could also help become a pattern for using diagnostic channels for both APMs and for development debug logging, i.e., if you can debug in development, you can get APM for it

If you'd be cool with using the diagnostics channels, I'd be happy to try and write a PR to add a debuglog for them.

ThisIsMissEm avatar Aug 25 '21 20:08 ThisIsMissEm

(I'm trying to understand why I shouldn't use those for this?)

ThisIsMissEm avatar Aug 25 '21 20:08 ThisIsMissEm

There are two reasons:

  1. diagnostics_channel is only available in v15 and v16.
  2. the requirements for supporting APMs and developers are only partially overlapping and we might end up with a solution that is not 100% satisfactory for both.

mcollina avatar Aug 26 '21 07:08 mcollina

oooh, okay! Those are very good points! (I totally thought diagnostics_channel was out already 😅 ) and yeah, you're right that they're not fully overlapping; I just didn't wanna add unnecessary code to what's likely a very hot code path.

ThisIsMissEm avatar Aug 26 '21 15:08 ThisIsMissEm

diagnostics_channel is in v14 too. Agreed that logging, at least while diagnostics_channel is not available across all version, should probably just be done directly.

Qard avatar Sep 27 '21 15:09 Qard