docs: async-hooks documentation by thlorenz · Pull Request #2 · thlorenz/node

thlorenz · 2016-11-28T12:09:31Z

WIP PR to get Feedback.

@rvagg @Fishrock123 @trevnorris

trevnorris

Docs will need to be word wrapped before going into core. I'll let you decide when to take care of that.

trevnorris · 2016-12-01T23:32:29Z

doc/api/async_hooks.md

nit: the value inside of the {} is usually capitalized in core docs at least.

cool will fix

trevnorris · 2016-12-01T23:33:12Z

doc/api/async_hooks.md

parentUid -> triggerId

trevnorris · 2016-12-01T23:33:57Z

doc/api/async_hooks.md

thought: should we automatically stop listening in the 'exit' event?

I'd opt for not doing any magic here. It doesn't matter much anyways since the process is exiting ...

trevnorris · 2016-12-01T23:35:16Z

doc/api/async_hooks.md

This should either be asyncHook.enable() or AsyncHook#enable(). Like this it looks like a static member (i.e. not one on the prototype).

which one of these two is more common given this is a module method? cc @Fishrock123

trevnorris · 2016-12-01T23:36:00Z

doc/api/async_hooks.md

nit: "current execution context". remove a hair more ambiguity.

trevnorris · 2016-12-01T23:37:29Z

doc/api/async_hooks.md

The preferred embedder API is to use AsyncEvent. The async_hooks.emitInit(), etc., were made for internals, but I could find reasonable need for them in user code. But want to make clear they should attempt to use AsyncEvent first, if possible.

OK will dig into that a bit more AsyncEvent and update the docs as far as I'll understand things at that point ...

Fishrock123 · 2017-01-05T15:03:13Z

doc/api/async_hooks.md

Needs a terminology section: https://github.com/nodejs/node-eps/blob/e35d4813fdbbd99a751296e24361dba0d0dd9e10/XXX-asyncwrap-api.md#terminology

The top-level description should probably not include the word "handle".

Fishrock123

So, al the Node.js docs follow some order:

All sections and content within them must be alphabetical, the exception being if you have API and subheadings in the same spot, then the API references must be alphabetical but the sub-heading should usually go last.

Also, we tend to put examples after API references, except in the case of a top-level example.

thlorenz · 2017-01-17T20:35:56Z

@Fishrock123 @trevnorris, ready for another review. Fixed a bunch of nits and integrated big parts from the EPS

Most now comes from this EPS, except I reordered things a bit to clearly separate the different parts of the API.
I also removed some parts that described implementation details not useful to the users.

I also removed the original example I had in there, as now lots of examples are already included in numerous places.

LMK what you think at this point.

Fishrock123

I think it would also be good to have a section explaining that you must sue process._rawDebug() to log within a look so as to prevent an infinite loop.

Fishrock123 · 2017-01-26T16:28:52Z

doc/api/async_hooks.md

extra spaces?

could you clarify? Missing spaces, need to add them and where?

Fishrock123 · 2017-01-26T16:31:45Z

doc/api/async_hooks.md

tasks, such as -- the last sentence probably shouldn't be it's own.

OK will fix.

Fishrock123 · 2017-01-26T16:44:12Z

doc/api/async_hooks.md

You mean retrieving the handle if the user has stored it?

Should probably be id instead of handle .. I agree it's confusing.
This came from the EP (slightly adapted) so @trevnorris should know.

... retrieving the handle of the current trigger

That comment dates back to when I had implemented a map to keep track of all resources, and there was async_hooks.getResourceById(). Which would return the resource from the map, but it ended up being too costly. So it was removed.

Fishrock123 · 2017-01-26T16:44:58Z

doc/api/async_hooks.md

a new AsyncHook instance.

Hairsplitting, but I've got no strong opinion, so will fix.

Fishrock123 · 2017-01-26T17:25:04Z

doc/api/async_hooks.md

It will actually prevent those

Need clarification from @trevnorris here.

