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: add tracing channel #44943

Merged
merged 9 commits into from
Mar 31, 2023

Conversation

Qard
Copy link
Member

@Qard Qard commented Oct 10, 2022

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

@Qard Qard added the diagnostics_channel Issues and PRs related to diagnostics channel label Oct 10, 2022
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Oct 10, 2022
lib/diagnostics_channel.js Outdated Show resolved Hide resolved
lib/diagnostics_channel.js Outdated Show resolved Hide resolved
lib/diagnostics_channel.js Outdated Show resolved Hide resolved
@Flarna
Copy link
Member

Flarna commented Oct 12, 2022

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.

@Qard
Copy link
Member Author

Qard commented Oct 12, 2022

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.

@Flarna
Copy link
Member

Flarna commented Oct 13, 2022

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.

Copy link
Member

@tony-go tony-go left a 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 ^^

lib/diagnostics_channel.js Outdated Show resolved Hide resolved
@Qard
Copy link
Member Author

Qard commented Oct 18, 2022

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.

@Flarna
Copy link
Member

Flarna commented Oct 19, 2022

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?

@Qard
Copy link
Member Author

Qard commented Oct 20, 2022

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.

@Qard Qard force-pushed the diagnostics-channel-tracing-channel branch 2 times, most recently from ac655d8 to f3afea1 Compare December 3, 2022 02:26
@Qard Qard force-pushed the diagnostics-channel-tracing-channel branch from f3afea1 to 8765e7d Compare December 14, 2022 03:49
@Qard Qard marked this pull request as ready for review December 14, 2022 03:49
@Qard Qard force-pushed the diagnostics-channel-tracing-channel branch from 8765e7d to db4c8c9 Compare December 15, 2022 00:55
@Qard Qard added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 15, 2022
@Qard Qard force-pushed the diagnostics-channel-tracing-channel branch from db4c8c9 to 9128df4 Compare December 15, 2022 01:25
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 15, 2022
@nodejs-github-bot

This comment was marked as outdated.

@Qard Qard force-pushed the diagnostics-channel-tracing-channel branch 3 times, most recently from 1d8b89e to 1215d64 Compare December 15, 2022 02:50
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/50728/


channel.start.bindStore(store, common.mustCall(() => {
return context;
}));
Copy link
Member

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.

Copy link
Member Author

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. 😂

@tlhunter
Copy link
Contributor

This looks good on our end!

@Qard Qard added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 31, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 31, 2023
@nodejs-github-bot nodejs-github-bot merged commit fe751df into main Mar 31, 2023
@nodejs-github-bot nodejs-github-bot deleted the diagnostics-channel-tracing-channel branch March 31, 2023 17:40
@nodejs-github-bot
Copy link
Collaborator

Landed in fe751df

RafaelGSS pushed a commit that referenced this pull request Apr 5, 2023
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]>
@RafaelGSS RafaelGSS mentioned this pull request Apr 6, 2023
RafaelGSS pushed a commit that referenced this pull request Apr 6, 2023
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]>
RafaelGSS pushed a commit that referenced this pull request Apr 7, 2023
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]>
RafaelGSS pushed a commit that referenced this pull request Apr 8, 2023
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]>
@danielleadams danielleadams added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Jul 3, 2023
targos pushed a commit that referenced this pull request Nov 10, 2023
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]>
targos added a commit that referenced this pull request Nov 27, 2023
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
targos added a commit that referenced this pull request Nov 28, 2023
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
targos added a commit that referenced this pull request Nov 29, 2023
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
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
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]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
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
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
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]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. blocked PRs that are blocked by other issues or PRs. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. diagnostics_channel Issues and PRs related to diagnostics channel needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.