Use async_hooks for Node.js 8.2.0 and above #77

watson · 2017-11-24T14:02:11Z

Todo:

Investigate if it's worth it filtering out TIMERWRAP - See Use async_hooks for Node.js 8.2.0 and above #77 (comment)
Investigate if we should add a hook for promiseResolve as well
~~Benchmark the difference in overhead between this solution and the monkey-patching solution in use pre-v8.2.0~~

watson · 2017-11-24T15:41:57Z

lib/instrumentation/async-hooks.js

-      }
-    }
+  function before (asyncId) {
+    if (!initState.has(asyncId)) return // in case type === TIMERWRAP


We need to consider if the benefit of filtering out TIMERWRAP types outweigh the overhead of these has() calls

watson · 2017-11-29T20:31:37Z

Concern: We'd probably lose context if using AsyncHooks if a call goes through a native module that isn't using n-api. There's work going on to get AsyncHooks to work with Nan in the Node.js Diagnostic WG, but it's far from ready.

watson · 2017-12-13T14:53:38Z

@Qard I just took a look at WeakMap and as far as I can see we can't use it as the weak part is the key and not the value, and so the key needs to be an object (asyncId is a number):

The WeakMap object is a collection of key/value pairs in which the keys are weakly referenced. The keys must be objects and the values can be arbitrary values.

Qard · 2017-12-13T17:25:53Z

Ah, right...maybe we can just do some testing around the clear* functions to make sure the references are getting cleared from the maps properly?

watson · 2017-12-13T18:26:57Z

Concern: We'd probably lose context if using AsyncHooks if a call goes through a native module that isn't using n-api. There's work going on to get AsyncHooks to work with Nan in the Node.js Diagnostic WG, but it's far from ready.

On second thought, this really doesn't matter as it's the same issue with our monkey patched version of the code.

codecov-io · 2017-12-13T20:53:24Z

Codecov Report

Merging #77 into master will decrease coverage by 1.96%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #77      +/-   ##
==========================================
- Coverage   82.51%   80.55%   -1.97%     
==========================================
  Files          38       39       +1     
  Lines        1922     1939      +17     
==========================================
- Hits         1586     1562      -24     
- Misses        336      377      +41

Impacted Files	Coverage Δ
lib/config.js	`95.55% <ø> (ø)`	⬆️
lib/instrumentation/index.js	`90.9% <100%> (+0.36%)`	⬆️
lib/instrumentation/async-hooks.js	`100% <100%> (ø)`
lib/instrumentation/es6-wrapped-promise.js	`14.28% <0%> (-78.58%)`	⬇️
lib/instrumentation/patch-async.js	`48.98% <0%> (-12.15%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eee12ef...6a384ff. Read the comment docs.

watson · 2017-12-19T12:18:24Z

lib/instrumentation/async-hooks.js

+  }
+
+  function destroy (asyncId) {
+    if (!transactions.has(asyncId)) return // in case type === TIMERWRAP


I imagine leaving this up to the Map.prototype.delete function is just as efficient

Same as get, this may deopt if it tries to delete an item that doesn't exist.

Fixes elastic#75

Qard · 2017-12-21T21:12:52Z

lib/instrumentation/async-hooks.js

+  Object.defineProperty(ins, 'currentTransaction', {
+    get: function () {
+      const asyncId = asyncHooks.executionAsyncId()
+      return transactions.get(asyncId)


Not sure if it's still the case, but previously map.get(...) could deopt if the item did not exist. It might be a good idea to put a if (!transactions.has(asyncId)) return before the get.

Qard · 2017-12-21T21:13:58Z

lib/instrumentation/async-hooks.js

+  }
+
+  function destroy (asyncId) {
+    if (!transactions.has(asyncId)) return // in case type === TIMERWRAP


Same as get, this may deopt if it tries to delete an item that doesn't exist.

Qard · 2017-12-21T22:46:25Z

Looks like async_hooks.executionAsyncId() was actually not added until 8.2.

watson · 2017-12-22T00:10:27Z

@Qard Ah good catch regarding v8.2. I'll update the version requirement 😃

watson · 2017-12-23T10:04:31Z

Here's how the new Compatibility top section looks:

And here's how the new section in the Agent API docs looks:

watson · 2017-12-23T10:07:37Z

@Qard I think this is ready now. If you're ok with it I'll merge it into master and release a new version to npm

watson added the in progress label Nov 24, 2017

watson force-pushed the asynchooks branch 4 times, most recently from 16572d5 to 75a6d05 Compare November 24, 2017 15:16

watson commented Nov 24, 2017

View reviewed changes

watson force-pushed the asynchooks branch 4 times, most recently from 6ee9f1c to b55231e Compare November 27, 2017 13:26

jkrems mentioned this pull request Nov 29, 2017

[async_hooks] stable API - tracking issue nodejs/diagnostics#124

Closed

20 tasks

watson force-pushed the asynchooks branch 4 times, most recently from 13fb8c8 to 917b181 Compare December 13, 2017 20:03

AndreasMadsen mentioned this pull request Dec 18, 2017

Performance impact of async_hooks nodejs/benchmarking#181

Open

watson commented Dec 19, 2017

View reviewed changes

watson added 2 commits December 21, 2017 15:14

refactor(*): use async_hooks for Node.js 8.1.0 and above

5ec35f3

Fixes elastic#75

Simplify async_hooks logic

60e2bf7

watson force-pushed the asynchooks branch from 74364a2 to 60e2bf7 Compare December 21, 2017 14:18

Qard suggested changes Dec 21, 2017

View reviewed changes

watson added 3 commits December 23, 2017 10:38

default to async hooks on

63b4be0

Address PR comments

74552f1

update docs

baff5e0

watson changed the title ~~Use async_hooks for Node.js 8.1.0 and above~~ Use async_hooks for Node.js 8.2.0 and above Dec 23, 2017

watson added 2 commits December 23, 2017 10:41

fix travis

db9b120

fix tests

6a384ff

Qard approved these changes Dec 27, 2017

View reviewed changes

watson removed the in progress label Jan 2, 2018

watson merged commit ab5d6cd into elastic:master Jan 2, 2018

watson deleted the asynchooks branch January 2, 2018 15:53

watson mentioned this pull request Jan 8, 2018

Node.js 8+ compatibility issues #75

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use async_hooks for Node.js 8.2.0 and above #77

Use async_hooks for Node.js 8.2.0 and above #77

watson commented Nov 24, 2017 •

edited

Loading

watson Nov 24, 2017

watson commented Nov 29, 2017

watson commented Dec 13, 2017

Qard commented Dec 13, 2017

watson commented Dec 13, 2017

codecov-io commented Dec 13, 2017 •

edited

Loading

watson Dec 19, 2017

Qard Dec 21, 2017 •

edited

Loading

Qard Dec 21, 2017

Qard Dec 21, 2017 •

edited

Loading

Qard commented Dec 21, 2017

watson commented Dec 22, 2017

watson commented Dec 23, 2017

watson commented Dec 23, 2017

Use async_hooks for Node.js 8.2.0 and above #77

Use async_hooks for Node.js 8.2.0 and above #77

Conversation

watson commented Nov 24, 2017 • edited Loading

watson Nov 24, 2017

Choose a reason for hiding this comment

watson commented Nov 29, 2017

watson commented Dec 13, 2017

Qard commented Dec 13, 2017

watson commented Dec 13, 2017

codecov-io commented Dec 13, 2017 • edited Loading

Codecov Report

watson Dec 19, 2017

Choose a reason for hiding this comment

Qard Dec 21, 2017 • edited Loading

Choose a reason for hiding this comment

Qard Dec 21, 2017

Choose a reason for hiding this comment

Qard Dec 21, 2017 • edited Loading

Choose a reason for hiding this comment

Qard commented Dec 21, 2017

watson commented Dec 22, 2017

watson commented Dec 23, 2017

watson commented Dec 23, 2017

watson commented Nov 24, 2017 •

edited

Loading

codecov-io commented Dec 13, 2017 •

edited

Loading

Qard Dec 21, 2017 •

edited

Loading

Qard Dec 21, 2017 •

edited

Loading