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

2016-12-02 - Node Interactive - Notes #76

Merged
merged 1 commit into from
Dec 7, 2016
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
238 changes: 238 additions & 0 deletions wg-meetings/2016-12-02.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,238 @@
# Diagnostics WG Meeting

* Date: 2016-12-02 (Collaborators' Summit - Austin)
* GitHub issue: https://github.com/nodejs/summit/issues/30
* Raw notes: <https://docs.google.com/document/d/1hUiG1nv4hxjnznN2NjU3F-MzXtKhWxvMI3R8caJyzCs>

## Agenda:

* TracingController
* Inspector and Protocol
* async_hooks

---

## TracingController

Agenda:

* Intended use cases
* JS API
* Trace points in Core


### Intended use cases

For use with timing; intended first for statistical and event-based profiling.

If people would use for broader event logging there could be perf implications.

Could it be used as an alternative form for string-based logging (like
console.log) too, and address some of the problems with console.log?

Core thesis is that profiling, tracing, etc. are all the same, all track
something with a timestamp.

Can timestamp be correlated with other events? Within the same process,
`process.hrtime` also uses the same clock so can be used for correlation.

What about a shared clock across processes? No, but TRACE_EVENT has a clock
sync event we might investigate.


### JS API

Is there anything we’re risking, e.g. perf or security, by exposing to userland?

* Maybe, but it’s worse with current implementations.
* If we allow arbitrary messages in a trace API, could spend too much time
building strings which aren’t used. Need to expose API in a way that only does
these steps when needed.

Should we provide this API through `console`, e.g. `console.trace` or somesuch?

Need a convention for category naming in userland.

* use package names?

Do we need to define a convention for the `args` structures?

* For what? What beyond the core fields defined in [Chrome Tracing Format][]
might be broadly needed in userland modules?

Do we need a "level"-like field as part of the core set? It would have
categories like Error, Warning, and Info and could be a primary field for filtering.

* Could use a string in the category, e.g. `pkgname:verbose`. But will
authors use many different names, e.g. "verbose" and "info"?

[Chrome Tracing Format]: https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAySU/

Consensus is that we should expose this to userland.


### Trace Points in Core

First step is to use macros throughout C++ in Node core and develop more opinions.

Specifically:

* libuv barriers, especially async_wrap
* to track event loop metrics
* current usages of debug.log

Would be ideal to be able to choose categories with a regex. To do so need to add regex parsing to current impl.


### What remains to land PR?

Provide an experimental warning if `--enable-tracing` runtime flag is specified.

@mhdawson: Can this implementation be abstracted so as not to depend on V8?

@mattloring: Work in progress to abstract this infrastructure to an independent
tracing library (libtracing).

@mhdawson: Should we wait for that then?

@ofrobots: To use that new implementation once available we'll only have to change
the build configuration.

@mattloring: The actual interface is the macros, which will not change.

Discussion to continue in GitHub
(https://github.com/nodejs/node/pull/9304#discussion_r90739052)

* Wrap macros for simpler use? Not useful to add a layer in Node just for this.
* Specify log location? Could be added later.
* Programmatic API to access buffers? Could be added later.
* Is there a way to listen for events? Could be added later.
* Does Chromium have sinks we can reuse, e.g. for ETW. There may be one in Chromium to start from.
* Check SmartOS failure.

Unless there’s objection, go ahead and land.


### **Next steps** for TracingController:

* Add an "experimental" warning if `--enable-tracing` is specified.
* Discuss if/how to abstract from V8.
* If no objections, land initial PR (#9304).
* List next tasks in Diag repo, such as instrumenting the wraps in core, adding a JS
API, and adding a callback mode.


---

## Inspector and Protocol

Old V8 debugger to be removed: https://github.com/nodejs/node/issues/9789.

But our deprecation policy is typically at least 2 majors. @ofrobots to
investigate.

What needs to be deprecated in Node as part of removal of old V8 debugger?

* `lib/_debugger.js`
* use of special debug Context, e.g. as returned by [GetDebugContext](https://github.com/v8/v8/blob/02c6b04179a632a3d7a493e60086e5f4c5f47f95/include/v8-debug.h#L251).
* other uses of [include/v8-debug.h](https://github.com/v8/v8/blob/02c6b04179a632a3d7a493e60086e5f4c5f47f95/include/v8-debug.h)?

**Next steps**:

* In issue for removal of old debugger [node#9789](https://github.com/nodejs/node/issues/9789) discuss what else will be effected/deprecated.

### CLI Debugger

We’ll need to deprecate `node debug` soon since it’s based on deprecated protocol.

Should we replace `lib/_debugger.js` (the CLI debugger, used through `node
debug`) with one that implements the new protocol?

[node-inspect](https://github.com/nodejs/diagnostics/issues/67) by @jkrems can do
that.

Questions to answer:

* Does Node need an official CLI debugger, maintained in core?
* Should `node-inspect` be in the Foundation?
* Should it be bundled in default distribution?
* Should it be in `./deps` or `./lib`?

What are the use cases which would justify it in core? When no GUI is
available. Still, could be acquired via a package manager. What about if no
network/package manager is available?

Do other langs have built-in debuggers? Some.

**Consensus** that node-inspect should be in the Foundation.

Continue discussion on bundling in Core on GitHub.

**Next steps**:

* Open a specific issue for deprecating existing CLI debugger
(`lib/_debugger.js`, `node debug`).
* Open an issue in the TSC repo to bring node-inspect into the Foundation.
* Continue discussion on whether and how a CLI debugger should be bundled.

### Activate inspector in running process

[node#8464](https://github.com/nodejs/node/issues/8464)

Final suggested options are:

1. Activate both inspector and debugger and turn one off based on initial message.
2. Don’t do anything and switch default to activate inspector in 8.0.

Consensus is option 2 and to switch the default for SIGUSR1 **now** in master only to
activate inspector. This would land in 8.x releases.

**Next steps**:

* Reply in existing issue that we plan to completely switch in v8.x.
* Notify client debugger projects.
* Message to community.

### **Next steps** for Inspector and Protocol

* In issue for removal of old debugger [node#9789](https://github.com/nodejs/node/issues/9789) discuss what else will be effected/deprecated.
* Open a specific issue for deprecating existing CLI debugger (`lib/_debugger.js`, `node debug`).
* Open an issue in the TSC repo to bring node-inspect into the Foundation.
* Continue discussion on whether and how a CLI debugger should be bundled in default distribition.
* Reply in [node#8464](https://github.com/nodejs/node/issues/8464) that we plan to completely switch to inspector in v8.x.
* Notify client debugger projects about switch of SIGUSR1.
* Message changes to community.


---

## async_hooks & context propagation

* PR: https://github.com/nodejs/node/pull/8531
* Node-EP: https://github.com/nodejs/node-eps/pull/18

How can we help the ecosystem with this problem of tracking context across async
boundaries?

Domains and Zones were attempts.

Could we add a context variable whenever a function is registered? Problem is
that it's not clear what context the programmer would intend to capture.

async_hooks only works with Node’s native object hierarchy (\*Wraps). What about
callbacks handled outside of Node’s hierarchy or in userland? E.g. Bluebird.

Userland queueing of callbacks breaks the async tracking mechanisms we’ve tried,
cause many callbacks come from a single source.

async_hooks originally had this problem, trying to address by adding a userland
API - "Embedder API." This will allow module owners to fire their own hooks.

Embedder API doesn’t help modules which are entirely in JS.

Do we have a native (C++) API for async_hooks? Not at the moment, but could be added.

**Next steps**:

* Review Embedder API added to async_hooks and start explaining to ecosystem.