fix: adjust async_hooks cls behavior by kjin · Pull Request #734 · googleapis/cloud-trace-nodejs

kjin · 2018-05-02T20:40:35Z

This PR introduces the following changes to async_hooks-based tracing:

We now always use the triggerId when determining the parent AsyncResource unless the current AsyncResource is of type PROMISE.
We clear this.contexts upon disabling CLS (in practice, this happens when the Trace Agent is disabled).
Instead of sharing the value of the current context between different async scopes, we share a reference to it instead. This way, if we set context to a different value, the change is reflected in other scopes. (This makes things more consistent but I don't think it has behavioral consequences from the perspective of the user.)
- Async scopes refer to either (1) scope in the sense of AsyncResource#runInAsyncScope, or (2) the function passed to either AsyncHooksCLS#runInNewContext or AsyncHooksCLS#bindToCurrentContext.
Instead of using this.currentContext to get the current context, we now directly look up the current context in this.contexts based on the current execution ID.
- As a result, the before hook can now be removed.

Sign in to view

-    this.hook = (require('async_hooks') as AsyncHooksModule).createHook({
+    // Store a reference to the async_hooks module, since we will need to query
+    // the current AsyncResource ID often.
+    this.ah = require('async_hooks') as AsyncHooksModule;


Sign in to view


  setContext(value: Context): void {
-    this.currentContext.value = value;
+    this.contexts[this.ah.executionAsyncId()].value = value;


Sign in to view

-    const boundContext = this.currentContext.value;
+    const that = this;
+    // Capture the context of the current AsyncResource.
+    const boundContext = this.contexts[outerId];


codecov · 2018-05-02T23:06:11Z

Codecov Report

Merging #734 into master will increase coverage by 0.05%.
The diff coverage is 92.85%.

@@            Coverage Diff             @@
##           master     #734      +/-   ##
==========================================
+ Coverage   90.88%   90.93%   +0.05%     
==========================================
  Files          30       30              
  Lines        1568     1577       +9     
  Branches      304      307       +3     
==========================================
+ Hits         1425     1434       +9     
  Misses         59       59              
  Partials       84       84

Impacted Files	Coverage Δ
src/cls/async-hooks.ts	`96.36% <92.85%> (+0.71%)`	⬆️

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 d0009ff...1082eb1. Read the comment docs.

Sign in to view

-// constructor, rather than upon module load.
-import * as asyncHooksModule from 'async_hooks';
+// This file requires async_hooks in the AsyncHooksCLS constructor, rather than
+// upon module load.


ofrobots

LGTM. left some nits.

Intuitively, it didn't make sense how you could implement CLS without the before/after hooks, but after looking at the PR and pondering, I agree with you that this should be correct. As long as we can trust executionAsyncId to be correct, this code is correct. AsyncWrap does the before/after hooks to ensure that the executionAsyncId is correct. We don't need to. Nice work!

Sign in to view

-// constructor, rather than upon module load.
-import * as asyncHooksModule from 'async_hooks';
+// This file requires async_hooks in the AsyncHooksCLS constructor, rather than
+// upon module load.


Sign in to view

+        // that runs within the scope of this new AsyncResource to see the same
+        // context as its "parent" AsyncResource. The criteria for the parent
+        // depends on the type of the AsyncResource.
+        if (type === 'PROMISE') {


Sign in to view

+    return current ? current.value : this.defaultContext;
  }

  setContext(value: Context): void {


Sign in to view

-        current.value = oldContext;
+        // Revert the current context to what it was before it was set to the
+        // captured context.
+        that.contexts[id] = oldContext;


Sign in to view

+    const boundContext = this.contexts[this.ah.executionAsyncId()];
+    // Return if we have already wrapped the function, or there is no current
+    // context to bind.
+    if ((fn as ContextWrapped<Func<T>>)[WRAPPED] || !boundContext) {


kjin added 3 commits May 2, 2018 10:55

fix: adjust async_hooks cls behavior

476b2ab

test: add new tests for cls-ah

26ded3e

chore: add TODOs to rename fields on CLS class

6fa8101

googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 2, 2018

kjin requested review from DominicKramer, jinwoo and ofrobots May 2, 2018 20:49

jinwoo reviewed May 2, 2018

View reviewed changes

chore: address comments

c6232b3

jinwoo approved these changes May 2, 2018

View reviewed changes

ofrobots approved these changes May 3, 2018

View reviewed changes

kjin added 2 commits May 3, 2018 15:14

chore: address comments, pt 2

2af0060

test: improve code coverage

1082eb1

kjin merged commit 79ab435 into googleapis:master May 4, 2018

kjin mentioned this pull request Jun 13, 2018

async_hooks-based tracing doesn't work for gRPC server (in Node 8) #661

Closed

kjin mentioned this pull request Nov 7, 2018

test: remove cluster test todo #907

Merged

release-please Bot mentioned this pull request Jun 30, 2022

chore(main): release 7.0.0 #1452

Merged

Conversation

kjin commented May 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

codecov Bot commented May 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kjin commented May 2, 2018 •

edited

Loading

codecov Bot commented May 2, 2018 •

edited

Loading