Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve developer experience (Debuggability) #1001

Closed
ThisIsMissEm opened this issue Aug 24, 2021 · 13 comments
Closed

Improve developer experience (Debuggability) #1001

ThisIsMissEm opened this issue Aug 24, 2021 · 13 comments
Labels
enhancement New feature or request

Comments

@ThisIsMissEm
Copy link

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 ThisIsMissEm added the enhancement New feature or request label Aug 24, 2021
@mcollina
Copy link
Member

Here is the PR that adds the diagnostics_channel integrations #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.

@ThisIsMissEm
Copy link
Author

@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?

@mcollina
Copy link
Member

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!

@ThisIsMissEm
Copy link
Author

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
Copy link
Author

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

@mcollina
Copy link
Member

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.

@ThisIsMissEm
Copy link
Author

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.

@Qard
Copy link
Member

Qard commented Sep 27, 2021

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.

@fengmk2

This comment has been minimized.

@mcollina

This comment has been minimized.

@fengmk2

This comment has been minimized.

@mcollina

This comment has been minimized.

@metcoder95
Copy link
Member

Can we close this issue after v6.3.0?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants