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

Add July meeting notes #63

Merged
merged 1 commit into from
Oct 11, 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
137 changes: 137 additions & 0 deletions wg-meetings/2016-07-13.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
# Node.js Diagnostics WG Meeting

- Date: 2016-07-13
- YouTube: http://www.youtube.com/watch?v=BtZ6aFrD8gQ

## Attendees

- Josh Gavant (@joshgav)
- Michael Dawson (@mhdawson)
- Thomas Watson (@watson)
- Andreas Madsen (@andreasmadsen)
- Matt Loring (@matthewloring)
- Ali Sheikh (@ofrobots)
- Raymond Kang (@misterpoe)
- Cristian Cavalli (@cristiancavalli)
- Daniel Khan (@danielkhan)
- Trevor Norris (@trevnorris)

## Agenda

- AsyncWrap - [[nodejs/node-eps#18](https://github.com/nodejs/node-eps/pull/18)]
- Trace Controller [[nodejs/diagnostics#53](https://github.com/nodejs/diagnostics/issues/53)]
- Inspector Protocol [[nodejs/node#7393](https://github.com/nodejs/node/issues/7393)]
- APM Summit [[nodejs/diagnostics#58](https://github.com/nodejs/diagnostics/issues/58)

## Minutes

## AsyncWrap

- [[nodejs/node-eps#18](https://github.com/nodejs/node-eps/pull/18)]
- [[nodejs/diagnostics#29](https://github.com/nodejs/diagnostics/issues/29)]

@trevnorris: Do not have ability to instrument promises and it doesn’t look promising.

Follow [[nodejs/diagnostics#29](https://github.com/nodejs/diagnostics/issues/29)], create separate issues for specific items.

**Questions**:

- Do we need to deprecate `AsyncWrap::MakeCallback`? @trevnorris: eventually.
- What about the duplication of `node::MakeCallback` and `AsyncWrap::MakeCallback`? @trevnorris: There will be less duplication but some is still needed.
- User modules cannot currently be instrumented by AsyncWrap.
- How will AsyncWrap be integrated in NaN's callback wrapper?
- How will AsyncWrap be implemented in the in-progress ABI-stable API?

**Next steps**:

- One significant change remains to enable multiple listeners.
- @trevnorris is working on a JS wrapper module for AsyncWrap to make it more usable for end users.


## Tracing Controller

- [[nodejs/diagnostics#53](https://github.com/nodejs/diagnostics/issues/53)]

Some components will live in V8, some in Node. PR with V8 component: <https://codereview.chromium.org/2137013006/>

Will provide interface for configuring trace collection and serialization format. Will also provide aggregation and filtering capabilities.

Prototyped a Node-specific subclass of V8 component and able to get V8 traces. Added some trace points in `fs` to test.

@joshgav: Will there be an interface for user modules (native or JS) to read and write traces?

@ofrobots: Goal is to get basics in, then it’s up to Node.js to decide if/how to expose to users.

@mattloring: Currently based on C++ macros and included directly in code.

**Q**: What information is provided in a trace?

**A**: CPU, TID, PID, timestamp. Follow-on work planned to allow alternate arguments. Also, some TRACE_EVENT macros accept additional metadata.

**Q**: Where is output written to?

**A**: Will provide a `TraceWriter` interface which users can derive from and write to file, stream, whatever.

**Timeline**: V8 v5.4.

**Next steps**:

- Matt and Ali continue working on V8/Chromium implementation.
- Add trace points throughout Node core codebase.
- Define and implement interfaces for JS and native modules to read from and write to trace system.


## Inspector

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

@joshgav: Intent is to enable other engines to use same debugging protocol as V8 without having to implement V8's diagnostic APIs.

Also need to update CLI debugger.

Check `v8_inspector` tag in nodejs/node for needed contributions.


## APM Summit

[[nodejs/diagnostics#58](https://github.com/nodejs/diagnostics/issues/58)]


@watson: At NodeConf Adventure discussed how APM providers are solving the same problem over and over again. Idea is a shared service in a module or part of core.

Agenda would be:

- Application-level traces especially for HTTP, DB requests (I/O). Currently requires ugly monkey-patching. AsyncWrap addresses some needs but not all. Key need is to keep a context across async boundaries.

- Problem with ES modules because they can’t be monkey-patched.

@joshgav: Can application-level traces be provided through the Tracing Controller?

@ofrobots: TRACE_EVENT could be a source of information, but true need is to track async context.

@watson: Can Trace Controller be used in production?

@ofrobots: Yes, it’s designed for investigating perf issues, but it depends on number of events.

@ofrobots: Think of Tracing Controller as “structural profiling” as opposed to "sampling profiling."

Right now no user modules have been instrumented with TRACE_EVENT anyway. But eventually modules could be instrumented with same.

@watson: Provide a standard API/format for tracing so all module authors could instrument their own code.

@andreasmadsen: In what way does AsyncWrap not address this problem?

@watson: example is postgres module, which maintains a pool of callbacks. Callbacks retain context in which pool was originally created [so AsyncWrap fails].

@ofrobots: AsyncWrap helps with Node core, but the notion of a logical context doesn’t exist in JS.

Zones proposal was also an attempt to solve this problem.

**Next steps**:

- Many won’t be at Node Summit, we’ll postpone to a later conference. @watson will create a Doodle.


## Next Meeting

- Early September.