I'm not really in favor of adding 3 more modules in our bootstrap.

Agree, will try to cut this down :-)

Made this a little bit better.

Can you dynamically require this?

absolutely

In my opinion, trace_events_async_hooks.js should depend on trace_events.js. Making trace_events.js depend on trace_events_async_hooks.js makes the code very coupled.

I disagree. I think the dependency ordering actually makes more sense this way. Rather than burying the loading of trace_events_async_hooks.js inside bootstrap, it's loaded on demand only when tracing for that category is actually enabled.

it's loaded on demand only when tracing for that category is actually enabled.

This I agree with.

I think the dependency ordering actually makes more sense this way.

This I disagree with.

I don't see the connection between thoese two statements. To me, the current implementations corresponds to putting http.js code in events.js.

With this change, the async hook is only created and enabled when the callback within internal/trace_events.js is called and the node.async_hooks category is enabled. I'm not sure how that dependency would be worked if flipped. Can you suggest an alternative approach?

make trace_events.js an EventEmitter and make trace_events_async_hooks.js a listener.

Code not tested and didn't do any cleanup. Also didn't dynamically load trace_events_async_hooks.js. However you could do that by moving the listener to the bootstrap. It is less nice, but at least a more futureprof API.

diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js
index 66d734a5b2..e93321a777 100644
--- a/lib/internal/bootstrap/node.js
+++ b/lib/internal/bootstrap/node.js
@@ -303,10 +303,12 @@
   }
 
   function setupTraceCategoryState() {
-    const { toggleTraceCategoryState } =
-       NativeModule.require('internal/trace_events');
-    toggleTraceCategoryState();
-    _setupTraceCategoryState(toggleTraceCategoryState);
+    const traceEventsModule = NativeModule.require('internal/trace_events');
+    _setupTraceCategoryState(
+      traceEventsModule.emit.bind(traceEventsModule, 'categoryStateUpdated')
+    );
+
+    NativeModule.require('internal/trace_events_async_hooks').setup();
   }
 
   function setupProcessObject() {
diff --git a/lib/internal/trace_events.js b/lib/internal/trace_events.js
index 577738466c..6b0054c45e 100644
--- a/lib/internal/trace_events.js
+++ b/lib/internal/trace_events.js
@@ -1,26 +1,6 @@
 'use strict';
 
 const { internalBinding } = require('internal/bootstrap/loaders');
-const { traceCategoryState } = internalBinding('trace_events');
-const kCategoryAsyncHooks = 0;
-let createHook;
-let traceEventsAsyncHook;
 
-function toggleTraceCategoryState() {
-  // Dynamically enable/disable the traceEventsAsyncHook
-  const asyncHooksEnabled = !!traceCategoryState[kCategoryAsyncHooks];
-
-  if (asyncHooksEnabled && !traceEventsAsyncHook) {
-    if (!createHook)
-      createHook = require('internal/trace_events_async_hooks').createHook;
-    traceEventsAsyncHook = createHook();
-    traceEventsAsyncHook.enable();
-  } else if (!asyncHooksEnabled && traceEventsAsyncHook) {
-    traceEventsAsyncHook.disable();
-    traceEventsAsyncHook = undefined;
-  }
-}
-
-module.exports = {
-  toggleTraceCategoryState
-};
+class TraceEvents extends events.EventEmitter {}
+module.exports = new TraceEvents();
diff --git a/lib/internal/trace_events_async_hooks.js b/lib/internal/trace_events_async_hooks.js
index 0b9ffed818..bcd877fee9 100644
--- a/lib/internal/trace_events_async_hooks.js
+++ b/lib/internal/trace_events_async_hooks.js
@@ -1,7 +1,8 @@
 'use strict';
 
 const { internalBinding } = require('internal/bootstrap/loaders');
-const { trace } = internalBinding('trace_events');
+const traceEvents = require('internal/bootstrap/trace_events');
+const { trace, isTraceCategoryEnabled } = internalBinding('trace_events');
 const async_wrap = process.binding('async_wrap');
 const async_hooks = require('async_hooks');
 
@@ -64,4 +65,16 @@ function createHook() {
   });
 }
 
-module.exports = { createHook };
+const hook = createHook();
+
+function setup() {
+  traceEvents.on('categoryStateUpdated', function () {
+    if (isTraceCategoryEnabled('async_hooks')) {
+      hook.enable();
+    } else {
+      hook.disable();
+    }
+  });
+}
+
+module.exports = { setup };

Not really a fan of this approach and don't see it as much of an improvement.

A file named trace_events_async_hooks.js should logically depend on trace_events and async_hooks. I wouldn't expect a file named trace_events.js to depend on async_hooks.js. This is more than just file naming, but what is intuitive in a dependency tree.

If we generalize this to more things that are going to depend on trace_events, should we then continue to inline hooks into the trace_events.js file? Wouldn't it make more sense to being able to listen to an event, in a way that doesn't couple everything into the same function?

PS: Could you please provide arguments other than "Not really a fan of this approach".

At least make this a singleton. Meaning module.exports = createHook();

I had it that way in the original version but I've got concerns over the internal state tracking... Specifically, we end up with a memory leak if tracing is dynamically disabled and we still have items in the map that are waiting for a destroy. Doing it this way resets the internal state. Will think of another way of doing that.

@jasnell Thanks. That actually makes sense. But yeah, it is still wired it is not a singleton.

One approach we could take is to add a reset function to the async_hook instance. I could do that in a one off way or we could make it part of the async_hook API. Internally, the async_hook can call reset when disable is called.

@jasnell I think it is best to do it in a "one-off way". I would even consider just exposing a enable and a disable function, that simply relays to async_hooks and clears the state.

In general, I think async_hooks should be as state-free as possible. As it can get very heavy performance wise and is easily implemented in userland.

I can live with that :-) making the updates now, will push a new commit shortly

is the !asyncHooksEnabled && necessary in the condition here?

Yes, we only want to turn it off if that specific category is turned off

I might be missing something, but isn't asyncHooksEnabled necessarily false on the else side of branch?

Ha! Yeah you're right. I changed the if condition going in and didn't even think about that one ;)

Apologies that I took so long to do a full review. Post-landing LGTM w/ the following comment, which can be addressed as a follow-on.

AFAICT, this 0 is a hard-coded constant that appears here and also at env.cc:109. Is it possible to define this symbolically somewhere?