fix: more reliable span.sync determination

Current `span.sync` is tracked via:
- the "before" async hook setting it false on the "active" span, and
- Instrumentation#bindFunction's wrapper setting it false, on the
  fair assumption that all usages of bindFunction are for async
  callbacks.
The former has issues when there are multiple active spans within a
single async task -- as is the case with Elasticsearch instrumentation
(issue #1996) and the aws-sdk instrumentations (which have manual
workarounds).

This changes to set sync=false if the executionAsyncId() at end-time
is different than at start-time. This works for whatever `asyncHooks`
config var value.

Fixes: #1996

lib/instrumentation/async-hooks.js

@@ -92,14 +92,6 @@ module.exports = function (ins) {
   }
 
   function before (asyncId) {
-    const span = activeSpans.get(asyncId)
-    if (span && !span.ended) {
-      span.sync = false
-    }
-    const transaction = span ? span.transaction : activeTransactions.get(asyncId)
-    if (transaction && !transaction.ended) {
-      transaction.sync = false
-    }
     ins.bindingSpan = null
   }
 

lib/instrumentation/index.js

@@ -353,11 +353,7 @@ var wrapped = Symbol('elastic-apm-wrapped-function')
 // the callback function gets instrument on "the right" transaction and span.
 //
 // The instrumentation programmer is still responsible for starting a span,
-// and ending a span.  Additionally, this function will set a span's sync
-// property to `false` -- it's up to the instrumentation programmer to ensure
-// that the callback they're binding is really async.  If bindFunction is
-// passed a callback that the wrapped function executes synchronously, it will
-// still mark the span's `async` property as `false`.
+// and ending a span.
 //
 // @param {function} original
 Instrumentation.prototype.bindFunction = function (original) {
@@ -379,8 +375,6 @@ Instrumentation.prototype.bindFunction = function (original) {
     ins.currentTransaction = trans
     ins.bindingSpan = null
     ins.activeSpan = span
-    if (trans) trans.sync = false
-    if (span) span.sync = false
     var result = original.apply(this, arguments)
     ins.currentTransaction = prevTrans
     return result

lib/instrumentation/modules/aws-sdk/dynamodb.js

@@ -100,13 +100,6 @@ function instrumentationDynamoDb (orig, origArguments, request, AWS, agent, { ve
       span._setOutcomeFromErrorCapture(constants.OUTCOME_FAILURE)
     }
 
-    // Workaround a bug in the agent's handling of `span.sync`.
-    //
-    // The bug: Currently this span.sync is not set `false` because there is
-    // an HTTP span created (for this S3 request) in the same async op. That
-    // HTTP span becomes the "active span" for this async op, and *it* gets
-    // marked as sync=false in `before()` in async-hooks.js.
-    span.sync = false
     span.end()
   })
 

lib/instrumentation/modules/aws-sdk/s3.js

@@ -109,14 +109,6 @@ function instrumentationS3 (orig, origArguments, request, AWS, agent, { version,
         }
       }
 
-      // Workaround a bug in the agent's handling of `span.sync`.
-      //
-      // The bug: Currently this span.sync is not set `false` because there is
-      // an HTTP span created (for this S3 request) in the same async op. That
-      // HTTP span becomes the "active span" for this async op, and *it* gets
-      // marked as sync=false in `before()` in async-hooks.js.
-      span.sync = false
-
       span.end()
     })
   }

lib/instrumentation/modules/aws-sdk/sqs.js

@@ -163,9 +163,6 @@ function instrumentationSqs (orig, origArguments, request, AWS, agent, { version
       span._setOutcomeFromErrorCapture(constants.OUTCOME_FAILURE)
     }
 
-    // we'll need to manually mark this span as async.  The actual async hop
-    // is captured by the agent's async hooks instrumentation
-    span.sync = false
     span.end()
 
     if (request.operation === 'receiveMessage' && response && response.data) {

lib/instrumentation/span.js

@@ -1,5 +1,6 @@
 'use strict'
 
+const { executionAsyncId } = require('async_hooks')
 var util = require('util')
 
 var Value = require('async-value-promise')
@@ -34,6 +35,7 @@ function Span (transaction, name, ...args) {
   this._message = null
   this._stackObj = null
   this._capturedStackTrace = null
+  this._startXid = executionAsyncId()
 
   this.transaction = transaction
   this.name = name || 'unnamed'
@@ -71,6 +73,9 @@ Span.prototype.end = function (endTime) {
   }
 
   this._timer.end(endTime)
+  if (executionAsyncId() !== this._startXid) {
+    this.sync = false
+  }
 
   this._setOutcomeFromSpanEnd()
 

lib/instrumentation/transaction.js

@@ -1,5 +1,6 @@
 'use strict'
 
+const { executionAsyncId } = require('async_hooks')
 var util = require('util')
 
 var ObjectIdentityMap = require('object-identity-map')
@@ -35,6 +36,7 @@ function Transaction (agent, name, ...args) {
   this._contextLost = false // TODO: Send this up to the server some how
   this._abortTime = 0
   this._breakdownTimings = new ObjectIdentityMap()
+  this._startXid = executionAsyncId()
 
   this.outcome = constants.OUTCOME_UNKNOWN
 }
@@ -232,6 +234,9 @@ Transaction.prototype.end = function (result, endTime) {
   this._timer.end(endTime)
   this._captureBreakdown(this)
   this.ended = true
+  if (executionAsyncId() !== this._startXid) {
+    this.sync = false
+  }
 
   var trans = this._agent._instrumentation.currentTransaction