I updated the EP to simply say:

// Disable listening for all new asynchronous events.

Fishrock123 · 2017-01-26T17:48:17Z

doc/api/async_hooks.md

returned from `currentId()`

Fishrock123 · 2017-01-26T17:49:22Z

doc/api/async_hooks.md

Perhaps we want to put in a thing about that I/O pools need to use this?

Need more details on that or maybe you or @trevnorris can suggest the addition here?

@Fishrock123 can you clarify what you mean?

Fishrock123 · 2017-01-26T17:50:13Z

doc/api/async_hooks.md

Perhaps this comment should go under the actual api header with an example?

Yeah, makes sense. Will fix.

Fishrock123 · 2017-01-26T17:50:44Z

doc/api/async_hooks.md

The argument names are supposed to be surrounded in backticks. e.g. `type`. Please change throughout the document.

Fishrock123 · 2017-01-26T17:51:29Z

doc/api/async_hooks.md

Perhaps we should say it should not be called or an example given?

Not fully sure what you mean. What should not be called?

trevnorris

Thanks for the work. I like the added example of using the AsyncEvent API.

trevnorris · 2017-01-27T23:31:48Z

doc/api/async_hooks.md

I believe the initial release will be labeled 1 - Experimental so that we have a major version to make any necessary tweaks.

trevnorris · 2017-01-27T23:33:13Z

doc/api/async_hooks.md

This doesn't make much sense to me. The absolute simplest would be to log metadata about asynchronous resources when they occur.

Removed the paragraph

trevnorris · 2017-01-27T23:35:56Z

doc/api/async_hooks.md

... retrieving the handle of the current trigger

That comment dates back to when I had implemented a map to keep track of all resources, and there was async_hooks.getResourceById(). Which would return the resource from the map, but it ended up being too costly. So it was removed.

trevnorris · 2017-01-27T23:36:49Z

doc/api/async_hooks.md

I updated the EP to simply say:

// Disable listening for all new asynchronous events.

trevnorris · 2017-01-27T23:38:47Z

doc/api/async_hooks.md

If that is removed I'd suggest explicitly mentioning all four callbacks. e.g.

The callbacks init()/before()/after()/destroy() are registered via...

trevnorris · 2017-01-27T23:44:06Z

doc/api/async_hooks.md

Did you add that console.*() should never be used from hook callbacks? May need to mention this.

trevnorris · 2017-01-27T23:45:08Z

doc/api/async_hooks.md

The script above, and this output, were both updated in the EP. Please reference.

Updated the output .. didn't see any changes to the script.
Note that we're using class like function declarations (instead of arrow functions) as per request by @Fishrock123

trevnorris · 2017-01-27T23:47:19Z

doc/api/async_hooks.md

It's anything that uses MakeWeak() in C++. Can throw that list together if needed.

trevnorris · 2017-01-27T23:48:58Z

doc/api/async_hooks.md

@Fishrock123 can you clarify what you mean?

trevnorris · 2017-01-27T23:50:33Z

doc/api/async_hooks.md

The argument names are supposed to be surrounded in backticks. e.g. `type`. Please change throughout the document.

thlorenz · 2017-01-30T17:41:57Z

@Fishrock123 @trevnorris addressed the nits.

Still need input/clarification on the below

thlorenz · 2017-02-07T17:16:14Z

So I fixed the nits pointed out by Trevor, but still need some clarification from @Fishrock123 for some of the above.

brycebaril · 2017-03-31T16:37:56Z

doc/api/async_hooks.md

+TTYWRAP(6) -> Timeout(4) -> TIMERWRAP(5) -> TickObject(3) -> root(1)
+```
+
+The `TCPWRAP` isn't part of this graph; evne though it was the reason for


s/evne/even/

brycebaril · 2017-03-31T16:38:27Z

doc/api/async_hooks.md

+```js
+const async_hooks = require('async_hooks');
+
+// Return the id of the current execution context.


