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

trace_events,async_hooks: use intrinsic trace #22127

Closed
wants to merge 4 commits into from

Conversation

Projects
6 participants
@jasnell
Copy link
Member

commented Aug 4, 2018

Switch to using the faster intrinsic trace event method for async_hooks.

This is a breaking change because of the switch to a nested data argument for exec id and trigger id values. Although trace events and async hooks are still both experimental, there is code deployed that depends on the current data format for async hooks in trace events (e.g. https://github.com/nearform/node-clinic). Therefore, while this is not technically semver-major, I'm marking it "don't land" on 10, 8, and 6.

/cc @mcollina ... once this does land in master, clinic would need to be updated to account for the new data format. The version metadata in the trace event log can be used to determine which format applies.

Example old format: trace_event.args.triggerAsyncId
Example new format: trace_event.args.data.triggerAsyncId

@nodejs/diagnostics @nodejs/trace-events @nodejs/async_hooks

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@jasnell jasnell requested review from mcollina and mafintosh Aug 4, 2018

@AndreasMadsen
Copy link
Member

left a comment

LGTM, but I think this should also remove the old emit API from the binding module.

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Aug 4, 2018

Yeah, I'm planning to remove that but wanted to make sure there were no objections to this first

@mcollina
Copy link
Member

left a comment

LGTM

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Aug 6, 2018

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2018

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2018

Trying again on linux due to a seemingly unrelated failure: https://ci.nodejs.org/job/node-test-commit-linux/20518/

jasnell added some commits Aug 4, 2018

trace_events,async_hooks: use intrinsic trace
Switch to using the intrinsic trace event method for async_hooks.

This is a breaking change because of the switch to a nested data
argument for exec id and trigger id values.
src: remove old process.binding('trace_events').emit
Remove the older emit and categoryGroupEnabled bindings in
favor of the new intrinsics

@jasnell jasnell force-pushed the jasnell:trace_events_async_hooks_args branch from 5d45705 to c5e0db7 Aug 9, 2018

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2018

@mcollina @ofrobots @AndreasMadsen ... updated the PR to remove the older emit and categoryGroupEnabled functions. Please take another look.

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

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2018

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2018

CI is good

@@ -35,6 +36,7 @@ procEnabled.once('exit', common.mustCall(() => {
const procDisabled = cp.spawn(
process.execPath,
[ '--trace-event-categories', 'other',
'--no-warnings',

This comment has been minimized.

Copy link
@AndreasMadsen

AndreasMadsen Aug 10, 2018

Member

Why this? Maybe add a comment.

This comment has been minimized.

Copy link
@jasnell

jasnell Aug 10, 2018

Author Member

just to make the test less noisy since the import of internal/test/binding emits a warning

@mcollina
Copy link
Member

left a comment

LGTM

jasnell added a commit that referenced this pull request Aug 10, 2018

trace_events,async_hooks: use intrinsic trace
Switch to using the intrinsic trace event method for async_hooks.

This is a breaking change because of the switch to a nested data
argument for exec id and trigger id values.

PR-URL: #22127
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>

jasnell added a commit that referenced this pull request Aug 10, 2018

src: remove old process.binding('trace_events').emit
Remove the older emit and categoryGroupEnabled bindings in
favor of the new intrinsics

PR-URL: #22127
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
@jasnell

This comment has been minimized.

Copy link
Member Author

commented Aug 10, 2018

Landed in c85933c and b854604

@jasnell jasnell closed this Aug 10, 2018

@targos targos added this to Don't land (for now) in v10.x Sep 23, 2018

@targos targos moved this from Don't land (for now) to Don't land (ever) in v10.x Sep 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.