-
Notifications
You must be signed in to change notification settings - Fork 29k
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: add tracing channel #44943
Conversation
It might be helpful to allow/support something like |
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. |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great 🙌
You probably will add documentation, but I'd like to see a full use case with an HTTP server there ^^
95f3198
to
0438150
Compare
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. |
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 |
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. |
ac655d8
to
f3afea1
Compare
f3afea1
to
8765e7d
Compare
8765e7d
to
db4c8c9
Compare
db4c8c9
to
9128df4
Compare
This comment was marked as outdated.
This comment was marked as outdated.
1d8b89e
to
1215d64
Compare
|
||
channel.start.bindStore(store, common.mustCall(() => { | ||
return context; | ||
})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'll do that as a follow up. Don't want to tempt fate with CI the day before v20 cut-off. 😂
This looks good on our end! |
Landed in fe751df |
PR-URL: #44943 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Bryan English <[email protected]>
PR-URL: #44943 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Bryan English <[email protected]>
PR-URL: #44943 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Bryan English <[email protected]>
PR-URL: #44943 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Bryan English <[email protected]>
PR-URL: #44943 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Bryan English <[email protected]>
Notable changes: deps: * (SEMVER-MINOR) update uvwasi to 0.0.19 (Node.js GitHub Bot) #49908 * (SEMVER-MINOR) upgrade npm to 10.0.0 (npm team) #49423 doc: * add new TSC members (Michael Dawson) #48841 * move and rename loaders section (Geoffrey Booth) #49261 esm: * use import attributes instead of import assertions (Antoine du Hamel) #50140 * --experimental-default-type flag to flip module defaults (Geoffrey Booth) #49869 * unflag import.meta.resolve (Guy Bedford) #49028 * move hook execution to separate thread (Jacob Smith) #44710 * leverage loaders when resolving subsequent loaders (Maël Nison) #43772 lib: * (SEMVER-MINOR) add api to detect whether source-maps are enabled (翠 / green) #46391 * (SEMVER-MINOR) add tracing channel to diagnostics_channel (Stephen Belanger) #44943 src: * (SEMVER-MINOR) add cjs_module_lexer_version base64_version (Jithil P Ponnan) #45629 stream: * use bitmap in readable state (Benjamin Gruenbaum) #49745 test_runner: * (SEMVER-MINOR) accept `testOnly` in `run` (Moshe Atlow) #49753 * (SEMVER-MINOR) add junit reporter (Moshe Atlow) #49614 * (SEMVER-MINOR) expose location of tests (Colin Ihrig) #48975 * (SEMVER-MINOR) add shards support (Raz Luvaton) #48639 * (SEMVER-MINOR) add initial draft for fakeTimers (Erick Wendel) #47775 test_runner, cli: * (SEMVER-MINOR) add --test-concurrency flag (Colin Ihrig) #49996 tls: * (SEMVER-MINOR) add ALPNCallback server option for dynamic ALPN negotiation (Tim Perry) #45190 vm: * (SEMVER-MINOR) use import attributes instead of import assertions (Antoine du Hamel) #50141 wasi: * (SEMVER-MINOR) updates required for latest uvwasi version (Michael Dawson) #49908 PR-URL: TODO
Notable changes: deps: * (SEMVER-MINOR) update uvwasi to 0.0.19 (Node.js GitHub Bot) #49908 * (SEMVER-MINOR) upgrade npm to 10.2.3 (npm team) #50531 doc: * move and rename loaders section (Geoffrey Booth) #49261 esm: * use import attributes instead of import assertions (Antoine du Hamel) #50140 * --experimental-default-type flag to flip module defaults (Geoffrey Booth) #49869 * unflag import.meta.resolve (Guy Bedford) #49028 * move hook execution to separate thread (Jacob Smith) #44710 * leverage loaders when resolving subsequent loaders (Maël Nison) #43772 lib: * (SEMVER-MINOR) add api to detect whether source-maps are enabled (翠 / green) #46391 * (SEMVER-MINOR) add tracing channel to diagnostics_channel (Stephen Belanger) #44943 src: * (SEMVER-MINOR) add cjs_module_lexer_version base64_version (Jithil P Ponnan) #45629 stream: * use bitmap in readable state (Benjamin Gruenbaum) #49745 test_runner: * (SEMVER-MINOR) accept `testOnly` in `run` (Moshe Atlow) #49753 * (SEMVER-MINOR) add junit reporter (Moshe Atlow) #49614 * (SEMVER-MINOR) expose location of tests (Colin Ihrig) #48975 * (SEMVER-MINOR) add shards support (Raz Luvaton) #48639 * (SEMVER-MINOR) add initial draft for fakeTimers (Erick Wendel) #47775 test_runner, cli: * (SEMVER-MINOR) add --test-concurrency flag (Colin Ihrig) #49996 tls: * (SEMVER-MINOR) add ALPNCallback server option for dynamic ALPN negotiation (Tim Perry) #45190 vm: * (SEMVER-MINOR) use import attributes instead of import assertions (Antoine du Hamel) #50141 PR-URL: #50953
Notable changes: deps: * (SEMVER-MINOR) update uvwasi to 0.0.19 (Node.js GitHub Bot) #49908 * (SEMVER-MINOR) upgrade npm to 10.2.3 (npm team) #50531 doc: * move and rename loaders section (Geoffrey Booth) #49261 esm: * use import attributes instead of import assertions (Antoine du Hamel) #50140 * --experimental-default-type flag to flip module defaults (Geoffrey Booth) #49869 * unflag import.meta.resolve (Guy Bedford) #49028 * move hook execution to separate thread (Jacob Smith) #44710 * leverage loaders when resolving subsequent loaders (Maël Nison) #43772 lib: * (SEMVER-MINOR) add api to detect whether source-maps are enabled (翠 / green) #46391 * (SEMVER-MINOR) add tracing channel to diagnostics_channel (Stephen Belanger) #44943 src: * (SEMVER-MINOR) add cjs_module_lexer_version base64_version (Jithil P Ponnan) #45629 stream: * use bitmap in readable state (Benjamin Gruenbaum) #49745 test_runner: * (SEMVER-MINOR) accept `testOnly` in `run` (Moshe Atlow) #49753 * (SEMVER-MINOR) add junit reporter (Moshe Atlow) #49614 * (SEMVER-MINOR) expose location of tests (Colin Ihrig) #48975 * (SEMVER-MINOR) add shards support (Raz Luvaton) #48639 * (SEMVER-MINOR) add initial draft for fakeTimers (Erick Wendel) #47775 test_runner, cli: * (SEMVER-MINOR) add --test-concurrency flag (Colin Ihrig) #49996 tls: * (SEMVER-MINOR) add ALPNCallback server option for dynamic ALPN negotiation (Tim Perry) #45190 vm: * (SEMVER-MINOR) use import attributes instead of import assertions (Antoine du Hamel) #50141 PR-URL: #50953
PR-URL: nodejs/node#44943 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Bryan English <[email protected]>
Notable changes: deps: * (SEMVER-MINOR) update uvwasi to 0.0.19 (Node.js GitHub Bot) nodejs/node#49908 * (SEMVER-MINOR) upgrade npm to 10.2.3 (npm team) nodejs/node#50531 doc: * move and rename loaders section (Geoffrey Booth) nodejs/node#49261 esm: * use import attributes instead of import assertions (Antoine du Hamel) nodejs/node#50140 * --experimental-default-type flag to flip module defaults (Geoffrey Booth) nodejs/node#49869 * unflag import.meta.resolve (Guy Bedford) nodejs/node#49028 * move hook execution to separate thread (Jacob Smith) nodejs/node#44710 * leverage loaders when resolving subsequent loaders (Maël Nison) nodejs/node#43772 lib: * (SEMVER-MINOR) add api to detect whether source-maps are enabled (翠 / green) nodejs/node#46391 * (SEMVER-MINOR) add tracing channel to diagnostics_channel (Stephen Belanger) nodejs/node#44943 src: * (SEMVER-MINOR) add cjs_module_lexer_version base64_version (Jithil P Ponnan) nodejs/node#45629 stream: * use bitmap in readable state (Benjamin Gruenbaum) nodejs/node#49745 test_runner: * (SEMVER-MINOR) accept `testOnly` in `run` (Moshe Atlow) nodejs/node#49753 * (SEMVER-MINOR) add junit reporter (Moshe Atlow) nodejs/node#49614 * (SEMVER-MINOR) expose location of tests (Colin Ihrig) nodejs/node#48975 * (SEMVER-MINOR) add shards support (Raz Luvaton) nodejs/node#48639 * (SEMVER-MINOR) add initial draft for fakeTimers (Erick Wendel) nodejs/node#47775 test_runner, cli: * (SEMVER-MINOR) add --test-concurrency flag (Colin Ihrig) nodejs/node#49996 tls: * (SEMVER-MINOR) add ALPNCallback server option for dynamic ALPN negotiation (Tim Perry) nodejs/node#45190 vm: * (SEMVER-MINOR) use import attributes instead of import assertions (Antoine du Hamel) nodejs/node#50141 PR-URL: nodejs/node#50953
PR-URL: nodejs/node#44943 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Bryan English <[email protected]>
Notable changes: deps: * (SEMVER-MINOR) update uvwasi to 0.0.19 (Node.js GitHub Bot) nodejs/node#49908 * (SEMVER-MINOR) upgrade npm to 10.2.3 (npm team) nodejs/node#50531 doc: * move and rename loaders section (Geoffrey Booth) nodejs/node#49261 esm: * use import attributes instead of import assertions (Antoine du Hamel) nodejs/node#50140 * --experimental-default-type flag to flip module defaults (Geoffrey Booth) nodejs/node#49869 * unflag import.meta.resolve (Guy Bedford) nodejs/node#49028 * move hook execution to separate thread (Jacob Smith) nodejs/node#44710 * leverage loaders when resolving subsequent loaders (Maël Nison) nodejs/node#43772 lib: * (SEMVER-MINOR) add api to detect whether source-maps are enabled (翠 / green) nodejs/node#46391 * (SEMVER-MINOR) add tracing channel to diagnostics_channel (Stephen Belanger) nodejs/node#44943 src: * (SEMVER-MINOR) add cjs_module_lexer_version base64_version (Jithil P Ponnan) nodejs/node#45629 stream: * use bitmap in readable state (Benjamin Gruenbaum) nodejs/node#49745 test_runner: * (SEMVER-MINOR) accept `testOnly` in `run` (Moshe Atlow) nodejs/node#49753 * (SEMVER-MINOR) add junit reporter (Moshe Atlow) nodejs/node#49614 * (SEMVER-MINOR) expose location of tests (Colin Ihrig) nodejs/node#48975 * (SEMVER-MINOR) add shards support (Raz Luvaton) nodejs/node#48639 * (SEMVER-MINOR) add initial draft for fakeTimers (Erick Wendel) nodejs/node#47775 test_runner, cli: * (SEMVER-MINOR) add --test-concurrency flag (Colin Ihrig) nodejs/node#49996 tls: * (SEMVER-MINOR) add ALPNCallback server option for dynamic ALPN negotiation (Tim Perry) nodejs/node#45190 vm: * (SEMVER-MINOR) use import attributes instead of import assertions (Antoine du Hamel) nodejs/node#50141 PR-URL: nodejs/node#50953
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