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

Scope of inspector/tracing agents #22513

Closed
addaleax opened this issue Aug 24, 2018 · 6 comments
Closed

Scope of inspector/tracing agents #22513

addaleax opened this issue Aug 24, 2018 · 6 comments
Labels
inspector Issues and PRs related to the V8 inspector protocol question Issues that look for answers. trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. worker Issues and PRs related to Worker support.

Comments

@addaleax
Copy link
Member

  • Subsystem: inspector, tracing

Currently, the inspector agent and a possible tracing writer are per-Environment on Node’s side. However, as far as I can tell, on the V8 side they are fundamentally per-Isolate and per-Platform, respectively.

So, my question for @nodejs/v8-inspector @nodejs/trace-events here is:

  • Should we move the Inspector handle and CLI debug options from Environment to IsolateData?
  • Should we move the tracing handle and corresponding CLI options to a global/per-process model?
@addaleax addaleax added question Issues that look for answers. inspector Issues and PRs related to the V8 inspector protocol trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. labels Aug 24, 2018
@jasnell
Copy link
Member

jasnell commented Aug 25, 2018

Yes, tracing should be global/per-Platform in the current implementation. Whether or not that makes sense long term I don't know. /cc @ofrobots

@eugeneo
Copy link
Contributor

eugeneo commented Aug 25, 2018

Inspector agent is per-"target" - i.e. workers have their own instance. Are there any cases when there are multiple isolates for one environment?

IMHO, inspector Agent does not need to know about CLI options at all.

@pavelfeldman
Copy link
Contributor

@eugeneo, Tracing is global, Tracing domain on the root target should handle tracing for everything, including workers. There should be no tracing agents on the workers. Tracing started event should be propagated to all the isolates (workers) so that tracing controller singleton was capturing all the samples from all of them. This functionality would be typically implemented in the tracing controller with the Tracing domain being an interfacing shim sitting on top of it.

@addaleax
Copy link
Member Author

Are there any cases when there are multiple isolates for one environment?

@eugeneo There can be multiple Environments for a single Isolate. Since currently inspector Agents and Environments are 1:1, I guess the question is, do multiple Agents per Isolate conflict with each other?

@pavelfeldman
Copy link
Contributor

do multiple Agents per Isolate conflict with each other?

they don't in chrome, but it requires proper context goup management. @ak239 would know if we already do that in node.

@addaleax addaleax added the worker Issues and PRs related to Worker support. label Sep 23, 2018
addaleax added a commit to addaleax/node that referenced this issue Oct 21, 2018
As per the conversation in nodejs#22513,
this is essentially global, and adding this on the Environment
is generally just confusing.

Refs: nodejs#22513
Fixes: nodejs#22767
mmarchini pushed a commit that referenced this issue Oct 24, 2018
As per the conversation in #22513,
this is essentially global, and adding this on the Environment
is generally just confusing.

Refs: #22513
Fixes: #22767

PR-URL: #23781
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
mmarchini pushed a commit that referenced this issue Oct 24, 2018
Forbid modifying tracing state from worker threads, either
through the built-in module or inspector sessions, since
the main thread owns all global state, and at least
the `async_hooks` integration is definitely not thread
safe in its current state.

PR-URL: #23781
Fixes: #22767
Refs: #22513
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
targos pushed a commit that referenced this issue Oct 27, 2018
As per the conversation in #22513,
this is essentially global, and adding this on the Environment
is generally just confusing.

Refs: #22513
Fixes: #22767

PR-URL: #23781
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
targos pushed a commit that referenced this issue Oct 27, 2018
Forbid modifying tracing state from worker threads, either
through the built-in module or inspector sessions, since
the main thread owns all global state, and at least
the `async_hooks` integration is definitely not thread
safe in its current state.

PR-URL: #23781
Fixes: #22767
Refs: #22513
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
targos pushed a commit that referenced this issue Nov 18, 2018
As per the conversation in #22513,
this is essentially global, and adding this on the Environment
is generally just confusing.

Refs: #22513
Fixes: #22767

PR-URL: #23781
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
targos pushed a commit that referenced this issue Nov 18, 2018
Forbid modifying tracing state from worker threads, either
through the built-in module or inspector sessions, since
the main thread owns all global state, and at least
the `async_hooks` integration is definitely not thread
safe in its current state.

PR-URL: #23781
Fixes: #22767
Refs: #22513
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
rvagg pushed a commit that referenced this issue Nov 28, 2018
As per the conversation in #22513,
this is essentially global, and adding this on the Environment
is generally just confusing.

Refs: #22513
Fixes: #22767

PR-URL: #23781
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
rvagg pushed a commit that referenced this issue Nov 28, 2018
Forbid modifying tracing state from worker threads, either
through the built-in module or inspector sessions, since
the main thread owns all global state, and at least
the `async_hooks` integration is definitely not thread
safe in its current state.

PR-URL: #23781
Fixes: #22767
Refs: #22513
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
@addaleax
Copy link
Member Author

This is answered as far as I am concerned, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inspector Issues and PRs related to the V8 inspector protocol question Issues that look for answers. trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. worker Issues and PRs related to Worker support.
Projects
None yet
Development

No branches or pull requests

4 participants