diagnostics_channel: add tracing channel - #44943
Conversation
Sorry, something went wrong.
|
It might be helpful to allow/support something like update to provide some more data between start and end, e.g. if some streaming use case is traced. |
Sorry, something went wrong.
|
The idea was to just modify the ctx object whenever updates occur between the events. Open to adding additional events though if we have a need to capture immediate data for whatever reason. |
Sorry, something went wrong.
That should be fine. Main issue I see with the context object is that there are some pre-ocupied property names (error, result as of now) which could easily result in conflicts various channels add more and more such properties to transport more data. |
Sorry, something went wrong.
tony-go
left a comment
There was a problem hiding this comment.
Great 🙌
You probably will add documentation, but I'd like to see a full use case with an HTTP server there ^^
Sorry, something went wrong.
|
Made some changes to split up the logic for sync, callback, and promise functions. What do people think of this design? If this seems reasonable I can move forward with writing docs for it. |
Sorry, something went wrong.
|
I think the split API is better because usually you know if you trace a sync or async function. We might add a "cb or promise" variant later if needed. And there are always special cases which don't fit (e.g. the returned promise is actually a mixin with an event emitter,...). How is it intended that a subscriber knows if operation is sync or async? We could transport the API used on the given context object. Or should this be part of the channel documentation? For the sync part we have start/end which could be used for calling enter/exit on AsyncLocalStore and more (see #44894 (comment)) . But we don't have this for the callback. Is the assumption that this is not needed and the traced library is at least using AsyncResource correctly internally? |
Sorry, something went wrong.
|
Yes, we're only caring about the trace up to when the callback runs. If the user wants more they can use async_hooks. As for detecting sync vs async, we could have the start or end events include some flag like sync: true or sync: false depending on which version was used. |
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
|
||
| channel.start.bindStore(store, common.mustCall(() => { | ||
| return context; | ||
| })); |
There was a problem hiding this comment.
Should we add something similar as in the callback case here:
channel.asyncStart.bindStore(store, common.mustCall(() => {
return secondContext;
}));
But asserts stay as is to actually verify the runStores is not used in promise case?
Not meant as blocking comment, perfectly fine to do this in a followup or skip at all.
Sorry, something went wrong.
There was a problem hiding this comment.
I think I'll do that as a follow up. Don't want to tempt fate with CI the day before v20 cut-off. 😂
Sorry, something went wrong.
|
This looks good on our end! |
Sorry, something went wrong.
This adds a helper to present tracing functionality through a group of channels and a shared context object. The shared context object can be used to communicate meta information about the action being traced.
No effort is made to link traces together, this only provides the basics to express a span for a single sync or async task. It's left up to the user to track and link span data through something like AsyncLocalStorage.
Similar to #44894, I'm starting this as a draft and skipping docs for the moment to get feedback on the API design. If we settle on this design satisfying our needs I'll write up some proper docs for it.
cc @nodejs/diagnostics