search

nodejs / diagnostics

Node.js Diagnostics Working Group
MIT License
537 stars 78 forks source link

Bringing node-inspect into the fold #67

Closed jkrems closed 7 years ago

jkrems commented 8 years ago

EDIT: Please refer to https://github.com/nodejs/node/issues/11421 for the latest status.

This issue tracks outstanding questions & tasks that have to be resolved to bring node-inspect into the nodejs org.

Context & History

mhdawson commented 8 years ago

one more step to add at the end is to open an issue in the TSC repo requesting the ok for the move.

mhdawson commented 8 years ago

I think the diagnostics WG repo should have a GOVERNANCE.md and CONTRIBUTING.md as all repos should. @joshgav is that something you can look at fixing ?

joshgav commented 8 years ago

Thanks @jkrems! I hope to look over soon and help with questions.

@mhdawson - yes absolutely, will copy from some other repos shortly. Thanks!

gibfahn commented 8 years ago

Regarding using tap (by which I assume you mean https://www.npmjs.com/package/tap), we already use it for the npm tests, and I think there are possibly plans to use it more for other things, so I'm pretty sure that should be fine.

MylesBorins commented 8 years ago

@jkrems as a standalone repo there is no problem with using tap... we use it for citgm :D

gibfahn commented 8 years ago

For linting, I personally like standardjs, but it might be better to keep consistency with the other node repos (so people coming from other areas within the nodejs org don't have to keep track of multiple linting styles).

jkrems commented 8 years ago

I think the current lint settings match what node core is doing for the most part. I couldn't find a published version of the node core config and copy&paste of the file(s) didn't seem like an improvement over the current situation. :)

Updated the implementation, filling some of the last gaps & adding tap tests. node-inspect@1.7.0 is published to npm if you want to play around with it.

cjihrig commented 8 years ago

I couldn't find a published version of the node core config

I know @geek published https://www.npmjs.com/package/node-style. It looks like it's probably stale, but could be updated if you wanted to use it.

Fishrock123 commented 8 years ago

shouldn't uh, this just land in core itself if it is to replace node debug?

jkrems commented 8 years ago

@Fishrock123 The feedback I got when I proposed that was:

This is definitely interesting but I'm wondering if it would be better to develop this as a standalone client for the time being. Once it's functional, then we can look to see how much sense it makes to bringing it in to core.

See: https://github.com/nodejs/node-eps/pull/42#issuecomment-245664873

ofrobots commented 8 years ago

I think this should be in core. Now that V8_inspector debug protocol has landed in V8 directly, the old debug protocol (that has been deprecated for a long while) is likely to be removed soon-ish. This project would be needed in core at that point or we will have to deprecate/remove node debug from core. Might as well put it in core now.

jkrems commented 8 years ago

I'd be happy to unblock anyone who would want to port the current code to node core. The license already is the same (it's a derived work of a node core file) and the code has no external dependencies. The biggest task would be to port the test suite but that should hopefully be relatively straight-forward. :)

jkrems commented 8 years ago

One argument for it landing in node core itself in some shape or form: node 6.9 is the second 6.x release in a row that breaks it (because details of the protocol / expectations changed).

Fishrock123 commented 8 years ago

Hmmm ok, read through the other threads. Could you describe a bit what benefit this may have in the foundation?

jkrems commented 8 years ago

@Fishrock123 To clarify your question: Benefit of having an officially supported CLI debugger or benefit of keeping it in its own repository?

I don't really have a good answer for the former (other than "It's the status quo and the people using it might dislike the removal").

If you were asking for "benefits of a separate repo": I think others are better equipped to answer that question. From the transcript it looks like the reason came down to "if there's no good reason to have it in core, don't put it in core":

@mhdawson: Why does this need to be in nodejs/node?

@jkrems: In theory it’s similar to readable-stream and doesn’t require anything in nodejs/node.

@mhdawson: nodereport isn’t in core, but we made it a project in Node.js. Should we do the same in this case?

No opposition.

Having it as a separate repo would not prevent us from bringing it into core in the future. Potentially it would even allow us to have development in a separate repo and roll new releases into core from time to time. That could allow relatively fast development of the CLI debugger under the stewardship of the diagnostics WG with new releases available via npm immediately. The precedent would be readable-stream and npm, both of which ship with core but are also available stand-alone.

I'm going to defer further work until there's agreement on the path forward. :)

jkrems commented 8 years ago

Published node-inspect@v1.9.1 which works with the latest version of node 6, exposes Debugger.* and Runtime.* directly in the repl, and doesn't crash when you run exec console.profile() (but doesn't store the profiles anywhere yet).

jkrems commented 8 years ago

Just gave it a quick try for verification purposes: Integrating the current code into node itself takes 5 lines.

> ./node inspect test/fixtures/empty.js
< Debugger listening on port 9229.
< Warning: This is an experimental feature and could change at any time.
< To start debugging, open the following URL in Chrome:
<     chrome-devtools://devtools/bundled/inspector.html?experiments=true&v8only=true&ws=127.0.0.1:9229/9fcdb074-adfe-4beb-bad6-18948fcf0a87
< Debugger attached.
break in test/fixtures/empty.js:2
  1 (function (exports, require, module, __filename, __dirname) {
> 2 });
debug>
jkrems commented 8 years ago

