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

[next] Use diagnostics_channel for http instrumentations #4319

Open
dyladan opened this issue Nov 22, 2023 · 7 comments
Open

[next] Use diagnostics_channel for http instrumentations #4319

dyladan opened this issue Nov 22, 2023 · 7 comments

Comments

@dyladan
Copy link
Member

dyladan commented Nov 22, 2023

Now that we've removed support for old runtimes, we can use the new diagnostics channels to instrument node core http libraries.

Known issues

http.client.request.start - event.request is not writeable

Because the event is emitted after the request is already in flight, we cannot modify the headers. This means we cannot add the required tracing headers for context propagation. We will still need to rely on patching the http and https modules for at least this purpose unless the http channel implementations change.

If nodejs adds something similar to undici's undici:request:create, this issue may be solved and we may be able to remove all manual monkeypatching from the instrumentation. In this case, we may even be able to remove {require|import}-in-the-middle.

Context Management

See #4319 (comment) below

@dyladan
Copy link
Member Author

dyladan commented Nov 22, 2023

/cc @legendecas

@Flarna
Copy link
Member

Flarna commented Nov 22, 2023

Another topic is likely the context manage.

Hooks to get lifecycle of an request is enough to start/end a span and maybe even inject headers. But ContextManger requires to wrap the http.request() call with context.with() to get the context with the span active. Otherwise an inner DNS request wouldn't result in a child span.

There is an advanced diag channel API TracingChannel which puts AsyncLocalStore and DiagChannel together. But as of now noone did the work to actually adapt the http implementation in node to use this.

@dyladan
Copy link
Member Author

dyladan commented Nov 22, 2023

Thanks for the info. It came up in the meeting today but nobody brought up the TracingChannel point. I think the current solution of using bind() on the request/response objects should be ok, but i'll make sure to test that.

@Flarna
Copy link
Member

Flarna commented Nov 22, 2023

I think for client http the req/res objects are bound on the context holding the parent span.
This is because whatever is triggered in e.g. a data or response event is actually no inner operation of the http request (which would be a child span) - it's a followup operation running afterwards so a sibling.

Copy link

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@github-actions github-actions bot added the stale label Jan 22, 2024
Copy link

This issue was closed because it has been stale for 14 days with no activity.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Feb 26, 2024
@legendecas legendecas reopened this Feb 26, 2024
@legendecas
Copy link
Member

http.client.request.created was added and available since 22.12.0. This can be an alternative to http.client.request.start.

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

No branches or pull requests

3 participants