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

doc: add meeting minutes 2017-03-23 #93

Closed
wants to merge 1 commit into from
Closed
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
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.

This comment was marked as off-topic.

This comment was marked as off-topic.


@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.