fix incorrect metrics for async resources (#604)

DataDog · Jun 27, 2019 · 22b7221 · 22b7221
1 parent c0748ff
commit 22b7221
Show file tree

Hide file tree

Showing 3 changed files with 50 additions and 25 deletions.
diff --git a/packages/dd-trace/src/scope/async_hooks.js b/packages/dd-trace/src/scope/async_hooks.js
@@ -27,7 +27,7 @@ class Scope extends Base {
       before: this._before.bind(this),
       after: this._after.bind(this),
       destroy: this._destroy.bind(this),
-      promiseResolve: this._destroy.bind(this)
+      promiseResolve: this._promiseResolve.bind(this)
     })
 
     this._hook.enable()
@@ -80,16 +80,31 @@ class Scope extends Base {
     }
   }
 
-  _init (asyncId, type, triggerAsyncId, resource) {
+  _add (asyncId, type) {
     const span = this._active()
 
+    this._ref(span, asyncId)
+
     this._spans[asyncId] = span
     this._types[asyncId] = type
+  }
 
-    this._ref(span, asyncId)
+  _delete (asyncId) {
+    const span = this._spans[asyncId]
+
+    this._unref(span, asyncId)
+
+    delete this._spans[asyncId]
+    delete this._types[asyncId]
+  }
+
+  _init (asyncId, type, triggerAsyncId, resource) {
+    this._add(asyncId, type)
+
+    this._first = this._first || asyncId
 
     if (hasKeepAliveBug && (type === 'TCPWRAP' || type === 'HTTPPARSER')) {
-      this._destroy(this._weaks.get(resource))
+      this._delete(this._weaks.get(resource))
       this._weaks.set(resource, asyncId)
     }
 
@@ -106,18 +121,18 @@ class Scope extends Base {
   }
 
   _destroy (asyncId) {
-    const span = this._spans[asyncId]
     const type = this._types[asyncId]
 
-    this._unref(span, asyncId)
+    this._delete(asyncId)
 
-    if (type) {
+    if (asyncId >= this._first) {
       platform.metrics().decrement('async.resources')
       platform.metrics().decrement('async.resources.by.type', `resource_type:${type}`)
     }
+  }
 
-    delete this._spans[asyncId]
-    delete this._types[asyncId]
+  _promiseResolve (asyncId) {
+    this._delete(asyncId)
   }
 }
 

diff --git a/packages/dd-trace/src/scope/async_hooks/async_wrap.js b/packages/dd-trace/src/scope/async_hooks/async_wrap.js
@@ -26,34 +26,38 @@ module.exports = {
 
     if (callbacks.init) {
       hooks.init = (uid, handle, provider, parentUid, parentHandle) => {
-        callbacks.init(uid, providers[provider], parentUid, handle)
+        if (typeof parentUid === 'number') {
+          parentUid = -parentUid
+        }
+
+        callbacks.init(-uid, providers[provider], parentUid, handle)
       }
     }
 
     if (callbacks.before) {
       hooks.pre = (uid, handle) => {
-        callbacks.before(uid)
+        callbacks.before(-uid)
       }
     }
 
     if (callbacks.after) {
       hooks.post = (uid, handle, didThrow) => {
-        callbacks.after(uid)
+        callbacks.after(-uid)
       }
     }
 
     if (callbacks.destroy) {
       hooks.destroy = (uid) => {
-        callbacks.destroy(uid)
+        callbacks.destroy(-uid)
       }
     }
 
     asyncHook.addHooks({
       pre: (uid, handle) => {
-        stack.push(uid)
+        stack.push(-uid)
       },
       post: (uid, handle, didThrow) => {
-        if (uid === this.executionAsyncId()) {
+        if (-uid === this.executionAsyncId()) {
           stack.pop()
         }
       }

diff --git a/packages/dd-trace/test/scope/async_hooks.spec.js b/packages/dd-trace/test/scope/async_hooks.spec.js
@@ -26,25 +26,31 @@ describe('Scope', () => {
   })
 
   it('should keep track of asynchronous resource count', () => {
-    scope._init(0, 'TEST')
-    scope._destroy(0)
+    const asyncId = scope._first + 1
+
+    scope._init(asyncId, 'TEST')
+    scope._destroy(asyncId)
 
     expect(metrics.increment).to.have.been.calledWith('async.resources')
     expect(metrics.decrement).to.have.been.calledWith('async.resources')
   })
 
   it('should keep track of asynchronous resource count by type', () => {
-    scope._init(0, 'TEST')
-    scope._destroy(0)
+    const asyncId = scope._first + 1
+
+    scope._init(asyncId, 'TEST')
+    scope._destroy(asyncId)
 
     expect(metrics.increment).to.have.been.calledWith('async.resources.by.type', 'resource_type:TEST')
     expect(metrics.decrement).to.have.been.calledWith('async.resources.by.type', 'resource_type:TEST')
   })
 
-  it('should only track destroys once', () => {
-    scope._init(0, 'TEST')
-    scope._destroy(0)
-    scope._destroy(0)
+  it('should only track promise destroys once', () => {
+    const asyncId = scope._first + 1
+
+    scope._init(asyncId, 'TEST')
+    scope._promiseResolve(asyncId)
+    scope._destroy(asyncId)
 
     expect(metrics.decrement).to.have.been.calledTwice
     expect(metrics.decrement).to.have.been.calledWith('async.resources')
@@ -84,12 +90,12 @@ describe('Scope', () => {
     it('should work around the HTTP keep-alive bug in Node', () => {
       const resource = {}
 
-      sinon.spy(scope, '_destroy')
+      sinon.spy(scope, '_delete')
 
       scope._init(1, 'TCPWRAP', 0, resource)
       scope._init(1, 'TCPWRAP', 0, resource)
 
-      expect(scope._destroy).to.have.been.called
+      expect(scope._delete).to.have.been.called
     })
   }