Skip to content

Commit

Permalink
doc: add meeting minutes 2017-03-23
Browse files Browse the repository at this point in the history
PR-URL: #93
  • Loading branch information
joshgav committed Apr 5, 2017
1 parent e851671 commit 2c3ee9a
Showing 1 changed file with 172 additions and 0 deletions.
172 changes: 172 additions & 0 deletions wg-meetings/2017-03-23.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,172 @@
# Diag WG Meeting - March 2017

* Date: 2017-03-23
* Issue: <https://github.com/nodejs/diagnostics/issues/89>
* Recording: <https://www.youtube.com/watch?v=_-9ygz6yOBo>
* Minutes: <https://docs.google.com/document/d/1lhy1H37hsbjKljY0XYr5tt2nO5Xbo54Vf_PjBuIjvOA>
* Previous meeting: <https://docs.google.com/document/d/1Rt6yFAgCSmBFYpWDd9JL90f8BWG-AhhiJ7gOluRo-XQ>

---

## Attendees

* Thomas Watson @watson
* Richard Lau @richardlau
* Zbyszek Tenerowicz @naugtur
* Jan Krems @jkrems
* Matt Loring @matthewloring
* Ali Sheikh @ofrobots
* Eugene Ostroukhov @eugeneo
* Josh Gavant @joshgav

---

## Today’s Agenda

