Async hooks based context tracking by matthewloring · Pull Request #538 · googleapis/cloud-trace-nodejs

matthewloring · 2017-08-10T22:56:19Z

Introduces a lower overhead, more correct context tracking
mechanism based on async hooks that is available when using the
trace agent on newer versions of node (>=8). To enable async
hooks based context, set the GCLOUD_TRACE_NEW_CONTEXT environment
variable. This change is not expected to have observable behavior
difference but due to the nature of context propagation it is
difficult to say for sure without real world testing.

matthewloring · 2017-08-14T04:25:30Z

I'm still working through the right test plan for this and I'm not sold on the way namespaces are represented in the async hook based implementation but all the high level structure is there and the tests are passing. I would be interested to get some early feedback on this, especially on the required changes to the tests (grpc required the most invasive changes).

Sign in to view

    var traceId = incomingTraceContext.traceId || (uuid.v4().split('-').join(''));
    var parentId = incomingTraceContext.spanId || '0';
+    var frameOffset;
+    if (semver.satisfies(process.version, '>=8')) {


ofrobots

Still looking.. some initial comments.

Sign in to view

 // Load continuation-local-storage first to ensure the core async APIs get
 // patched before any user-land modules get loaded.
-require('continuation-local-storage');
+if (require('semver').satisfies(process.version, '<8')) {


Sign in to view

@@ -0,0 +1,146 @@
+/**
+ * Copyright 2015 Google Inc. All Rights Reserved.


Sign in to view

      true, /* Is root span */
-      options.skipFrames ? options.skipFrames + 2 : 2);
+      options.skipFrames ?
+        options.skipFrames + ROOT_SPAN_STACK_OFFSET : ROOT_SPAN_STACK_OFFSET);


Sign in to view

+  // Monkeypatch the monkeypatcher
+  var oldIt = global.it;
+  global.it = function it(title, fn) {
+    return oldIt.call(this, title, cls.createNamespace().bind(fn));


Sign in to view

+// Core Zone Api
+
+function Context() {
+  this.properties = {};


Sign in to view

+
+Context.prototype.run = function(cb, applyThis, applyArgs) {
+  const oldContext = current;
+  current = new Context();


Sign in to view

+  }
+};
+
+Namespace.prototype.run = function(cb) {


Sign in to view

+  }
+};
+
+Namespace.prototype.runAndReturn = function(cb) {


Sign in to view

+  }
+};
+
+Namespace.prototype.bind = function(cb) {


Sign in to view

+  }
+};
+
+Namespace.prototype.runAndReturn = function(cb) {


Sign in to view

+  }
+};
+
+Namespace.prototype.bindEmitter = function(ee) {


Sign in to view

 var trace = require('../..');
 var cls = require('../../src/cls.js');
+if (semver.satisfies(process.version, '>=8')) {
+  // Monkeypatch the monkeypatcher


matthewloring · 2017-08-18T02:29:06Z

It looks like appveyor is hanging on hiredis compilation but everything else looks clean. Comments should be addressed. I think this is ready for another look. @kjin @ofrobots

Sign in to view


 env:
-  - CXX=g++-4.8
+  - CXX=g++-4.8 GCLOUD_TRACE_NEW_CONTEXT=1


Sign in to view

@@ -25,6 +25,7 @@ install:
  - ps: cd ..\..\..\..
  # set GCLOUD_PROJECT


Sign in to view

 // Load continuation-local-storage first to ensure the core async APIs get
 // patched before any user-land modules get loaded.
-require('continuation-local-storage');
+if (require('semver').satisfies(process.version, '<8') &&


Sign in to view

+  return res;
+};
+
+Namespace.prototype.runAndReturn = function(fn) {


Sign in to view

+  }
+};
+
+var n = null;


Sign in to view

+const contexts = {};
+var current = {};
+
+const hook = asyncHook.createHook({init, before, after, destroy});


Sign in to view

+const asyncHook = require('async_hooks');
+
+const wrappedSymbol = Symbol('context_wrapped');
+const contexts = {};


Sign in to view

+
+function Namespace() {}
+
+Namespace.prototype.get = function(k) {


Sign in to view

+    return cb;
+  }
+  const boundContext = current;
+  const wrapped = cb.length === 0 ? function() {


Sign in to view

+
+Namespace.prototype.bindEmitter = function(ee) {
+  for (var method of eventEmitterMethods) {
+    patchEventEmitterMethod(ee, method, this);


ofrobots

sorry.. butter fingers

matthewloring · 2017-08-21T23:10:34Z

@ofrobots All comments should be addressed. PTAL.

Sign in to view

+let contexts = {};
+let current = {};
+
+const hook = asyncHook.createHook({init, before, destroy});


Sign in to view

+function Namespace() {}
+
+Namespace.prototype.get = function(k) {
+  return current ? current[k] : undefined;


ofrobots

LGTM w/ nits.

Sign in to view

+    return cb;
+  }
+  const boundContext = current;
+  const wrapped = function contextWrapper() {


Sign in to view

+const eventEmitterMethods =
+  [ 'addListener', 'on', 'once', 'prependListener', 'prependOncelistener' ];
+
+Namespace.prototype.bindEmitter = function(ee) {


Introduces a lower overhead, more correct context tracking mechanism based on async hooks that is available when using the trace agent on newer versions of node (>=8). To enable async hooks based context, set the GCLOUD_TRACE_NEW_CONTEXT environment variable. This change is not expected to have observable behavior difference but due to the nature of context propagation it is difficult to say for sure without real world testing.

Introduces a lower overhead, more correct context tracking mechanism based on async hooks that is available when using the trace agent on newer versions of node (>=8). To enable async hooks based context, set the GCLOUD_TRACE_NEW_CONTEXT environment variable. This change is not expected to have observable behavior difference but due to the nature of context propagation it is difficult to say for sure without real world testing. PR-URL: #538

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 10, 2017

jmdobry added the in progress label Aug 10, 2017

kjin self-requested a review August 11, 2017 17:14

matthewloring requested a review from ofrobots August 12, 2017 04:35

evanlucas reviewed Aug 14, 2017

View reviewed changes

ofrobots reviewed Aug 14, 2017

View reviewed changes

kjin reviewed Aug 15, 2017

View reviewed changes

ofrobots approved these changes Aug 18, 2017

View reviewed changes

ofrobots suggested changes Aug 18, 2017

View reviewed changes

ofrobots reviewed Aug 24, 2017

View reviewed changes

ofrobots approved these changes Aug 29, 2017

View reviewed changes

matthewloring changed the title ~~[WIP]: Experiment with async hooks based context tracking~~ Async hooks based context tracking Aug 29, 2017

matthewloring merged commit 87e4254 into googleapis:master Aug 30, 2017

matthewloring deleted the cls branch August 30, 2017 02:10

jmdobry removed the in progress label Aug 30, 2017

kjin mentioned this pull request Nov 29, 2017

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

Closed

13 of 20 tasks

AndreasMadsen mentioned this pull request Dec 18, 2017

Performance impact of async_hooks nodejs/benchmarking#181

Open

		@@ -0,0 +1,146 @@
		/**
		* Copyright 2015 Google Inc. All Rights Reserved.

		@@ -25,6 +25,7 @@ install:
		- ps: cd ..\..\..\..
		# set GCLOUD_PROJECT


		function Namespace() {}

		Namespace.prototype.get = function(k) {

Conversation

matthewloring commented Aug 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matthewloring commented Aug 14, 2017

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

ofrobots left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

matthewloring commented Aug 18, 2017

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

matthewloring commented Aug 10, 2017 •

edited

Loading