Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

process: delay and simplify the setup of async hooks trace events #26062

Closed
wants to merge 4 commits into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Feb 12, 2019

src: move async hooks trace events setup to pre_execution.js

Reasons:

  • Moves more environment-dependent setup out of bootstrap/node.js
  • No async operations should be done before the call to
    the setup functions in pre_execution.js so no async hooks should be
    triggered before that. Therefore it is safe to delay the setup
    until then.

process: simplify the setup of async hooks trace events

  • Remove trace_category_state from Environment - since this is
    only accessed in the bootstrap process and later in the
    trace category update handler, we could just pass the initial
    values into JS land via the trace_events binding, and pass
    the dynamic values directly to the handler later, instead of
    accessing them out-of-band via the AliasedBuffer.
  • Instead of creating the hooks directly in
    trace_events_async_hooks.js, export the hook factory and
    create the hooks in trace category state toggle.

test: add test for node.async_hooks tracing in workers

test: add test for dynamically enabling node.async_hooks tracing

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

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Feb 12, 2019
@joyeecheung joyeecheung requested a review from jasnell February 12, 2019 23:18
@joyeecheung
Copy link
Member Author

@jasnell
Copy link
Member

jasnell commented Feb 18, 2019

Trace events are not available in worker threads.

I think I missed something. Why is this? They should be available.

@addaleax
Copy link
Member

@jasnell Right – it’s just the JS module that’s not accessible… Is there a test for this code? I think that should have failed in CI if this PR breaks trace_events + Workers, but it looks like everything passed here

@joyeecheung
Copy link
Member Author

@addaleax I don't think there is a test, it could be added but I don't think the dynamic switch can be tested in workers since dynamically enabling the category requires the trace_events module. I could add the preparation code to the worker bootstrap script though, at least that allows us to test the initial enabling of the events.

Reasons:

- Moves more environment-dependent setup out of bootstrap/node.js
- No async operations should be done before the call to
  the setup functions in pre_execution.js so no async hooks should be
  triggered before that. Therefore it is safe to delay the setup
  until then.
- Remove `trace_category_state` from `Environment` - since this is
  only accessed in the bootstrap process and later in the
  trace category update handler, we could just pass the initial
  values into JS land via the trace_events binding, and pass
  the dynamic values directly to the handler later, instead of
  accessing them out-of-band via the AliasedBuffer.
- Instead of creating the hooks directly in
  `trace_events_async_hooks.js`, export the hook factory and
  create the hooks in trace category state toggle.
@joyeecheung
Copy link
Member Author

I added the setup to the worker bootstrap, fixed a bug in the C++ part (when the category is enabled the value is 0x7, not 1, so we should test for != 0 instead), and added two tests. @jasnell @addaleax PTAL.

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

@joyeecheung
Copy link
Member Author

Added hasTracing guards in tests. Not sure why the CI complained about missing dynamically linked deps and libc, let's see what happens in a new CI: https://ci.nodejs.org/job/node-test-pull-request/20907/

@joyeecheung
Copy link
Member Author

@richardlau Thanks! I've updated the env passed to spawnSync. CI: https://ci.nodejs.org/job/node-test-pull-request/20913/

@joyeecheung joyeecheung added async_hooks Issues and PRs related to the async hooks subsystem. trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. labels Feb 21, 2019
@joyeecheung
Copy link
Member Author

Landed in dec3dad...85df2c4

joyeecheung added a commit that referenced this pull request Feb 21, 2019
Reasons:

- Moves more environment-dependent setup out of bootstrap/node.js
- No async operations should be done before the call to
  the setup functions in pre_execution.js so no async hooks should be
  triggered before that. Therefore it is safe to delay the setup
  until then.

PR-URL: #26062
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
joyeecheung added a commit that referenced this pull request Feb 21, 2019
- Remove `trace_category_state` from `Environment` - since this is
  only accessed in the bootstrap process and later in the
  trace category update handler, we could just pass the initial
  values into JS land via the trace_events binding, and pass
  the dynamic values directly to the handler later, instead of
  accessing them out-of-band via the AliasedBuffer.
- Instead of creating the hooks directly in
  `trace_events_async_hooks.js`, export the hook factory and
  create the hooks in trace category state toggle.

PR-URL: #26062
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
joyeecheung added a commit that referenced this pull request Feb 21, 2019
PR-URL: #26062
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
joyeecheung added a commit that referenced this pull request Feb 21, 2019
PR-URL: #26062
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Feb 21, 2019
Reasons:

- Moves more environment-dependent setup out of bootstrap/node.js
- No async operations should be done before the call to
  the setup functions in pre_execution.js so no async hooks should be
  triggered before that. Therefore it is safe to delay the setup
  until then.

PR-URL: #26062
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Feb 21, 2019
- Remove `trace_category_state` from `Environment` - since this is
  only accessed in the bootstrap process and later in the
  trace category update handler, we could just pass the initial
  values into JS land via the trace_events binding, and pass
  the dynamic values directly to the handler later, instead of
  accessing them out-of-band via the AliasedBuffer.
- Instead of creating the hooks directly in
  `trace_events_async_hooks.js`, export the hook factory and
  create the hooks in trace category state toggle.

PR-URL: #26062
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Feb 21, 2019
PR-URL: #26062
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Feb 21, 2019
PR-URL: #26062
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Feb 26, 2019
rvagg pushed a commit that referenced this pull request Feb 28, 2019
Reasons:

- Moves more environment-dependent setup out of bootstrap/node.js
- No async operations should be done before the call to
  the setup functions in pre_execution.js so no async hooks should be
  triggered before that. Therefore it is safe to delay the setup
  until then.

PR-URL: #26062
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
- Remove `trace_category_state` from `Environment` - since this is
  only accessed in the bootstrap process and later in the
  trace category update handler, we could just pass the initial
  values into JS land via the trace_events binding, and pass
  the dynamic values directly to the handler later, instead of
  accessing them out-of-band via the AliasedBuffer.
- Instead of creating the hooks directly in
  `trace_events_async_hooks.js`, export the hook factory and
  create the hooks in trace category state toggle.

PR-URL: #26062
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
PR-URL: #26062
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
PR-URL: #26062
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants