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

Missing diagnostics channel http.client.request.error #635

Closed
dreamorosi opened this issue May 2, 2024 · 5 comments · Fixed by nodejs/node#54054
Closed

Missing diagnostics channel http.client.request.error #635

dreamorosi opened this issue May 2, 2024 · 5 comments · Fixed by nodejs/node#54054
Labels

Comments

@dreamorosi
Copy link

Hi, I am not sure if this is the right forum to ask this question, but I since Node.js diagnostics_channel seems to fall under this working group I got here.

As of today, according to the Node.js docs there are two HTTP built-in channels for client events: http.client.request.start and http.client.response.finish.

Looking at the Node.js http module implementation, and after some tests it appears that there might be a gap in the events.

Subscribers can get notified when a request starts and when a response arrives, but there seems to not be a channel to subscribe for when the request fails (i.e. in the event of a ECONNREFUSED).

I am experimenting with tracing requests made with the http and https modules without having to patch the modules directly (which would require a loader with ESM) and without this type of channel/event if the request is going to fail any span created on http.client.request.start will remain orphaned & opened.

The fetch module has similar channels, but also has a undici:request:error channel that can be used for this.

Is the lack of this channel for the built-in HTTP events intentional? Am I misinterpreting entirely why these channels are there in the first place?

@Flarna
Copy link
Member

Flarna commented May 2, 2024

I don't think it's intentional. There was simply noone to take the time to implement all these events.

@dreamorosi
Copy link
Author

Thank you for your reply.

I'll spend some time understanding what would be the appropriate process to propose this, in the meanwhile I'd be interested - if possible - to hear how other APM vendors are approaching the problem of instrumenting http requests with ESM.

I'm familiar with loader-based approaches like import-in-the-middle and I am wondering if there's any other approach that you have seen.

@Flarna
Copy link
Member

Flarna commented May 3, 2024

http is a built in module and not ESM so besides ESM customization hooks it should be also possible to use old style monkey patching.
Clearly all this approach to monkey patch node internals, rewrite imported files, change import graph go hand in hand with risk.

Copy link

github-actions bot commented Aug 2, 2024

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

@github-actions github-actions bot added the stale label Aug 2, 2024
@dreamorosi
Copy link
Author

This seems to have landed in nodejs/node#54054 - we can close for now.

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

Successfully merging a pull request may close this issue.

2 participants