run-context for spans created by instrumentations should not bleed out to user code

[trentm]

the issues

Typically a package that makes some external call will have an API like this:

var foo = require("foo")
var client = foo.createClient(/* ... */)

// Callback API
client.doAThing(/* opts */, function afterTheThingIsDone (err, result) {
  // ... do whatever next ...
})
// ... synchronous code after client.doAThing() ...

// And/or a Promise API
client.doAThing(/* opts */)
  .then(function afterTheThingIsDone (result) {
    // ... do whatever next ...
  })
// ... synchronous code after client.doAThing() ...

// Using the Promise API via await.
async function main() {
  const result = await client.doAThing(/* opts */)
  // ... do whatever next ...
}
main()

Typically, the APM agent's instrumentation of these types of packages will create a span (an exit span) for the doAThing external call.

1. When the user's ... do whatever next ... code is called, it is important that the "run context" does not include that automatically created span -- it should be the same run context when client.doAThing() was called (the "parent" context). If ... do whatever next ... is bound to the create span context, even if that span has now ended, it can result in subsequently created spans being a child of that automatically created span.
2. Likewise, when the user's ... synchronous code after client.doAThing() ... code is called, it is important that the run context does not include the automatically created span.

In the current redis instrumentation, for example, this code:

var client = redis.createClient('6379', process.env.REDIS_HOST)
agent.startTransaction('t0')
client.set('key1', 'val1', function (err, reply) {
  // ...
})
client.set('key2', 'val2')

results in the span for SET key2 being a child of the span for SET key1. I.e. the redis instrumentation suffers from issue 2. IIRC the @elastic/elasticsearch instrumentation suffers from issues 1 and 2.

This is a problem for handling exit spans. https://github.com/elastic/apm/blob/master/specs/agents/tracing-spans.md#child-spans-of-exit-spans says:

Exit spans MUST not have child spans that have a different type or subtype.

And exit span handling is a prerequisite for span compression and work for span.context.destination.service.resource.

checklist

Review of all instrumentations and the need for: bindFunction, .with, startSpan -> createSpan, etc. Basically this involves adding test/asserts for the apm.currentSpan in the callbacks, event handlers, etc. for these integrations. Often creating a examples/trace-$thing.js is helpful to learn and illustrate. When an item is marked "N/A" in the following list, that indicates that the instrumentation does not createSpan, so there is no expectation of bleeding spans. There is still possibility that these instrumentations could better be ins.bind*()'ing for user expectations.

• @elastic/elasticsearch: fix: ensure correct run context for '@elastic/elasticsearch' instrumentation #2550
• apollo-server-core: N/A. This just sets trans._graphqlRoute.
• sns: fix: ensure correct run-context for instrumented aws-sdk client usage #2472
• sqs: fix: ensure correct run-context for instrumented aws-sdk client usage #2472
• dynamodb: fix: run context handling for aws-sdk DynamoDB instrumentation #2504
• bluebird: N/A. This does not create spans, so there should be no "bleed" of spans.
• cassandra-driver: fix: ensure correct run context for 'cassandra-driver' instrumentation #2549
• elasticsearch: fix: ensure correct run context for 'elasticsearch' (legacy) instrumentation #2551
• express: N/A.
• express-graphql: N/A. This just sets trans._graphqlRoute.
• express-queue: N/A.
• fastify: N/A. Just captureError and setDefaultTransactionName.
• finalhandler: N/A. Just captureError.
• generic-pool: N/A. This does not create spans, so there should be no "bleed" of spans.
• graphql: fix: ensure correct run context for 'graphql' instrumentation #2509
• handlebars: Using template-shared. No change necessary.
• hapi,@hapi/hapi: N/A.
• http: Using http-shared. fix: ensure the span run-context for outgoing http requests does not spill into user code #2470
• https: Using http-shared. fix: ensure the span run-context for outgoing http requests does not spill into user code #2470
• http2: fix: run-context for 'http2.request' instrumentation #2483
• ioredis: fix: rewrite ioredis instrumentation: run-context, add destination context, captureError #2460
• jade: Using template-shared. No change necessary.
• knex: N/A.
• koa: N/A.
• koa-router,@koa/router: N/A.
• memcached: fix: ensure correct run-context for 'memcached' instrumentation #2475
• mongodb-core: fix: ensure correct run context for 'mongodb-core' instrumentation #2548
• mongodb: fix: ensure correct run context for 'mongodb' instrumentation #2512
• mysql: fix: ensure run-context for mysql span does not spill into user code #2463
• mysql2: fix: ensure correct run context for mysql2 instrumentation #2487
• pg: fix: ensure correct run-context for pg instrumentation #2506
• pug: Using template-shared. No change necessary.
• redis: fix: redis run-context handling, error capture, span.outcome #2457
• restify: N/A.
• tedious: fix: ensure correct run-context for "tedious" instrumentation #2494
• ws: fix: ensure correct run context for 'ws' instrumentation #2481
• mimic-response: N/A.
• s3: fix: ensure correct run-context for instrumented aws-sdk client usage #2472