* inspector: make `debug` an alias for `inspect` [node#11441](https://github.com/nodejs/node/pull/11441)
* Switch the CLI debugger to V8 inspector [node#11421](https://github.com/nodejs/node/issues/11421)
* \[WIP\] inspector: hint text update [node#11207](https://github.com/nodejs/node/pull/11207)
* debug: activate inspector with \_debugProcess [node#11431](https://github.com/nodejs/node/issues/11431)
* What will Domain be replaced with? [node#10843](https://github.com/nodejs/node/issues/10843)

* \[trace\] tracking issue [diagnostics#84](https://github.com/nodejs/diagnostics/issues/84)
* \[async_hooks\] tracking issue [diagnostics#29](https://github.com/nodejs/diagnostics/issues/29)


## Previous Meeting Review

* proposal: programmatically expose the V8 inspector URL [node#11496](https://github.com/nodejs/node/issues/11496)
* inspector: make `debug` an alias for `inspect` [node#11441](https://github.com/nodejs/node/pull/11441)
* Switch the CLI debugger to V8 inspector [node#11421](https://github.com/nodejs/node/issues/11421)
* \[WIP\] inspector: hint text update [node#11207](https://github.com/nodejs/node/pull/11207)
* What will Domain be replaced with? [node#10843](https://github.com/nodejs/node/issues/10843)
* src: separate trace_event_common from trace_event [node#10628](https://github.com/nodejs/node/pull/10628)
* async_hooks initial implementation [node#8531](https://github.com/nodejs/node/pull/8531)

* wg: nominating jkrems [diagnostics#87](https://github.com/nodejs/diagnostics/pull/87)
* Request to join Diagnostics WG [diagnostics#86](https://github.com/nodejs/diagnostics/pull/86)
* \[trace\] tracking issue; out of experimental [diagnostics#84](https://github.com/nodejs/diagnostics/issues/84)
* [async_hooks] tracking issue [diagnostics#29](https://github.com/nodejs/diagnostics/issues/29)

* guides: debugging getting started guide [nodejs.org#1131](https://github.com/nodejs/nodejs.org/pull/1131)
* blog: diag wg update and --debug deprecation [nodejs.org#1156](https://github.com/nodejs/nodejs.org/pull/1156)

---

## Minutes

### inspector: make `debug` an alias for `inspect` [node#11441](https://github.com/nodejs/node/pull/11441)

* Switch the CLI debugger to V8 inspector [node#11421](https://github.com/nodejs/node/issues/11421)
* debug: activate inspector with \_debugProcess
[node#11431](https://github.com/nodejs/node/pull/11431)

To discuss:

1. Alias `node debug script.js` to `node inspect script.js`.
2. Alias `--debug` to `--inspect`.
3. Deprecate `vm.runInDebugContext` API.
4. Switch SIGUSR1 to activate Inspector.

Need to decide what to change now before 8.0.0.

#### 1. Alias `node debug script.js` to `node inspect script.js`.

Conclusion: We should alias `node debug` to `node inspect` and include a deprecation warning.

#### 2. Alias `--debug` to `--inspect`.

@joshgav: Does this depend on Node@8 including V8 5.7 or 5.8?

@jkrems: Even if Node@8 starts with V8 5.7 we should alias `--debug` to `--inspect`. That way we won’t have to support the old interface through the lifetime of Node@8.

@joshgav: Are we concerned about breaking ecosystem? Concerns about node-inspector?

@ofrobots: People have moved on to other tools.

@joshgav: Any reason to not alias `--debug` and instead remove entirely?

@jkrems: If end goal is to encourage `--inspect` might be best to just completely remove, reduce confusion. But might be unexpected to current users.

@ofrobots: Best to keep it around as an alias for a little while.

@joshgav: Propose a) making it an alias to `--inspect`, b) with a runtime deprecation warning, and c) plan to remove it in a later version entirely.

@jkrems: Any objections? A: No.

#### 3. Deprecate `vm.runInDebugContext` API.

`vm.runInDebugContext` will go away at end of 2017, so we should deprecate now in preparation for Node 10 (April 2018).

@ofrobots: Are there other use cases for this? Most common use case is `var debug = vm.runInDebugContext(‘Debug’)` to access debugging methods.

@joshgav: open a PR for the deprecation and see what turns up?

#### 4. Switch SIGUSR1 to activate Inspector.

Based on above, this needs to land in 8.0.0. Need reviews of [node#11431](https://github.com/nodejs/node/pull/11431).

**Next steps**

* @ofrobots to update [#11441](https://github.com/nodejs/node/pull/11441) to alias `--debug` to `--inspect` and `node debug` to `node inspect`, to land in 8.0.0.
* @joshgav to submit PR to deprecate `vm.runInDebugContext`.
* Please review [node#11431](https://github.com/nodejs/node/pull/11431) - switch SIGUSR1.

---

### \[WIP\] inspector: hint text update [#11207](https://github.com/nodejs/node/pull/11207)

@joshgav: Suggestion:

```
Debugger listening at ws://127.0.0.1:port/uuid
For more info go to https://nodejs.org/en/docs/guides/debugging-getting-started
```

@jkrems: Prefer just port as previously with --debug. Tools know how to handle changing UUIDs, so better not to surface UUID, would be confusing.

@joshgav: IP address can be important, for example when attaching to process in a Docker container, which doesn’t necessarily allow access to localhost. Also UUIDs are needed for lower-level tool users.

What about the `chrome-devtools` URL (e.g. `chrome-devtools://devtools/bundled/inspector.html?experiments=true&v8only=true&ws=127.0.0.1:9229/e027329e-7b98-4b7d-85f0-184a0ea24b74`)? Should that continue to be included?

@joshgav: Could Chrome DevTools accept the short `ws` URL and interpolate into the full `chrome-devtools` URL? That would also solve some of the feedback about the URL being too long to copy out of the terminal.

@ofrobots: May be security issues, `chrome-devtools` scheme has special rights.

@jkrems: Possible to add different IPs and ports in chrome://inspect now, but would need to reconfigure for all available combinations.

@joshgav: Are there strong opinions that we should keep the chrome-devtools URL?

**Next steps**

* @joshgav to open PR with suggestion above and continue discussion there.

---

### What will Domain be replaced with? [#10843](https://github.com/nodejs/node/issues/10843)

Remove from diag-agenda, perhaps close the issue.

---

### \[trace\] tracking issue [#84](https://github.com/nodejs/diagnostics/issues/84)

@matthewloring: Macros we added from V8 fell out of date with V8 so they don’t work. Will update them shortly, and would be good to also add an actual use and test case to core to know if they fall out of date.

Propose 2 PRs:
1. Update macros to get back in sync with V8.
2. Add a couple actual instrumentation points in core.

No objections.

---

### \[async_hooks\] tracking issue [#29](https://github.com/nodejs/diagnostics/issues/29)

New PR: https://github.com/nodejs/node/pull/11883.

---

## Q&A

Schedule? Okay to follow the last TSC meeting each month? No objections.

0 comments on commit 2c3ee9a

Please sign in to comment.