From 13e85cce3008bacc4490faecdd58ff06bf508653 Mon Sep 17 00:00:00 2001 From: rochdev Date: Wed, 28 Aug 2019 18:36:05 -0400 Subject: [PATCH 1/4] add async scope tracking as an official feature enabled by default --- docs/test.ts | 4 ++-- index.d.ts | 8 ++++++-- packages/dd-trace/src/config.js | 4 ++-- packages/dd-trace/src/scope/async_hooks.js | 6 +++--- packages/dd-trace/test/config.spec.js | 8 ++++---- packages/dd-trace/test/setup/core.js | 4 +--- 6 files changed, 18 insertions(+), 16 deletions(-) diff --git a/docs/test.ts b/docs/test.ts index 801d6ecef38..b32bd3e3b6b 100644 --- a/docs/test.ts +++ b/docs/test.ts @@ -39,9 +39,9 @@ tracer.init({ analytics: true, env: 'test', runtimeMetrics: true, + trackAsyncScope: false, experimental: { - b3: true, - thenables: true + b3: true }, hostname: 'agent', logger: { diff --git a/index.d.ts b/index.d.ts index fc57819eeb1..1aa0a163c27 100644 --- a/index.d.ts +++ b/index.d.ts @@ -230,6 +230,12 @@ export declare interface TracerOptions { */ runtimeMetrics?: boolean + /** + * Whether to track the scope of async functions. This is needed for async/await to work with non-native promises (thenables). Only disable this if you are sure only native promises are used with async/await. + * @default true + */ + trackAsyncScope?: boolean + /** * Experimental features can be enabled all at once by using true or individually using key / value pairs. * @default {} @@ -241,8 +247,6 @@ export declare interface TracerOptions { * @default false */ exporter?: 'log' | 'browser' | 'agent' - - thenables?: boolean }; /** diff --git a/packages/dd-trace/src/config.js b/packages/dd-trace/src/config.js index 859070a9b03..73fe779e0c9 100644 --- a/packages/dd-trace/src/config.js +++ b/packages/dd-trace/src/config.js @@ -53,10 +53,10 @@ class Config { port: String(coalesce(dogstatsd.port, platform.env('DD_DOGSTATSD_PORT'), 8125)) } this.runtimeMetrics = String(runtimeMetrics) === 'true' + this.trackAsyncScope = options.trackAsyncScope !== false this.experimental = { b3: !(!options.experimental || !options.experimental.b3), - exporter: options.experimental && options.experimental.exporter, - thenables: !(!options.experimental || !options.experimental.thenables) + exporter: options.experimental && options.experimental.exporter } this.reportHostname = String(reportHostname) === 'true' this.scope = process.env.DD_CONTEXT_PROPAGATION === 'false' ? scopes.NOOP : scope diff --git a/packages/dd-trace/src/scope/async_hooks.js b/packages/dd-trace/src/scope/async_hooks.js index 749ca6ac192..b9bb16572d5 100644 --- a/packages/dd-trace/src/scope/async_hooks.js +++ b/packages/dd-trace/src/scope/async_hooks.js @@ -18,7 +18,7 @@ class Scope extends Base { singleton = this - this._thenables = config.experimental.thenables + this._trackAsyncScope = config.trackAsyncScope this._current = null this._spans = Object.create(null) this._types = Object.create(null) @@ -62,7 +62,7 @@ class Scope extends Base { } _exit (span) { - this._thenables && this._await(span) + this._trackAsyncScope && this._await(span) this._current = span this._stack[this._depth] = null this._depth-- @@ -106,7 +106,7 @@ class Scope extends Base { platform.metrics().increment('async.resources') platform.metrics().increment('async.resources.by.type', `resource_type:${type}`) - if (this._thenables && type === 'PROMISE') { + if (this._trackAsyncScope && type === 'PROMISE') { this._initPromise() } } diff --git a/packages/dd-trace/test/config.spec.js b/packages/dd-trace/test/config.spec.js index 9838936a51d..e7d28d81c4d 100644 --- a/packages/dd-trace/test/config.spec.js +++ b/packages/dd-trace/test/config.spec.js @@ -27,6 +27,7 @@ describe('Config', () => { expect(config).to.have.property('flushInterval', 2000) expect(config).to.have.property('sampleRate', 1) expect(config).to.have.property('runtimeMetrics', false) + expect(config).to.have.property('trackAsyncScope', true) expect(config).to.have.deep.property('tags', {}) expect(config).to.have.property('plugins', true) expect(config).to.have.property('env', undefined) @@ -35,7 +36,6 @@ describe('Config', () => { expect(config).to.have.property('apiKey', undefined) expect(config).to.have.property('appKey', undefined) expect(config).to.have.nested.property('experimental.b3', false) - expect(config).to.have.nested.property('experimental.thenables', false) }) it('should initialize from the default service', () => { @@ -116,14 +116,14 @@ describe('Config', () => { tags, flushInterval: 5000, runtimeMetrics: true, + trackAsyncScope: false, reportHostname: true, plugins: false, scope: 'noop', apiKey: '123', appKey: '456', experimental: { - b3: true, - thenables: true + b3: true } }) @@ -141,6 +141,7 @@ describe('Config', () => { expect(config.tags).to.have.property('foo', 'bar') expect(config).to.have.property('flushInterval', 5000) expect(config).to.have.property('runtimeMetrics', true) + expect(config).to.have.property('trackAsyncScope', false) expect(config).to.have.property('reportHostname', true) expect(config).to.have.property('plugins', false) expect(config).to.have.property('scope', 'noop') @@ -150,7 +151,6 @@ describe('Config', () => { 'foo': 'bar' }) expect(config).to.have.nested.property('experimental.b3', true) - expect(config).to.have.nested.property('experimental.thenables', true) }) it('should initialize from the options with url taking precedence', () => { diff --git a/packages/dd-trace/test/setup/core.js b/packages/dd-trace/test/setup/core.js index 9d5b411db43..7989c6d9159 100644 --- a/packages/dd-trace/test/setup/core.js +++ b/packages/dd-trace/test/setup/core.js @@ -13,9 +13,7 @@ const agent = require('../plugins/agent') const externals = require('../plugins/externals.json') const asyncHooksScope = new AsyncHooksScope({ - experimental: { - thenables: true - } + trackAsyncScope: true }) chai.use(sinonChai) From 95c597e20519a8718a72da532ca851260744a623 Mon Sep 17 00:00:00 2001 From: rochdev Date: Fri, 6 Sep 2019 16:26:55 -0400 Subject: [PATCH 2/4] add safety net and nested awaits test --- packages/dd-trace/src/scope/async_hooks.js | 6 ++++++ .../dd-trace/test/scope/async_hooks.spec.js | 19 ++++++++++++++++++- 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/packages/dd-trace/src/scope/async_hooks.js b/packages/dd-trace/src/scope/async_hooks.js index b9bb16572d5..9dadf40551e 100644 --- a/packages/dd-trace/src/scope/async_hooks.js +++ b/packages/dd-trace/src/scope/async_hooks.js @@ -68,6 +68,11 @@ class Scope extends Base { this._depth-- } + _exitNative () { + this._current = null + this._promises[0] = false + } + _await (span) { if (!this._promises[this._depth]) return @@ -112,6 +117,7 @@ class Scope extends Base { } _before (asyncId) { + this._depth === 0 && this._exitNative() this._enter(this._spans[asyncId]) } diff --git a/packages/dd-trace/test/scope/async_hooks.spec.js b/packages/dd-trace/test/scope/async_hooks.spec.js index 695fa98188c..f5ebca9cc5b 100644 --- a/packages/dd-trace/test/scope/async_hooks.spec.js +++ b/packages/dd-trace/test/scope/async_hooks.spec.js @@ -91,7 +91,7 @@ describe('Scope (async_hooks)', () => { beforeEach(() => { thenable = { - then: () => {} + then: onFulfill => onFulfill() } test = async () => { @@ -155,6 +155,23 @@ describe('Scope (async_hooks)', () => { scope.activate(span, () => test()) }) }) + + it('should use the active span when using nested awaits', async () => { + return scope.activate(span, async () => { + await thenable + + thenable.then = (onFulfill, onReject) => { + try { + expect() + onFulfill() + } catch (e) { + onReject(e) + } + } + + await thenable + }) + }) }) testScope(() => scope) From ba7f179f0eb05d135748a2fbe3ca992749ab8e02 Mon Sep 17 00:00:00 2001 From: rochdev Date: Fri, 6 Sep 2019 16:41:21 -0400 Subject: [PATCH 3/4] update nock to fix tests --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 25d0d0a81ec..54fdf624368 100644 --- a/package.json +++ b/package.json @@ -89,7 +89,7 @@ "get-port": "^3.2.0", "graphql": "0.13.2", "mocha": "^5.2.0", - "nock": "^9.6.1", + "nock": "^11.3.3", "node-abi": "^2.8.0", "nyc": "^14.1.1", "prebuildify": "^2.11.0", From 0df1ab5f02ebad7967fb604e3da584b27b55d185 Mon Sep 17 00:00:00 2001 From: rochdev Date: Mon, 9 Sep 2019 15:34:50 -0400 Subject: [PATCH 4/4] remove global nock as it conflicts with the http plugin --- packages/.eslintrc.json | 1 - packages/dd-trace/test/.eslintrc.json | 1 - packages/dd-trace/test/platform/node/index.spec.js | 1 + packages/dd-trace/test/setup/core.js | 2 -- 4 files changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/.eslintrc.json b/packages/.eslintrc.json index afebed27d8a..d50e6adbf49 100644 --- a/packages/.eslintrc.json +++ b/packages/.eslintrc.json @@ -9,7 +9,6 @@ "expect": true, "sinon": true, "proxyquire": true, - "nock": true, "wrapIt": true, "withVersions": true }, diff --git a/packages/dd-trace/test/.eslintrc.json b/packages/dd-trace/test/.eslintrc.json index 2fe338462b3..43a96a43ea4 100644 --- a/packages/dd-trace/test/.eslintrc.json +++ b/packages/dd-trace/test/.eslintrc.json @@ -9,7 +9,6 @@ "expect": true, "sinon": true, "proxyquire": true, - "nock": true, "wrapIt": true, "withVersions": true }, diff --git a/packages/dd-trace/test/platform/node/index.spec.js b/packages/dd-trace/test/platform/node/index.spec.js index 0d397fd5155..6e67ed6e32f 100644 --- a/packages/dd-trace/test/platform/node/index.spec.js +++ b/packages/dd-trace/test/platform/node/index.spec.js @@ -1,5 +1,6 @@ 'use strict' +const nock = require('nock') const semver = require('semver') wrapIt() diff --git a/packages/dd-trace/test/setup/core.js b/packages/dd-trace/test/setup/core.js index 7989c6d9159..11509fdebcf 100644 --- a/packages/dd-trace/test/setup/core.js +++ b/packages/dd-trace/test/setup/core.js @@ -4,7 +4,6 @@ const sinon = require('sinon') const chai = require('chai') const sinonChai = require('sinon-chai') const proxyquire = require('../proxyquire') -const nock = require('nock') const semver = require('semver') const platform = require('../../src/platform') const node = require('../../src/platform/node') @@ -21,7 +20,6 @@ chai.use(sinonChai) global.sinon = sinon global.expect = chai.expect global.proxyquire = proxyquire -global.nock = nock global.wrapIt = wrapIt global.withVersions = withVersions