Probably worth defining 'execution context' somewhere.

brycebaril · 2017-03-31T16:41:09Z

doc/api/async_hooks.md

+hostname is actually synchronous, but to maintain a completely asynchronous API
+the user's callback is placed in a `process.nextTick()`.
+
+The graph only shows **when** a resource was created. Not **why**. So to track


Not why. So to track the why use triggerId.

This is a bit awkward

brycebaril · 2017-03-31T16:51:54Z

doc/api/async_hooks.md

+    event
+* Returns {AsyncEvent} A reference to `asyncHook`.
+
+Example usage:


This seems less like an embedder's use but an APM/wrapper's use. I'd think that the embedder would put the emitBefore and emitAfterinsidefunction this.db.getnot thegetInfo` wrapper?

thlorenz · 2017-05-10T21:18:34Z

@brycebaril let's continue discussion here: nodejs#12953

drewfish · 2017-06-02T13:37:58Z

doc/api/async_hooks.md

+const async_hooks = require('async_hooks');
+
+// Return new unique id for a constructing handle.
+const id = async_hooks.newUid();


Should this be newId()?

trevnorris reviewed Dec 1, 2016

View reviewed changes

thlorenz force-pushed the trevnorris-async-wrap-eps-impl+docs branch from cb4c5f2 to 8745613 Compare January 4, 2017 15:16

thlorenz force-pushed the trevnorris-async-wrap-eps-impl branch from f068849 to 620fb61 Compare January 5, 2017 14:27

Fishrock123 suggested changes Jan 5, 2017

View reviewed changes

thlorenz force-pushed the trevnorris-async-wrap-eps-impl+docs branch from 8745613 to 13c8190 Compare January 17, 2017 16:01

thlorenz force-pushed the trevnorris-async-wrap-eps-impl branch 4 times, most recently from 97194bd to 7dae377 Compare January 25, 2017 21:22

thlorenz force-pushed the trevnorris-async-wrap-eps-impl+docs branch from 175108c to df8e8e5 Compare January 26, 2017 16:28

Fishrock123 reviewed Jan 26, 2017

View reviewed changes

thlorenz force-pushed the trevnorris-async-wrap-eps-impl+docs branch from df8e8e5 to b3aed32 Compare January 26, 2017 19:33

thlorenz force-pushed the trevnorris-async-wrap-eps-impl branch from 33c2eb5 to 97a27f1 Compare January 27, 2017 15:29

thlorenz force-pushed the trevnorris-async-wrap-eps-impl+docs branch from b3aed32 to dc4833d Compare January 27, 2017 15:30

trevnorris suggested changes Jan 27, 2017

View reviewed changes

thlorenz force-pushed the trevnorris-async-wrap-eps-impl branch from 97a27f1 to 79b11ce Compare February 1, 2017 15:15

thlorenz force-pushed the trevnorris-async-wrap-eps-impl+docs branch from 17edc94 to e5193f7 Compare February 1, 2017 15:16

thlorenz force-pushed the trevnorris-async-wrap-eps-impl branch from 79b11ce to 1ff2dfb Compare February 2, 2017 15:44

docs: async-hooks documentation

4dac48a

thlorenz force-pushed the trevnorris-async-wrap-eps-impl+docs branch from e5193f7 to 4dac48a Compare February 2, 2017 15:50

docs: addressing nits pointed out by Trevor

5d7a544

brycebaril reviewed Mar 31, 2017

View reviewed changes

thlorenz mentioned this pull request May 22, 2017

docs: documenting async hooks features nodejs/node#12953

Closed

2 tasks

drewfish reviewed Jun 2, 2017

View reviewed changes

thlorenz closed this Oct 27, 2017

Conversation

thlorenz commented Nov 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trevnorris left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fishrock123 left a comment

Choose a reason for hiding this comment

Uh oh!

thlorenz commented Jan 17, 2017

Uh oh!

Fishrock123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

thlorenz commented Nov 28, 2016 •

edited

Loading