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

[diagnostics_channel] support body received? #1342

Open
KhafraDev opened this issue Apr 17, 2022 · 10 comments
Open

[diagnostics_channel] support body received? #1342

KhafraDev opened this issue Apr 17, 2022 · 10 comments
Labels
enhancement New feature or request

Comments

@KhafraDev
Copy link
Member

This would solve...

In a library I am working on, they had 2 events (request & response) that would fire before and after a request was made. The purpose of these events was for analytics (ie. receiving a "weird" response code and checking the body). With undici, I'm hoping that these events can be removed entirely and instead use the diagnostics_channel support (the library only supports v16.9.0+).

The implementation should look like...

In the request property, the body is sent but under a symbol property and uncloned. Exposing it publicly (maybe under bodyReceived or some other name) and cloning it so it can be used twice would be exactly what I need. Receiving a buffer would also be very useful.

Since it is using request, rather than fetch, it is using a node Readable stream so we can't just tee the stream.

I have also considered...

Additional context

The implementation previously used was not performant and didn't need to be because it was only supposed to be used when something majorly messed up.

@KhafraDev KhafraDev added the enhancement New feature or request label Apr 17, 2022
@mcollina
Copy link
Member

You can't clone a node stream. Here is a module that implements that functionality in case you need it: https://www.npmjs.com/package/cloneable-readable.

Overall everything that improves diagnostics_channel is great. However that's a low level instrument, so cloning would be off the table.

@KhafraDev
Copy link
Member Author

I think with a bodySent property it could be left to the user to use your package... although that might not be the greatest UX?

@mcollina
Copy link
Member

Would you mind to assemble a prototype? You have definitely researched this more than me.

@KhafraDev
Copy link
Member Author

Exposing the ReadableStream body can be done like so (not cloned)

diff --git a/lib/core/request.js b/lib/core/request.js
index f04fe4f..8bbe4e7 100644
--- a/lib/core/request.js
+++ b/lib/core/request.js
@@ -216,7 +216,10 @@ class Request {

     this.completed = true
     if (channels.trailers.hasSubscribers) {
-      channels.trailers.publish({ request: this, trailers })
+      channels.trailers.publish({
+        request: { ...this, bodyReceived: this[kHandler].res },
+        trailers
+      })
     }
     return this[kHandler].onComplete(trailers)
   }

Unsure if it's even useful as it probably would suffer from race conditions (consuming the body somewhere else before cloning)... I'll look into it more

@KhafraDev
Copy link
Member Author

Found a much better way to do this, I'll make a PR soon to see what you think.

@KhafraDev KhafraDev changed the title [diagnostics_channel] support body received in undici:request:trailers [diagnostics_channel] support body received? Apr 28, 2022
@DEVTomatoCake
Copy link

Is there any update regarding this? If not, is there any alternative solution without modifying undici or a package source code?

I would like to see the sent and received body for debugging, as error messages returned by other packages usually don't include the full body.
There's undici:request:bodySent, but apparently it only works for some kind of requests (might be using request vs fetch, it looks like the latter only returns [object AsyncGenerator] instead of the body buffer which I'm unable to convert to a string containing the body).

@metcoder95
Copy link
Member

You can always use interceptors for that. I'm not sure if enabling diagnostics to dig into the body content of the response might be a good idea given the security vectors around, but definitely knowing when the body is received and possible auxiliary data of the response is always helpful

@DEVTomatoCake
Copy link

I don't see how this would help me as I would still have to modify the source code to add the interceptor there, unless you're able to apply this globally which, if possible, I haven't been able to figure out.

@metcoder95
Copy link
Member

You can apply it by creating a custom Client and setting it as setGlobalDispatcher. The interceptors are added to the Client and any with no custom client (either Undici.request or fetch) will have these interceptors.
It can help apply, more or less, what you are looking for

@DEVTomatoCake
Copy link

Looks good, but as I have to specify an URL/domain when creating the Client, using this breaks all requests to URLs that aren't on the same domain as the one specified... However, this'll work for now, thank you so far!

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
4 participants