Let me know if you need anything else from my side. :)

bnoordhuis commented 8 years ago

Is there a CTC or TSC champion for this?

joshgav commented 8 years ago

@bnoordhuis @jkrems I've been reviewing and testing this and can help with logistics and some more review and updates. Still would be ideal to have a CTC member involved.

Although the CLI Debugger was integrated in core in the past, perhaps we should consider decoupling it at this inflection point, and maintaining it in a separate repo. That would allow it to get more attention I think, and make it less risky to add and innovate in it. For example, I've been thinking of adding some sort of "profile" command which could run a brief CPU profile and dump the output.

Also, the CLI debugger could potentially be opened against a web page or other implementor of the Inspector Protocol besides Node.

Even if it isn't in the core repo, we could bundle it with the default distribution.

Trott commented 8 years ago

Talked with a handful of folks at Node Interactive today about this. Apart from the important issue of bringing node-inspect into the nodejs org, is this a correct summary of the status?:

jkrems commented 8 years ago

@Trott Happy to chat in person if you're at node interactive as well!

Trott commented 8 years ago

@jkrems Yes, please. That would be great. I'm sitting quietly outside the empty conference rooms right now, if after-parties aren't your thing either. Otherwise, tomorrow some time?

jkrems commented 8 years ago

Same! Going to head down to the 4th floor.

jkrems commented 7 years ago

Updated the issue description to be more reflective of where we're at. @joshgav or @Trott: Are we tracking the overall project ("prepare for --debug removal") somewhere? I know some of it is captured in meeting notes and comments like this one but it might be good to have this is as... a dedicated issue? As part of this issue's TODO checkboxes? A Github project? Had to dig through multiple links & documents to remember half of what we discussed. :)

Trott commented 7 years ago

@jkrems I don't think it's being tracked centrally anywhere. Not sure what the best way to track it would be. I'm inclined to think a checklist in this issue would be workable, but @joshgav may be more plugged in to how to manage these sorts of things effectively, since not-letting-things-slip-through-the-cracks-on-collaborative-projects is part of his actual job (I think).

bnoordhuis commented 7 years ago

I'd open a tracking issue at nodejs/node. It affects core and an issue on the main bug tracker has better visibility (in theory anyway... it can be a black hole sometimes.)

joshgav commented 7 years ago

@jkrems @trott I did indeed contemplate setting up a GitHub project to gather the various discussions on this topic and can look into it again. In fact though, Jan's summary at the top of this thread is a great start already, perhaps we can use it as the main tracking point; we all should have edit access to it.

jkrems commented 7 years ago

The reason I punted on moving to an issue in nodejs/node is that there already is one with valuable discussion (https://github.com/nodejs/node/issues/7266) so it would feel like creating a duplicate. And for that issue I couldn't make the TODOs as easily visible (not an owner of nodejs/node and also it would feel weird to hijack @ofrobots' issue like that ^^).

jkrems commented 7 years ago

@joshgav Follow-up to the "do we have the old debugger in node 8" question: What would this mean for the kill signal? Would we still change it to start --inspect in node 8? Would we delay the switch until node 9?

ofrobots commented 7 years ago

it would feel weird to hijack @ofrobots' issue like that ^^

It would be good to have a single tracking issue for debug/inspect work. https://github.com/nodejs/node/issues/7266 seems to be pretty stale IMO, so it would be fine to close it and reference it in a new tracking issue on nodejs/node.

From my point of view, from what I have been able to determine from various issues, the next steps are something like the following:

  1. Make debug an alias for inspect. This is circularly dependent on the next item. I have started working on this. EDIT: WIP branch here: https://github.com/ofrobots/node/tree/debug-alias. I need to work through all the failing tests (help is most welcome).
  2. Switch SIGUSR1 to start inspector. @eugeneo has is working on this and has a WIP branch here: https://github.com/eugeneo/node/commits/inspector-signal. This would be dependent on the old debugger moving out of the way. (https://github.com/nodejs/node/issues/8464). Debugging a pid should just work once these first two are implemented.
  3. Add a JS API to allow in-process debuggers to work. This would be logical equivalent of accessing the Debug API from the debug context. (@eugeneo and @cristiancavalli may have some code for this already).
  4. node-inspect doesn't currently work well with cluster. The problem is that we don't have a way of communicating the child process' inspect port to node-inspect. process.debugPort currently corresponds to the old debugger and there is no equivalent for inspector. Once we make debug an alias for inspect, this can be fixed.
jkrems commented 7 years ago

Closing in favor of https://github.com/nodejs/node/issues/11421 in nodejs/node.

@ofrobots I tried to incorporate the points you brought up in the updated checklist. Feel free to edit/amend/editorialize. :)

cristiancavalli commented 7 years ago

Too @ofrobots third point (jic anyone's interested) here's the scaffolded prototype implementation of the JS wrapper to the inspector API for in-process communication: https://github.com/cristiancavalli/in-process-inspector-wrapper

joshgav commented 7 years ago

We should probably have a tracking issue for finishing up Inspector work generally, perhaps we can use the thread @jkrems started?

jkrems commented 7 years ago

@joshgav Happy to turn https://github.com/nodejs/node/issues/11421 into a generic "inspector stuff" tracking issue. In practice it's hard to decouple the two concerns anyhow.