|
| 1 | +# Diag WG Meeting - January 2017 |
| 2 | + |
| 3 | +* Date: 2017-01-19 |
| 4 | +* YouTube: <http://youtu.be/sMkzvd9Lf74> |
| 5 | +* Hangouts on Air: |
| 6 | + <https://hangouts.google.com/hangouts/_/ytl/9unU1P2s7ypaAu9YbP-mxzklTAY1HCgDyZAiYP19S7s=?eid=100598160817214911030> |
| 7 | +* Minutes: |
| 8 | + <https://docs.google.com/document/d/146Y4TMLa0_uovREHalBsobspWSSog4N2k3TIJe_Co84> |
| 9 | +* Previous meeting: |
| 10 | + <https://github.com/nodejs/diagnostics/blob/master/wg-meetings/2016-12-02.md> |
| 11 | + |
| 12 | +## Attendees |
| 13 | + |
| 14 | +* Josh Gavant @joshgav |
| 15 | +* Matt Loring @matthewloring |
| 16 | +* Ali Sheikh @ofrobots |
| 17 | +* Mark Marron @mrkmarron |
| 18 | +* Jason Ginchereau @jasongin |
| 19 | +* Jeremiah Senkpiel @Fishrock123 |
| 20 | +* Richard Chamberlain @rnchamberlain |
| 21 | +* Richard Lau @richardlau |
| 22 | + |
| 23 | +--- |
| 24 | + |
| 25 | +## Agenda |
| 26 | + |
| 27 | +### Inspector |
| 28 | + |
| 29 | +* RFC: Support for subprocess inspection |
| 30 | + [diagnostics#77](https://github.com/nodejs/diagnostics/issues/77) |
| 31 | +* Support for HTTP/WS inspection |
| 32 | + [diagnostics#75](https://github.com/nodejs/diagnostics/issues/75) |
| 33 | +* src, lib: deprecate old debug-agent and CLI debugger |
| 34 | + [node#10276](https://github.com/nodejs/node/pull/10276) |
| 35 | +* inspector, doc: update hint text, add guide |
| 36 | + [node#8978](https://github.com/nodejs/node/pull/8978) |
| 37 | +* Bringing node-inspect into the fold |
| 38 | + [diagnostics#67](https://github.com/nodejs/diagnostics/issues/67) |
| 39 | +* Bring 'node-inspect' (CLI Debugger) under the Foundation |
| 40 | + [tsc#190](https://github.com/nodejs/TSC/issues/190) |
| 41 | +* deps: Add node-inspect [node#10187](https://github.com/nodejs/node/pull/10187) |
| 42 | +* Move bnoordhuis/node-heapdump to nodejs/node-heapdump |
| 43 | + [post-mortem#40](https://github.com/nodejs/post-mortem/issues/40) |
| 44 | + |
| 45 | +### Trace |
| 46 | + |
| 47 | +* Trace Events trace system in Node.js |
| 48 | + [diagnostics#53](https://github.com/nodejs/diagnostics/issues/53) |
| 49 | +* trace_event.h tracking issue |
| 50 | + [diagnostics#30](https://github.com/nodejs/diagnostics/issues/30) |
| 51 | +* proposal: JS tracing APIs |
| 52 | + [node-eps#48](https://github.com/nodejs/node-eps/pull/48) |
| 53 | + |
| 54 | +### Async Context |
| 55 | + |
| 56 | +* async_hooks initial implementation |
| 57 | + [node#8531](https://github.com/nodejs/node/pull/8531) |
| 58 | +* What will Domain be replaced with? |
| 59 | + [node#10843](https://github.com/nodejs/node/issues/10843) |
| 60 | + |
| 61 | +### Post-Mortem |
| 62 | + |
| 63 | +* [node-report](https://github.com/nodejs/nodereport) |
| 64 | +* [llnode](https://github.com/nodejs/llnode) |
| 65 | + |
| 66 | + |
| 67 | +--- |
| 68 | + |
| 69 | +## Previous Meeting |
| 70 | + |
| 71 | +### Trace |
| 72 | + |
| 73 | +* Intended use cases (profiling, logging, etc.) |
| 74 | +* JS API |
| 75 | +* Conventions for category names and payload fields |
| 76 | +* Trace points in core |
| 77 | + |
| 78 | +Next steps: |
| 79 | + |
| 80 | +* Add an "experimental" warning if --enable-tracing is specified. |
| 81 | +* Discuss if/how to abstract from V8. |
| 82 | +* If no objections, land initial PR (#9304). |
| 83 | +* List next tasks in Diag repo, such as instrumenting the wraps in core, adding |
| 84 | + a JS API, and adding a callback (listener) mode. |
| 85 | + |
| 86 | + |
| 87 | +### Inspector |
| 88 | + |
| 89 | +* Deprecate `node --debug` (old Debugger interface) |
| 90 | +* Deprecate CLI debugger (`node debug`) |
| 91 | +* Replace CLI debugger with node-inspect |
| 92 | +* Activate inspector in running process with SIGUSR1 |
| 93 | + |
| 94 | +Next steps: |
| 95 | + |
| 96 | +* In issue for removal of old debugger node#9789 discuss what else will be |
| 97 | + effected/deprecated. |
| 98 | +* Open a specific issue for deprecating existing CLI debugger (`lib/_debugger.js`, |
| 99 | + `node debug`). |
| 100 | +* Open an issue in the TSC repo to bring node-inspect into the Foundation. |
| 101 | +* Continue discussion on whether and how a CLI debugger should be bundled in |
| 102 | + default distribution. |
| 103 | +* Reply in node#8464 that we plan to completely switch to inspector in v8.x. |
| 104 | +* Notify client debugger projects about switch of SIGUSR1. |
| 105 | +* Message changes to community. |
| 106 | + |
| 107 | + |
| 108 | +### Async Context |
| 109 | + |
| 110 | +Next steps: |
| 111 | + |
| 112 | +* Review Embedder API added to async\_hooks and start explaining to ecosystem. |
| 113 | + |
| 114 | +--- |
| 115 | + |
| 116 | +## Minutes |
| 117 | + |
| 118 | +### Inspector |
| 119 | + |
| 120 | +### RFC: Support for subprocess inspection [diagnostics#77](https://github.com/nodejs/diagnostics/issues/77) |
| 121 | + |
| 122 | +Consensus that this is a good idea, move forward with an initial implementation. |
| 123 | + |
| 124 | +--- |
| 125 | + |
| 126 | +### Support for HTTP/WS inspection [diagnostics#75](https://github.com/nodejs/diagnostics/issues/75) |
| 127 | + |
| 128 | +* Should we support the Network domain in Inspector? |
| 129 | +* Should it go through trace\_events? |
| 130 | + |
| 131 | +@Fishrock123: Chrome DevTools UI doesn’t provide a good interface for Node’s |
| 132 | +traffic load. |
| 133 | + |
| 134 | +@joshgav: Consider from protocol perspective independent of presentation. |
| 135 | + |
| 136 | +@Fishrock123: Network connections are already (able to be) wrapped by |
| 137 | +async\_hooks, why an additional sink? |
| 138 | + |
| 139 | +@ofrobots: Capture network events through trace\_event first. That capture could |
| 140 | +be built on top of async\_hooks. |
| 141 | + |
| 142 | +@Fishrock123: For example, a native module which implements async\_hooks for |
| 143 | +network events and sends events to trace system. |
| 144 | + |
| 145 | +Do we want to also expose to Network domain in Inspector? To add to Inspector |
| 146 | +protocol would need trace listeners for TraceController, something @JasonGin has |
| 147 | +raised. |
| 148 | + |
| 149 | +@ofrobots to discuss with @eugeneo. |
| 150 | + |
| 151 | +Next steps: |
| 152 | + |
| 153 | +* Decide if/how to add trace listeners to Trace Controller. |
| 154 | +* Continue discussion in GH. |
| 155 | + |
| 156 | +--- |
| 157 | + |
| 158 | +### src, lib: deprecate old debug-agent and CLI debugger [node#10276](https://github.com/nodejs/node/pull/10276) |
| 159 | + |
| 160 | +Josh proposes to separate deprecations into 2, land debug-agent deprecation in 7.x; |
| 161 | +CLI deprecation when node-inspect is integrated. |
| 162 | + |
| 163 | +Will old debugger be gone in Node@8? Depends on whether we go with V8 5.7 or |
| 164 | +5.8. 5.8 doesn’t include old debugger. It also includes Turbofan and removes |
| 165 | +`--expose-debug-as` flag. |
| 166 | + |
| 167 | +The Diagnostics WG suggests that Node@8 use V8 5.7 so old debug interface |
| 168 | +remains available. |
| 169 | + |
| 170 | +In that case we shouldn't land runtime deprecation in a 7.x release because a |
| 171 | +runtime deprecation is semver-major and we can land it in 8.x. |
| 172 | + |
| 173 | +Get confirmation from CTC that V8 5.7 will be target for Node@8, then we can |
| 174 | +delay landing deprecation till 8.x. |
| 175 | + |
| 176 | +@jkrems (comment): Isn't our policy to deprecate for two major versions? |
| 177 | + |
| 178 | +**Next steps** |
| 179 | + |
| 180 | +* @joshgav to update runtime deprecation PR. |
| 181 | +* Ask CTC to advise on target V8 version for Node@8. |
| 182 | +* Continue discussion on whether to land a runtime deprecation in a 7.x release. |
| 183 | + |
| 184 | +--- |
| 185 | + |
| 186 | +### inspector, doc: update hint text, add guide [node#8978](https://github.com/nodejs/node/pull/8978) |
| 187 | + |
| 188 | +@joshgav to move guide to nodejs.org following guides reorganization. |
| 189 | + |
| 190 | +* Should we update the hint text? What should it be? |
| 191 | +* How do we move out of experimental state? Do we have to move out before 8.x? |
| 192 | + |
| 193 | +Since we’re deprecating the old stuff in 8, seems new stuff should be out of |
| 194 | +experimental. |
| 195 | + |
| 196 | +Need CTC vote/review to move Inspector out of experimental. |
| 197 | + |
| 198 | +@Fishrock123: Open an issue/PR tagged ctc-agenda to bring out of experimental. |
| 199 | +Can be brought out in a minor. |
| 200 | + |
| 201 | +CTC will want to verify that API (e.g. protocol, command-line flags) won’t |
| 202 | +change radically in near future. It would be okay to add APIs. |
| 203 | + |
| 204 | +Current API: https://chromedevtools.github.io/debugger-protocol-viewer/v8/ |
| 205 | + |
| 206 | +Josh to ask ChakraCore about their needs and what it might require in current |
| 207 | +implementation. |
| 208 | + |
| 209 | +Hint text: |
| 210 | +* Should include link to docs. |
| 211 | +* As long as experimental note that. |
| 212 | +* Would be nice to include links for use with CDT and others. |
| 213 | + |
| 214 | +**Next steps** |
| 215 | + |
| 216 | +* @joshgav to move guide to nodejs.org following guides reorganization. |
| 217 | +* @ofrobots to ask for CTC review to move out of experimental state preferably in 8.x. |
| 218 | +* Josh to ask ChakraCore to review. |
| 219 | +* Continue discussion on hint text in GH. |
| 220 | + |
| 221 | +--- |
| 222 | + |
| 223 | +### node-inspect |
| 224 | + |
| 225 | + * Bringing node-inspect into the fold |
| 226 | + [diagnostics#67](https://github.com/nodejs/diagnostics/issues/67) |
| 227 | + * Bring 'node-inspect' (CLI Debugger) under the Foundation |
| 228 | + [tsc#190](https://github.com/nodejs/TSC/issues/190) |
| 229 | + * deps: Add node-inspect |
| 230 | + [node#10187](https://github.com/nodejs/node/pull/10187) |
| 231 | + |
| 232 | +Do we want the `node inspect script.js` and/or `node debug script.js` |
| 233 | +experience? |
| 234 | + |
| 235 | +Yes, good to have basic debugging built in a la `node debug`. |
| 236 | + |
| 237 | +Approved by TSC and CTC to bring into Node.js Foundation, @joshgav to work with |
| 238 | +org owners and @jkrems to migrate. |
| 239 | + |
| 240 | +--- |
| 241 | + |
| 242 | +### Move bnoordhuis/node-heapdump to nodejs/node-heapdump [post-mortem#40](https://github.com/nodejs/post-mortem/issues/40) |
| 243 | + |
| 244 | +@joshgav: Suggest integrating this functionality into node-inspect since |
| 245 | +Inspector provides this functionality. |
| 246 | + |
| 247 | +@ofrobots: node-inspect may not fully address the use case here so we should |
| 248 | +consider this independently. |
| 249 | + |
| 250 | +@Fishrock123: should this be added to the V8 module in core? |
| 251 | + |
| 252 | +@joshgav: Should we instead factor V8 module out to indicate support for other |
| 253 | +VMs? |
| 254 | + |
| 255 | +@Fishrock123 - just a name, could rename. |
| 256 | + |
| 257 | +Next steps: |
| 258 | + |
| 259 | +* Continue discussion in GH. |
| 260 | + |
| 261 | +--- |
| 262 | + |
| 263 | +### Trace |
| 264 | + |
| 265 | +* Tracking issues |
| 266 | + |
| 267 | + * Trace Events trace system in Node.js |
| 268 | + [diagnostics#53](https://github.com/nodejs/diagnostics/issues/53) |
| 269 | + * trace_event.h tracking issue |
| 270 | + [diagnostics#30](https://github.com/nodejs/diagnostics/issues/30) |
| 271 | + |
| 272 | +@joshgav to close existing issues and open new ones to track ongoing work on the |
| 273 | +following: |
| 274 | + |
| 275 | +* add listen capability |
| 276 | +* add JS API |
| 277 | +* add trace points to core |
| 278 | +* add C++ API over macros? |
| 279 | +* conventions for category names, payloads |
| 280 | +* what is needed to land in a release, and as non-experimental? |
| 281 | + |
| 282 | +--- |
| 283 | + |
| 284 | +### proposal: JS tracing APIs [node-eps#48](https://github.com/nodejs/node-eps/pull/48) |
| 285 | + |
| 286 | +Purpose is not just for async, but for any perf diagnostics - counters, instant |
| 287 | +operations, begin/end for synchronous operations, etc. |
| 288 | + |
| 289 | +Allows JS module authors to expose tracing data from their modules. |
| 290 | + |
| 291 | +async\_hooks is only for tracking async context. |
| 292 | + |
| 293 | +In EP, seeking feedback on what should go in core or not? |
| 294 | + |
| 295 | +**Next steps** |
| 296 | + |
| 297 | +* Read doc and provide feedback to Jason. |
| 298 | + |
| 299 | +--- |
| 300 | + |
| 301 | +### Async Context |
| 302 | + |
| 303 | +* async_hooks initial implementation |
| 304 | + [node#8531](https://github.com/nodejs/node/pull/8531) |
| 305 | +* What will Domain be replaced with? |
| 306 | + [node#10843](https://github.com/nodejs/node/issues/10843) |
| 307 | + |
| 308 | +* docs and tests for async_hooks to be added to PR soon |
| 309 | + |
| 310 | +@AndreasMadsen (comment): |
| 311 | + |
| 312 | +A while ago Trevor said the following were missing: |
| 313 | + |
| 314 | +* Remove all weak objects (will be handled in another PR to solve another issue) |
| 315 | +* Add support to node::MakeCallback |
| 316 | +* Implement public native API. |
| 317 | +* Detect a change in the hooks queue when in the middle of running hooks, so the |
| 318 | + original set of hooks can finish executing. |
| 319 | + |
| 320 | +My best guess is that only "Add support to node::MakeCallback" and "Implement |
| 321 | +public native API" is missing now, but that he wants to test the current |
| 322 | +implementation first. |
| 323 | + |
| 324 | +--- |
| 325 | + |
| 326 | +### Post-Mortem |
| 327 | + |
| 328 | +* [node-report](https://github.com/nodejs/nodereport) |
| 329 | + |
| 330 | +Human-readable “dump.” |
| 331 | + |
| 332 | +* [llnode](https://github.com/nodejs/llnode) |
| 333 | + |
| 334 | +MDB story for core dumps on Linux and Mac. |
| 335 | + |
| 336 | +--- |
| 337 | + |
| 338 | +### Next |
| 339 | + |
| 340 | +* Create a label for WG agenda. |
| 341 | +* Create a Doodle for next meeting in a few weeks. |
| 342 | + |
0 commit comments