From 2ec3ab49d0241bb15ccd33fa16050bc018e7221b Mon Sep 17 00:00:00 2001 From: Bryan English Date: Thu, 14 Nov 2024 16:44:07 -0500 Subject: [PATCH 01/13] initial poc --- .../datadog-plugin-dd-trace-api/src/index.js | 50 +++++++++++++++++++ packages/dd-trace/src/plugins/index.js | 1 + 2 files changed, 51 insertions(+) create mode 100644 packages/datadog-plugin-dd-trace-api/src/index.js diff --git a/packages/datadog-plugin-dd-trace-api/src/index.js b/packages/datadog-plugin-dd-trace-api/src/index.js new file mode 100644 index 00000000000..d16ecbabe17 --- /dev/null +++ b/packages/datadog-plugin-dd-trace-api/src/index.js @@ -0,0 +1,50 @@ +'use strict' + +const Plugin = require('../../dd-trace/src/plugins/plugin') + +const objectMap = new WeakMap() + +class DdTraceApiPlugin extends Plugin { + static get id () { + return 'dd-trace-api' + } + + constructor (...args) { + super(...args) + + const tracer = this._tracer + + const handleEvent = (name, fn) => { + // For v1, APIs are 1:1 with their internal equivalents, so we can just + // call the internal method directly. That's what we do here unless we + // want to override. As the API evolves, this may change. + this.addSub(`datadog-api:v1:${name}`, ({ self, args, ret, dummy }) => { + if (!fn && name.includes(':')) { + name = name.split(':').pop() + } + + try { + ret.value = fn ? fn(args, self) : self[name](...args) + if (dummy) { + objectMap.set(dummy, ret.value) + ret.value = dummy + } + } catch (e) { + ret.error = e + } + }) + } + + handleEvent('init') + // TODO(bengl) for API calls like this one that return an object created + // internally, care needs to be taken to ensure we're not breaking the + // calling API. We don't expect spans to change much, but if they do, this + // needs to be taken into account. + handleEvent('startSpan') + handleEvent('inject') + handleEvent('extract') + handleEvent('getRumData') + handleEvent('setUser') + handleEvent('profilerStarted') + } +} diff --git a/packages/dd-trace/src/plugins/index.js b/packages/dd-trace/src/plugins/index.js index 3e77226a119..0104417b2fc 100644 --- a/packages/dd-trace/src/plugins/index.js +++ b/packages/dd-trace/src/plugins/index.js @@ -34,6 +34,7 @@ module.exports = { get couchbase () { return require('../../../datadog-plugin-couchbase/src') }, get cypress () { return require('../../../datadog-plugin-cypress/src') }, get dns () { return require('../../../datadog-plugin-dns/src') }, + get 'dd-trace-api' () { return require('../../../datadog-plugin-dd-trace-api/src') }, get elasticsearch () { return require('../../../datadog-plugin-elasticsearch/src') }, get express () { return require('../../../datadog-plugin-express/src') }, get fastify () { return require('../../../datadog-plugin-fastify/src') }, From 5ea78d857db0177aa60a02f6b0060e7db2edde5e Mon Sep 17 00:00:00 2001 From: Bryan English Date: Fri, 15 Nov 2024 14:39:28 -0500 Subject: [PATCH 02/13] get the inject test working --- .../datadog-plugin-dd-trace-api/src/index.js | 37 +++++++++++++++---- packages/dd-trace/src/plugin_manager.js | 3 ++ 2 files changed, 32 insertions(+), 8 deletions(-) diff --git a/packages/datadog-plugin-dd-trace-api/src/index.js b/packages/datadog-plugin-dd-trace-api/src/index.js index d16ecbabe17..75bbe4a5aae 100644 --- a/packages/datadog-plugin-dd-trace-api/src/index.js +++ b/packages/datadog-plugin-dd-trace-api/src/index.js @@ -4,7 +4,7 @@ const Plugin = require('../../dd-trace/src/plugins/plugin') const objectMap = new WeakMap() -class DdTraceApiPlugin extends Plugin { +module.exports = class DdTraceApiPlugin extends Plugin { static get id () { return 'dd-trace-api' } @@ -14,20 +14,36 @@ class DdTraceApiPlugin extends Plugin { const tracer = this._tracer - const handleEvent = (name, fn) => { + this.addSub('datadog-api:v1:tracerinit', ({ proxy }) => { + const proxyVal = proxy() + objectMap.set(proxyVal, tracer) + }) + + const handleEvent = (name) => { // For v1, APIs are 1:1 with their internal equivalents, so we can just // call the internal method directly. That's what we do here unless we // want to override. As the API evolves, this may change. - this.addSub(`datadog-api:v1:${name}`, ({ self, args, ret, dummy }) => { - if (!fn && name.includes(':')) { + this.addSub(`datadog-api:v1:${name}`, ({ self, args, ret, proxy }) => { + if (name.includes(':')) { name = name.split(':').pop() } + if (objectMap.has(self)) { + self = objectMap.get(self) + } + + for (let i = 0; i < args.length; i++) { + if (objectMap.has(args[i])) { + args[i] = objectMap.get(args[i]) + } + } + try { - ret.value = fn ? fn(args, self) : self[name](...args) - if (dummy) { - objectMap.set(dummy, ret.value) - ret.value = dummy + ret.value = self[name](...args) + if (proxy) { + const proxyVal = proxy() + objectMap.set(proxyVal, ret.value) + ret.value = proxyVal } } catch (e) { ret.error = e @@ -46,5 +62,10 @@ class DdTraceApiPlugin extends Plugin { handleEvent('getRumData') handleEvent('setUser') handleEvent('profilerStarted') + handleEvent('span:context') + handleEvent('span:setTag') + handleEvent('span:addTags') + handleEvent('span:finish') + handleEvent('span:addLink') } } diff --git a/packages/dd-trace/src/plugin_manager.js b/packages/dd-trace/src/plugin_manager.js index 2e6c9be9460..c1b326dc767 100644 --- a/packages/dd-trace/src/plugin_manager.js +++ b/packages/dd-trace/src/plugin_manager.js @@ -31,6 +31,9 @@ loadChannel.subscribe(({ name }) => { // Globals maybeEnable(require('../../datadog-plugin-fetch/src')) +// Always enabled +maybeEnable(require('../../datadog-plugin-dd-trace-api/src')) + function maybeEnable (Plugin) { if (!Plugin || typeof Plugin !== 'function') return if (!pluginClasses[Plugin.id]) { From d88b02c28b92c91ae5ee1d15b07936524db540f4 Mon Sep 17 00:00:00 2001 From: Bryan English Date: Fri, 6 Dec 2024 16:23:41 -0500 Subject: [PATCH 03/13] support object mapping for callback args --- .../datadog-plugin-dd-trace-api/src/index.js | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/packages/datadog-plugin-dd-trace-api/src/index.js b/packages/datadog-plugin-dd-trace-api/src/index.js index 75bbe4a5aae..28390e59d46 100644 --- a/packages/datadog-plugin-dd-trace-api/src/index.js +++ b/packages/datadog-plugin-dd-trace-api/src/index.js @@ -2,6 +2,7 @@ const Plugin = require('../../dd-trace/src/plugins/plugin') +// api ==> here const objectMap = new WeakMap() module.exports = class DdTraceApiPlugin extends Plugin { @@ -23,7 +24,7 @@ module.exports = class DdTraceApiPlugin extends Plugin { // For v1, APIs are 1:1 with their internal equivalents, so we can just // call the internal method directly. That's what we do here unless we // want to override. As the API evolves, this may change. - this.addSub(`datadog-api:v1:${name}`, ({ self, args, ret, proxy }) => { + this.addSub(`datadog-api:v1:${name}`, ({ self, args, ret, proxy, revProxy }) => { if (name.includes(':')) { name = name.split(':').pop() } @@ -36,6 +37,20 @@ module.exports = class DdTraceApiPlugin extends Plugin { if (objectMap.has(args[i])) { args[i] = objectMap.get(args[i]) } + if (typeof args[i] === 'function') { + const orig = args[i] + args[i] = (...fnArgs) => { + for (let j = 0; j < fnArgs.length; j++) { + if (revProxy && revProxy[j]) { + const proxyVal = revProxy[j]() + objectMap.set(proxyVal, fnArgs[j]) + fnArgs[j] = proxyVal + } + } + // TODO do we need to apply(this, ...) here? + return orig(...fnArgs) + } + } } try { @@ -57,6 +72,8 @@ module.exports = class DdTraceApiPlugin extends Plugin { // calling API. We don't expect spans to change much, but if they do, this // needs to be taken into account. handleEvent('startSpan') + handleEvent('wrap') + handleEvent('trace') handleEvent('inject') handleEvent('extract') handleEvent('getRumData') From 0156d2fcbc45f514aed9dfe1f9312e3e4505dbb9 Mon Sep 17 00:00:00 2001 From: Bryan English Date: Thu, 12 Dec 2024 13:50:36 -0500 Subject: [PATCH 04/13] start adding telemetry --- packages/datadog-plugin-dd-trace-api/src/index.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/packages/datadog-plugin-dd-trace-api/src/index.js b/packages/datadog-plugin-dd-trace-api/src/index.js index 28390e59d46..6fb6925aa83 100644 --- a/packages/datadog-plugin-dd-trace-api/src/index.js +++ b/packages/datadog-plugin-dd-trace-api/src/index.js @@ -1,6 +1,8 @@ 'use strict' const Plugin = require('../../dd-trace/src/plugins/plugin') +const telemetryMetrics = require('../../dd-trace/src/telemetry/metrics') +const apiMetrics = telemetryMetrics.manager.namespace('tracers') // api ==> here const objectMap = new WeakMap() @@ -21,10 +23,18 @@ module.exports = class DdTraceApiPlugin extends Plugin { }) const handleEvent = (name) => { + const counter = apiMetrics.count('dd_trace_api.called', [ + `name:${name.replace(':', '.')}`, + `api_version:v1`, + `injection_enabled:${process.env.DD_INJECTION_ENABLED ? 'yes' : 'no'}` + ]) + // For v1, APIs are 1:1 with their internal equivalents, so we can just // call the internal method directly. That's what we do here unless we // want to override. As the API evolves, this may change. this.addSub(`datadog-api:v1:${name}`, ({ self, args, ret, proxy, revProxy }) => { + counter.inc() + if (name.includes(':')) { name = name.split(':').pop() } From 6a27dd761b1af57f9f0558350029951e2c382dc9 Mon Sep 17 00:00:00 2001 From: Bryan English Date: Thu, 2 Jan 2025 15:09:00 -0500 Subject: [PATCH 05/13] parity on sending and receiving sides --- .../datadog-plugin-dd-trace-api/src/index.js | 24 ++++++++++++++----- 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/packages/datadog-plugin-dd-trace-api/src/index.js b/packages/datadog-plugin-dd-trace-api/src/index.js index 6fb6925aa83..74bb634dab1 100644 --- a/packages/datadog-plugin-dd-trace-api/src/index.js +++ b/packages/datadog-plugin-dd-trace-api/src/index.js @@ -76,23 +76,35 @@ module.exports = class DdTraceApiPlugin extends Plugin { }) } - handleEvent('init') - // TODO(bengl) for API calls like this one that return an object created - // internally, care needs to be taken to ensure we're not breaking the - // calling API. We don't expect spans to change much, but if they do, this - // needs to be taken into account. + // handleEvent('configure') handleEvent('startSpan') handleEvent('wrap') handleEvent('trace') handleEvent('inject') handleEvent('extract') handleEvent('getRumData') - handleEvent('setUser') handleEvent('profilerStarted') handleEvent('span:context') handleEvent('span:setTag') handleEvent('span:addTags') handleEvent('span:finish') handleEvent('span:addLink') + handleEvent('scope') + handleEvent('scope:activate') + handleEvent('scope:active') + handleEvent('scope:bind') + handleEvent('appsec:blockRequest') + handleEvent('appsec:isUserBlocked') + handleEvent('appsec:setUser') + handleEvent('appsec:tracerCustomEvent') + handleEvent('appsec:trackUserLoginFailureEvent') + handleEvent('appsec:trackUserLoginSuccessEvent') + handleEvent('dogstatsd:decrement') + handleEvent('dogstatsd:distribution') + handleEvent('dogstatsd:flush') + handleEvent('dogstatsd:gauge') + handleEvent('dogstatsd:histogram') + handleEvent('dogstatsd:increment') + handleEvent('use') } } From e99fbdf92d8490ba2c7ac8753fa33f0cf88e9b3a Mon Sep 17 00:00:00 2001 From: Bryan English Date: Mon, 6 Jan 2025 13:42:23 -0500 Subject: [PATCH 06/13] initial dd-trace-api test --- .../test/index.spec.js | 95 +++++++++++++++++++ 1 file changed, 95 insertions(+) create mode 100644 packages/datadog-plugin-dd-trace-api/test/index.spec.js diff --git a/packages/datadog-plugin-dd-trace-api/test/index.spec.js b/packages/datadog-plugin-dd-trace-api/test/index.spec.js new file mode 100644 index 00000000000..118f8b30f59 --- /dev/null +++ b/packages/datadog-plugin-dd-trace-api/test/index.spec.js @@ -0,0 +1,95 @@ +'use strict' + +const dc = require('dc-polyfill') + +const Plugin = require('../src') +const NoopProxy = require('../../dd-trace/src/noop/proxy') +const assert = require('assert') + +describe('Plugin', () => { + describe('dd-trace-api', () => { + let dummyTracer + let plugin + let tracer + let scope + + const allChannels = new Set() + const testedChannels = new Set() + + function test({name, fn, self = dummyTracer, ret = undefined, args = []}) { + testedChannels.add('datadog-api:v1:' + name) + const ch = dc.channel('datadog-api:v1:' + name) + const payload = { + self, + args: [], + ret: {}, + proxy: () => ret, + revProxy: [] + } + ch.publish(payload) + expect(payload.ret.value).to.equal(ret) + expect(fn).to.have.been.calledOnceWithExactly(...args) + } + + before(() => { + sinon.spy(dc, 'channel') + + tracer = new NoopProxy() + scope = tracer._tracer._scope + sinon.spy(scope) + sinon.spy(tracer) + plugin = new Plugin(tracer, {}) + plugin.configure(true) + + for (let i = 0; i < dc.channel.callCount; i++) { + const call = dc.channel.getCall(i) + const channel = call.args[0] + allChannels.add(channel) + } + + dummyTracer = {} + const payload = { + proxy: () => dummyTracer, + args: [] + } + dc.channel('datadog-api:v1:tracerinit').publish(payload) + }) + + describe('scope', () => { + let dummyScope + it('should call underlying api', () => { + dummyScope = {} + test({name: 'scope', fn: tracer.scope, ret: dummyScope}) + }) + +// describe('scope:active', () => { +// it('should call underlying api', () => { +// const scope = tracer.scope() +// test({name: 'scope:active', fn: tracer._scope.active, self: scope, ret: null}) +// }) +// }) +// +// describe('scope:activate', () => { +// it('should call underlying api', () => { +// const scope = tracer.scope() +// test({name: 'scope:activate', fn: tracer._scope.active, self: scope}) +// }) +// }) +// +// describe('scope:bind', () => { +// it('should call underlying api', () => { +// const dummyScope = tracer.scope() +// test({name: 'scope:bind', fn: tracer._scope.active, self: scope}) +// }) +// }) + + }) + + + after('dd-trace-api all events tested', () => { + // TODO uncomment next line when all are tested + // assert.deepStrictEqual([...allChannels].sort(), [...testedChannels].sort()) + }) + }) +}) + From 1a846b34ad37bda2347c0eeb44303d9edd9513c5 Mon Sep 17 00:00:00 2001 From: Bryan English Date: Mon, 6 Jan 2025 16:37:00 -0500 Subject: [PATCH 07/13] rest of dd-trace-api testing --- .../datadog-plugin-dd-trace-api/src/index.js | 9 +- .../test/index.spec.js | 307 +++++++++++++++--- 2 files changed, 275 insertions(+), 41 deletions(-) diff --git a/packages/datadog-plugin-dd-trace-api/src/index.js b/packages/datadog-plugin-dd-trace-api/src/index.js index 74bb634dab1..208c60334b9 100644 --- a/packages/datadog-plugin-dd-trace-api/src/index.js +++ b/packages/datadog-plugin-dd-trace-api/src/index.js @@ -20,12 +20,14 @@ module.exports = class DdTraceApiPlugin extends Plugin { this.addSub('datadog-api:v1:tracerinit', ({ proxy }) => { const proxyVal = proxy() objectMap.set(proxyVal, tracer) + objectMap.set(proxyVal.appsec, tracer.appsec) + objectMap.set(proxyVal.dogstatsd, tracer.dogstatsd) }) const handleEvent = (name) => { const counter = apiMetrics.count('dd_trace_api.called', [ `name:${name.replace(':', '.')}`, - `api_version:v1`, + 'api_version:v1', `injection_enabled:${process.env.DD_INJECTION_ENABLED ? 'yes' : 'no'}` ]) @@ -69,6 +71,8 @@ module.exports = class DdTraceApiPlugin extends Plugin { const proxyVal = proxy() objectMap.set(proxyVal, ret.value) ret.value = proxyVal + } else if (ret.value && typeof ret.value === 'object') { + throw new TypeError('Objects need proxies when returned via API') } } catch (e) { ret.error = e @@ -84,6 +88,7 @@ module.exports = class DdTraceApiPlugin extends Plugin { handleEvent('extract') handleEvent('getRumData') handleEvent('profilerStarted') + // TODO does context need to be wrapped/proxied? handleEvent('span:context') handleEvent('span:setTag') handleEvent('span:addTags') @@ -96,7 +101,7 @@ module.exports = class DdTraceApiPlugin extends Plugin { handleEvent('appsec:blockRequest') handleEvent('appsec:isUserBlocked') handleEvent('appsec:setUser') - handleEvent('appsec:tracerCustomEvent') + handleEvent('appsec:trackCustomEvent') handleEvent('appsec:trackUserLoginFailureEvent') handleEvent('appsec:trackUserLoginSuccessEvent') handleEvent('dogstatsd:decrement') diff --git a/packages/datadog-plugin-dd-trace-api/test/index.spec.js b/packages/datadog-plugin-dd-trace-api/test/index.spec.js index 118f8b30f59..169d0446fad 100644 --- a/packages/datadog-plugin-dd-trace-api/test/index.spec.js +++ b/packages/datadog-plugin-dd-trace-api/test/index.spec.js @@ -2,52 +2,58 @@ const dc = require('dc-polyfill') -const Plugin = require('../src') -const NoopProxy = require('../../dd-trace/src/noop/proxy') +const agent = require('../../dd-trace/test/plugins/agent') const assert = require('assert') describe('Plugin', () => { describe('dd-trace-api', () => { let dummyTracer - let plugin let tracer - let scope const allChannels = new Set() const testedChannels = new Set() - function test({name, fn, self = dummyTracer, ret = undefined, args = []}) { + function testChannel ({ name, fn, self = dummyTracer, ret = undefined, args = [] }) { testedChannels.add('datadog-api:v1:' + name) const ch = dc.channel('datadog-api:v1:' + name) const payload = { self, - args: [], + args, ret: {}, - proxy: () => ret, + proxy: ret && typeof ret === 'object' ? () => ret : undefined, revProxy: [] } ch.publish(payload) + if (payload.ret.error) { + throw payload.ret.error + } expect(payload.ret.value).to.equal(ret) expect(fn).to.have.been.calledOnceWithExactly(...args) } - before(() => { + before(async () => { sinon.spy(dc, 'channel') - tracer = new NoopProxy() - scope = tracer._tracer._scope - sinon.spy(scope) + await agent.load('dd-trace-api') + + tracer = require('../../dd-trace') + sinon.spy(tracer) - plugin = new Plugin(tracer, {}) - plugin.configure(true) + sinon.spy(tracer.appsec) + sinon.spy(tracer.dogstatsd) for (let i = 0; i < dc.channel.callCount; i++) { const call = dc.channel.getCall(i) const channel = call.args[0] - allChannels.add(channel) + if (channel.startsWith('datadog-api:v1:') && !channel.endsWith('tracerinit')) { + allChannels.add(channel) + } } - dummyTracer = {} + dummyTracer = { + appsec: {}, + dogstatsd: {} + } const payload = { proxy: () => dummyTracer, args: [] @@ -55,41 +61,264 @@ describe('Plugin', () => { dc.channel('datadog-api:v1:tracerinit').publish(payload) }) + after(() => agent.close({ ritmReset: false })) + describe('scope', () => { let dummyScope + let scope + it('should call underlying api', () => { dummyScope = {} - test({name: 'scope', fn: tracer.scope, ret: dummyScope}) + testChannel({ name: 'scope', fn: tracer.scope, ret: dummyScope }) + }) + + describe('scope:active', () => { + it('should call underlying api', () => { + scope = tracer.scope() + sinon.spy(scope, 'active') + testChannel({ name: 'scope:active', fn: scope.active, self: dummyScope, ret: null }) + scope.active.restore() + }) + }) + + describe('scope:activate', () => { + it('should call underlying api', () => { + scope = tracer.scope() + sinon.spy(scope, 'activate') + testChannel({ name: 'scope:activate', fn: scope.activate, self: dummyScope }) + scope.activate.restore() + }) + }) + + describe('scope:bind', () => { + it('should call underlying api', () => { + scope = tracer.scope() + sinon.spy(scope, 'bind') + testChannel({ name: 'scope:bind', fn: scope.bind, self: dummyScope }) + scope.bind.restore() + }) + }) + }) + + describe('inject', () => { + it('should call underlying api', () => { + testChannel({ name: 'inject', fn: tracer.inject }) }) + }) -// describe('scope:active', () => { -// it('should call underlying api', () => { -// const scope = tracer.scope() -// test({name: 'scope:active', fn: tracer._scope.active, self: scope, ret: null}) -// }) -// }) -// -// describe('scope:activate', () => { -// it('should call underlying api', () => { -// const scope = tracer.scope() -// test({name: 'scope:activate', fn: tracer._scope.active, self: scope}) -// }) -// }) -// -// describe('scope:bind', () => { -// it('should call underlying api', () => { -// const dummyScope = tracer.scope() -// test({name: 'scope:bind', fn: tracer._scope.active, self: scope}) -// }) -// }) + describe('extract', () => { + it('should call underlying api', () => { + testChannel({ name: 'extract', fn: tracer.extract, ret: null }) + }) + }) + describe('getRumData', () => { + it('should call underlying api', () => { + testChannel({ name: 'getRumData', fn: tracer.getRumData, ret: '' }) + }) }) + describe('trace', () => { + it('should call underlying api', () => { + testChannel({ name: 'trace', fn: tracer.trace }) + }) + }) + + describe('wrap', () => { + it('should call underlying api', () => { + testChannel({ name: 'wrap', fn: tracer.wrap }) + }) + }) + + describe('use', () => { + it('should call underlying api', () => { + testChannel({ name: 'use', fn: tracer.use, ret: dummyTracer }) + }) + }) + + describe('profilerStarted', () => { + it('should call underlying api', () => { + testChannel({ name: 'profilerStarted', fn: tracer.profilerStarted, ret: Promise.resolve(false) }) + }) + }) + + describe('startSpan', () => { + let dummySpan + let dummySpanContext + let span + let spanContext + + it('should call underlying api', () => { + dummySpan = {} + testChannel({ name: 'startSpan', fn: tracer.startSpan, ret: dummySpan }) + span = tracer.startSpan.getCall(0).returnValue + sinon.spy(span) + }) + + describe('span:context', () => { + it('should call underlying api', () => { + dummySpanContext = {} + testChannel({ name: 'span:context', fn: span.context, self: dummySpan, ret: dummySpanContext }) + spanContext = span.context.getCall(0).returnValue + sinon.spy(spanContext) + }) + }) + + describe('span:setTag', () => { + it('should call underlying api', () => { + testChannel({ name: 'span:setTag', fn: span.setTag, self: dummySpan, ret: dummySpan }) + }) + }) + + describe('span:addTags', () => { + it('should call underlying api', () => { + testChannel({ name: 'span:addTags', fn: span.addTags, self: dummySpan, ret: dummySpan }) + }) + }) + + describe('span:finish', () => { + it('should call underlying api', () => { + testChannel({ name: 'span:finish', fn: span.finish, self: dummySpan }) + }) + }) + + describe('span:addLink', () => { + it('should call underlying api', () => { + testChannel({ + name: 'span:addLink', + fn: span.addLink, + self: dummySpan, + ret: dummySpan, + args: [dummySpanContext] + }) + }) + }) + }) + + describe('appsec:blockRequest', () => { + it('should call underlying api', () => { + testChannel({ + name: 'appsec:blockRequest', + fn: tracer.appsec.blockRequest, + self: tracer.appsec, + ret: false + }) + }) + }) + + describe('appsec:isUserBlocked', () => { + it('should call underlying api', () => { + testChannel({ + name: 'appsec:isUserBlocked', + fn: tracer.appsec.isUserBlocked, + self: tracer.appsec, + ret: false + }) + }) + }) + + describe('appsec:setUser', () => { + it('should call underlying api', () => { + testChannel({ + name: 'appsec:setUser', + fn: tracer.appsec.setUser, + self: tracer.appsec + }) + }) + }) + + describe('appsec:trackCustomEvent', () => { + it('should call underlying api', () => { + testChannel({ + name: 'appsec:trackCustomEvent', + fn: tracer.appsec.trackCustomEvent, + self: tracer.appsec + }) + }) + }) + + describe('appsec:trackUserLoginFailureEvent', () => { + it('should call underlying api', () => { + testChannel({ + name: 'appsec:trackUserLoginFailureEvent', + fn: tracer.appsec.trackUserLoginFailureEvent, + self: tracer.appsec + }) + }) + }) + + describe('appsec:trackUserLoginSuccessEvent', () => { + it('should call underlying api', () => { + testChannel({ + name: 'appsec:trackUserLoginSuccessEvent', + fn: tracer.appsec.trackUserLoginSuccessEvent, + self: tracer.appsec + }) + }) + }) + + describe('dogstatsd:decrement', () => { + it('should call underlying api', () => { + testChannel({ + name: 'dogstatsd:decrement', + fn: tracer.dogstatsd.decrement, + self: tracer.dogstatsd + }) + }) + }) + + describe('dogstatsd:distribution', () => { + it('should call underlying api', () => { + testChannel({ + name: 'dogstatsd:distribution', + fn: tracer.dogstatsd.distribution, + self: tracer.dogstatsd + }) + }) + }) + + describe('dogstatsd:flush', () => { + it('should call underlying api', () => { + testChannel({ + name: 'dogstatsd:flush', + fn: tracer.dogstatsd.flush, + self: tracer.dogstatsd + }) + }) + }) + + describe('dogstatsd:gauge', () => { + it('should call underlying api', () => { + testChannel({ + name: 'dogstatsd:gauge', + fn: tracer.dogstatsd.gauge, + self: tracer.dogstatsd + }) + }) + }) + + describe('dogstatsd:histogram', () => { + it('should call underlying api', () => { + testChannel({ + name: 'dogstatsd:histogram', + fn: tracer.dogstatsd.histogram, + self: tracer.dogstatsd + }) + }) + }) + + describe('dogstatsd:increment', () => { + it('should call underlying api', () => { + testChannel({ + name: 'dogstatsd:increment', + fn: tracer.dogstatsd.increment, + self: tracer.dogstatsd + }) + }) + }) after('dd-trace-api all events tested', () => { - // TODO uncomment next line when all are tested - // assert.deepStrictEqual([...allChannels].sort(), [...testedChannels].sort()) + assert.deepStrictEqual([...allChannels].sort(), [...testedChannels].sort()) }) }) }) - From 5f3779b39a6b486b8b2ae7939eb3fccb31484b6b Mon Sep 17 00:00:00 2001 From: Bryan English Date: Mon, 6 Jan 2025 16:51:38 -0500 Subject: [PATCH 08/13] quick test refactor --- .../datadog-plugin-dd-trace-api/src/index.js | 2 +- .../test/index.spec.js | 236 +++++------------- 2 files changed, 62 insertions(+), 176 deletions(-) diff --git a/packages/datadog-plugin-dd-trace-api/src/index.js b/packages/datadog-plugin-dd-trace-api/src/index.js index 208c60334b9..89f9111cae7 100644 --- a/packages/datadog-plugin-dd-trace-api/src/index.js +++ b/packages/datadog-plugin-dd-trace-api/src/index.js @@ -72,7 +72,7 @@ module.exports = class DdTraceApiPlugin extends Plugin { objectMap.set(proxyVal, ret.value) ret.value = proxyVal } else if (ret.value && typeof ret.value === 'object') { - throw new TypeError('Objects need proxies when returned via API') + throw new TypeError(`Objects need proxies when returned via API (${name})`) } } catch (e) { ret.error = e diff --git a/packages/datadog-plugin-dd-trace-api/test/index.spec.js b/packages/datadog-plugin-dd-trace-api/test/index.spec.js index 169d0446fad..1c682b3bfcd 100644 --- a/packages/datadog-plugin-dd-trace-api/test/index.spec.js +++ b/packages/datadog-plugin-dd-trace-api/test/index.spec.js @@ -5,6 +5,8 @@ const dc = require('dc-polyfill') const agent = require('../../dd-trace/test/plugins/agent') const assert = require('assert') +const SELF = Symbol('self') + describe('Plugin', () => { describe('dd-trace-api', () => { let dummyTracer @@ -13,24 +15,6 @@ describe('Plugin', () => { const allChannels = new Set() const testedChannels = new Set() - function testChannel ({ name, fn, self = dummyTracer, ret = undefined, args = [] }) { - testedChannels.add('datadog-api:v1:' + name) - const ch = dc.channel('datadog-api:v1:' + name) - const payload = { - self, - args, - ret: {}, - proxy: ret && typeof ret === 'object' ? () => ret : undefined, - revProxy: [] - } - ch.publish(payload) - if (payload.ret.error) { - throw payload.ret.error - } - expect(payload.ret.value).to.equal(ret) - expect(fn).to.have.been.calledOnceWithExactly(...args) - } - before(async () => { sinon.spy(dc, 'channel') @@ -100,48 +84,6 @@ describe('Plugin', () => { }) }) - describe('inject', () => { - it('should call underlying api', () => { - testChannel({ name: 'inject', fn: tracer.inject }) - }) - }) - - describe('extract', () => { - it('should call underlying api', () => { - testChannel({ name: 'extract', fn: tracer.extract, ret: null }) - }) - }) - - describe('getRumData', () => { - it('should call underlying api', () => { - testChannel({ name: 'getRumData', fn: tracer.getRumData, ret: '' }) - }) - }) - - describe('trace', () => { - it('should call underlying api', () => { - testChannel({ name: 'trace', fn: tracer.trace }) - }) - }) - - describe('wrap', () => { - it('should call underlying api', () => { - testChannel({ name: 'wrap', fn: tracer.wrap }) - }) - }) - - describe('use', () => { - it('should call underlying api', () => { - testChannel({ name: 'use', fn: tracer.use, ret: dummyTracer }) - }) - }) - - describe('profilerStarted', () => { - it('should call underlying api', () => { - testChannel({ name: 'profilerStarted', fn: tracer.profilerStarted, ret: Promise.resolve(false) }) - }) - }) - describe('startSpan', () => { let dummySpan let dummySpanContext @@ -195,130 +137,74 @@ describe('Plugin', () => { }) }) - describe('appsec:blockRequest', () => { - it('should call underlying api', () => { - testChannel({ - name: 'appsec:blockRequest', - fn: tracer.appsec.blockRequest, - self: tracer.appsec, - ret: false - }) - }) - }) + describeMethod('inject') + describeMethod('extract', null) + describeMethod('getRumData', '') + describeMethod('trace') + describeMethod('wrap') + describeMethod('use', SELF) + describeMethod('profilerStarted', Promise.resolve(false)) - describe('appsec:isUserBlocked', () => { - it('should call underlying api', () => { - testChannel({ - name: 'appsec:isUserBlocked', - fn: tracer.appsec.isUserBlocked, - self: tracer.appsec, - ret: false - }) - }) - }) + describeSubsystem('appsec', 'blockRequest', false) + describeSubsystem('appsec', 'isUserBlocked', false) + describeSubsystem('appsec', 'setUser') + describeSubsystem('appsec', 'trackCustomEvent') + describeSubsystem('appsec', 'trackUserLoginFailureEvent') + describeSubsystem('appsec', 'trackUserLoginSuccessEvent') + describeSubsystem('dogstatsd', 'decrement') + describeSubsystem('dogstatsd', 'distribution') + describeSubsystem('dogstatsd', 'flush') + describeSubsystem('dogstatsd', 'gauge') + describeSubsystem('dogstatsd', 'histogram') + describeSubsystem('dogstatsd', 'increment') - describe('appsec:setUser', () => { - it('should call underlying api', () => { - testChannel({ - name: 'appsec:setUser', - fn: tracer.appsec.setUser, - self: tracer.appsec - }) - }) - }) - - describe('appsec:trackCustomEvent', () => { - it('should call underlying api', () => { - testChannel({ - name: 'appsec:trackCustomEvent', - fn: tracer.appsec.trackCustomEvent, - self: tracer.appsec - }) - }) - }) - - describe('appsec:trackUserLoginFailureEvent', () => { - it('should call underlying api', () => { - testChannel({ - name: 'appsec:trackUserLoginFailureEvent', - fn: tracer.appsec.trackUserLoginFailureEvent, - self: tracer.appsec - }) - }) - }) - - describe('appsec:trackUserLoginSuccessEvent', () => { - it('should call underlying api', () => { - testChannel({ - name: 'appsec:trackUserLoginSuccessEvent', - fn: tracer.appsec.trackUserLoginSuccessEvent, - self: tracer.appsec - }) - }) - }) - - describe('dogstatsd:decrement', () => { - it('should call underlying api', () => { - testChannel({ - name: 'dogstatsd:decrement', - fn: tracer.dogstatsd.decrement, - self: tracer.dogstatsd - }) - }) - }) - - describe('dogstatsd:distribution', () => { - it('should call underlying api', () => { - testChannel({ - name: 'dogstatsd:distribution', - fn: tracer.dogstatsd.distribution, - self: tracer.dogstatsd - }) - }) - }) - - describe('dogstatsd:flush', () => { - it('should call underlying api', () => { - testChannel({ - name: 'dogstatsd:flush', - fn: tracer.dogstatsd.flush, - self: tracer.dogstatsd - }) - }) - }) - - describe('dogstatsd:gauge', () => { - it('should call underlying api', () => { - testChannel({ - name: 'dogstatsd:gauge', - fn: tracer.dogstatsd.gauge, - self: tracer.dogstatsd - }) - }) + after('dd-trace-api all events tested', () => { + assert.deepStrictEqual([...allChannels].sort(), [...testedChannels].sort()) }) - describe('dogstatsd:histogram', () => { - it('should call underlying api', () => { - testChannel({ - name: 'dogstatsd:histogram', - fn: tracer.dogstatsd.histogram, - self: tracer.dogstatsd + function describeMethod(name, ret) { + describe(name, () => { + it('should call underlying api', () => { + if (ret === SELF) { + ret = dummyTracer + } + testChannel({ name, fn: tracer[name], ret }) }) }) - }) + } - describe('dogstatsd:increment', () => { - it('should call underlying api', () => { - testChannel({ - name: 'dogstatsd:increment', - fn: tracer.dogstatsd.increment, - self: tracer.dogstatsd + function describeSubsystem(name, command, ret) { + describe(`${name}:${command}`, () => { + it('should call underlying api', () => { + const options = { + name: `${name}:${command}`, + fn: tracer[name][command], + self: tracer[name] + } + if (typeof ret !== 'undefined') { + options.ret = ret + } + testChannel(options) }) }) - }) + } - after('dd-trace-api all events tested', () => { - assert.deepStrictEqual([...allChannels].sort(), [...testedChannels].sort()) - }) + function testChannel ({ name, fn, self = dummyTracer, ret = undefined, args = [] }) { + testedChannels.add('datadog-api:v1:' + name) + const ch = dc.channel('datadog-api:v1:' + name) + const payload = { + self, + args, + ret: {}, + proxy: ret && typeof ret === 'object' ? () => ret : undefined, + revProxy: [] + } + ch.publish(payload) + if (payload.ret.error) { + throw payload.ret.error + } + expect(payload.ret.value).to.equal(ret) + expect(fn).to.have.been.calledOnceWithExactly(...args) + } }) }) From 50b49528897721760e1648a464f1a68c8e4a93fa Mon Sep 17 00:00:00 2001 From: Bryan English Date: Fri, 17 Jan 2025 15:25:50 -0500 Subject: [PATCH 09/13] support span context methods --- .../datadog-plugin-dd-trace-api/src/index.js | 4 ++- .../test/index.spec.js | 26 ++++++++++++++++++- 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/packages/datadog-plugin-dd-trace-api/src/index.js b/packages/datadog-plugin-dd-trace-api/src/index.js index 89f9111cae7..6ff4b49fe04 100644 --- a/packages/datadog-plugin-dd-trace-api/src/index.js +++ b/packages/datadog-plugin-dd-trace-api/src/index.js @@ -88,7 +88,9 @@ module.exports = class DdTraceApiPlugin extends Plugin { handleEvent('extract') handleEvent('getRumData') handleEvent('profilerStarted') - // TODO does context need to be wrapped/proxied? + handleEvent('context:toTraceId') + handleEvent('context:toSpanId') + handleEvent('context:toTraceparent') handleEvent('span:context') handleEvent('span:setTag') handleEvent('span:addTags') diff --git a/packages/datadog-plugin-dd-trace-api/test/index.spec.js b/packages/datadog-plugin-dd-trace-api/test/index.spec.js index 1c682b3bfcd..d9660cded9a 100644 --- a/packages/datadog-plugin-dd-trace-api/test/index.spec.js +++ b/packages/datadog-plugin-dd-trace-api/test/index.spec.js @@ -98,11 +98,35 @@ describe('Plugin', () => { }) describe('span:context', () => { + let traceId = '1234567890abcdef' + let spanId = 'abcdef1234567890' + let traceparent= `00-${traceId}-${spanId}-01` + it('should call underlying api', () => { dummySpanContext = {} testChannel({ name: 'span:context', fn: span.context, self: dummySpan, ret: dummySpanContext }) spanContext = span.context.getCall(0).returnValue - sinon.spy(spanContext) + sinon.stub(spanContext, 'toTraceId').callsFake(() => traceId) + sinon.stub(spanContext, 'toSpanId').callsFake(() => spanId) + sinon.stub(spanContext, 'toTraceparent').callsFake(() => traceparent) + }) + + describe('context:toTraceId', () => { + it('should call underlying api', () => { + testChannel({ name: 'context:toTraceId', fn: spanContext.toTraceId, self: dummySpanContext, ret: traceId }) + }) + }) + + describe('context:toSpanId', () => { + it('should call underlying api', () => { + testChannel({ name: 'context:toSpanId', fn: spanContext.toSpanId, self: dummySpanContext, ret: spanId }) + }) + }) + + describe('context:toTraceparent', () => { + it('should call underlying api', () => { + testChannel({ name: 'context:toTraceparent', fn: spanContext.toTraceparent, self: dummySpanContext, ret: traceparent }) + }) }) }) From 41102aa49e5dce4b8ef6d538edb300e4d57a3e29 Mon Sep 17 00:00:00 2001 From: Bryan English Date: Wed, 22 Jan 2025 15:35:56 -0500 Subject: [PATCH 10/13] lint --- .../test/index.spec.js | 89 +++++++++++++++---- 1 file changed, 72 insertions(+), 17 deletions(-) diff --git a/packages/datadog-plugin-dd-trace-api/test/index.spec.js b/packages/datadog-plugin-dd-trace-api/test/index.spec.js index d9660cded9a..55523177d9e 100644 --- a/packages/datadog-plugin-dd-trace-api/test/index.spec.js +++ b/packages/datadog-plugin-dd-trace-api/test/index.spec.js @@ -53,14 +53,23 @@ describe('Plugin', () => { it('should call underlying api', () => { dummyScope = {} - testChannel({ name: 'scope', fn: tracer.scope, ret: dummyScope }) + testChannel({ + name: 'scope', + fn: tracer.scope, + ret: dummyScope + }) }) describe('scope:active', () => { it('should call underlying api', () => { scope = tracer.scope() sinon.spy(scope, 'active') - testChannel({ name: 'scope:active', fn: scope.active, self: dummyScope, ret: null }) + testChannel({ + name: 'scope:active', + fn: scope.active, + self: dummyScope, + ret: null + }) scope.active.restore() }) }) @@ -69,7 +78,11 @@ describe('Plugin', () => { it('should call underlying api', () => { scope = tracer.scope() sinon.spy(scope, 'activate') - testChannel({ name: 'scope:activate', fn: scope.activate, self: dummyScope }) + testChannel({ + name: 'scope:activate', + fn: scope.activate, + self: dummyScope + }) scope.activate.restore() }) }) @@ -78,7 +91,11 @@ describe('Plugin', () => { it('should call underlying api', () => { scope = tracer.scope() sinon.spy(scope, 'bind') - testChannel({ name: 'scope:bind', fn: scope.bind, self: dummyScope }) + testChannel({ + name: 'scope:bind', + fn: scope.bind, + self: dummyScope + }) scope.bind.restore() }) }) @@ -92,19 +109,28 @@ describe('Plugin', () => { it('should call underlying api', () => { dummySpan = {} - testChannel({ name: 'startSpan', fn: tracer.startSpan, ret: dummySpan }) + testChannel({ + name: 'startSpan', + fn: tracer.startSpan, + ret: dummySpan + }) span = tracer.startSpan.getCall(0).returnValue sinon.spy(span) }) describe('span:context', () => { - let traceId = '1234567890abcdef' - let spanId = 'abcdef1234567890' - let traceparent= `00-${traceId}-${spanId}-01` + const traceId = '1234567890abcdef' + const spanId = 'abcdef1234567890' + const traceparent = `00-${traceId}-${spanId}-01` it('should call underlying api', () => { dummySpanContext = {} - testChannel({ name: 'span:context', fn: span.context, self: dummySpan, ret: dummySpanContext }) + testChannel({ + name: 'span:context', + fn: span.context, + self: dummySpan, + ret: dummySpanContext + }) spanContext = span.context.getCall(0).returnValue sinon.stub(spanContext, 'toTraceId').callsFake(() => traceId) sinon.stub(spanContext, 'toSpanId').callsFake(() => spanId) @@ -113,38 +139,67 @@ describe('Plugin', () => { describe('context:toTraceId', () => { it('should call underlying api', () => { - testChannel({ name: 'context:toTraceId', fn: spanContext.toTraceId, self: dummySpanContext, ret: traceId }) + testChannel({ + name: 'context:toTraceId', + fn: spanContext.toTraceId, + self: dummySpanContext, + ret: traceId + }) }) }) describe('context:toSpanId', () => { it('should call underlying api', () => { - testChannel({ name: 'context:toSpanId', fn: spanContext.toSpanId, self: dummySpanContext, ret: spanId }) + testChannel({ + name: 'context:toSpanId', + fn: spanContext.toSpanId, + self: dummySpanContext, + ret: spanId + }) }) }) describe('context:toTraceparent', () => { it('should call underlying api', () => { - testChannel({ name: 'context:toTraceparent', fn: spanContext.toTraceparent, self: dummySpanContext, ret: traceparent }) + testChannel({ + name: 'context:toTraceparent', + fn: spanContext.toTraceparent, + self: dummySpanContext, + ret: traceparent + }) }) }) }) describe('span:setTag', () => { it('should call underlying api', () => { - testChannel({ name: 'span:setTag', fn: span.setTag, self: dummySpan, ret: dummySpan }) + testChannel({ + name: 'span:setTag', + fn: span.setTag, + self: dummySpan, + ret: dummySpan + }) }) }) describe('span:addTags', () => { it('should call underlying api', () => { - testChannel({ name: 'span:addTags', fn: span.addTags, self: dummySpan, ret: dummySpan }) + testChannel({ + name: 'span:addTags', + fn: span.addTags, + self: dummySpan, + ret: dummySpan + }) }) }) describe('span:finish', () => { it('should call underlying api', () => { - testChannel({ name: 'span:finish', fn: span.finish, self: dummySpan }) + testChannel({ + name: 'span:finish', + fn: span.finish, + self: dummySpan + }) }) }) @@ -186,7 +241,7 @@ describe('Plugin', () => { assert.deepStrictEqual([...allChannels].sort(), [...testedChannels].sort()) }) - function describeMethod(name, ret) { + function describeMethod (name, ret) { describe(name, () => { it('should call underlying api', () => { if (ret === SELF) { @@ -197,7 +252,7 @@ describe('Plugin', () => { }) } - function describeSubsystem(name, command, ret) { + function describeSubsystem (name, command, ret) { describe(`${name}:${command}`, () => { it('should call underlying api', () => { const options = { From 13e11b1a5614d679f451eaa3a5ef03dd848b09f9 Mon Sep 17 00:00:00 2001 From: Bryan English Date: Tue, 28 Jan 2025 15:16:36 -0500 Subject: [PATCH 11/13] hoist counter tag --- packages/datadog-plugin-dd-trace-api/src/index.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/datadog-plugin-dd-trace-api/src/index.js b/packages/datadog-plugin-dd-trace-api/src/index.js index 6ff4b49fe04..e2c8c5ad4ff 100644 --- a/packages/datadog-plugin-dd-trace-api/src/index.js +++ b/packages/datadog-plugin-dd-trace-api/src/index.js @@ -7,6 +7,9 @@ const apiMetrics = telemetryMetrics.manager.namespace('tracers') // api ==> here const objectMap = new WeakMap() +const injectionEnabledTag = + `injection_enabled:${process.env.DD_INJECTION_ENABLED ? 'yes' : 'no'}` + module.exports = class DdTraceApiPlugin extends Plugin { static get id () { return 'dd-trace-api' @@ -28,7 +31,7 @@ module.exports = class DdTraceApiPlugin extends Plugin { const counter = apiMetrics.count('dd_trace_api.called', [ `name:${name.replace(':', '.')}`, 'api_version:v1', - `injection_enabled:${process.env.DD_INJECTION_ENABLED ? 'yes' : 'no'}` + injectionEnabledTag ]) // For v1, APIs are 1:1 with their internal equivalents, so we can just From 09d3fa18216506d98cb16ccc0bf5e87a88608844 Mon Sep 17 00:00:00 2001 From: Bryan English Date: Tue, 28 Jan 2025 15:21:09 -0500 Subject: [PATCH 12/13] add plugin test --- .github/workflows/plugins.yml | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/.github/workflows/plugins.yml b/.github/workflows/plugins.yml index d216a0fa5fe..4ee5836448e 100644 --- a/.github/workflows/plugins.yml +++ b/.github/workflows/plugins.yml @@ -317,6 +317,14 @@ jobs: suffix: plugins-${{ github.job }} - uses: codecov/codecov-action@v5 + dd-trace-api: + runs-on: ubuntu-latest + env: + PLUGINS: dd-trace-api + steps: + - uses: actions/checkout@v4 + - uses: ./.github/actions/plugins/test + dns: runs-on: ubuntu-latest env: From 85fe6d93a74a3974540ec26926aae1eff492f856 Mon Sep 17 00:00:00 2001 From: Bryan English Date: Fri, 31 Jan 2025 09:38:07 -0500 Subject: [PATCH 13/13] replace -> replaceAll --- packages/datadog-plugin-dd-trace-api/src/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/datadog-plugin-dd-trace-api/src/index.js b/packages/datadog-plugin-dd-trace-api/src/index.js index e2c8c5ad4ff..0e7c40764b1 100644 --- a/packages/datadog-plugin-dd-trace-api/src/index.js +++ b/packages/datadog-plugin-dd-trace-api/src/index.js @@ -29,7 +29,7 @@ module.exports = class DdTraceApiPlugin extends Plugin { const handleEvent = (name) => { const counter = apiMetrics.count('dd_trace_api.called', [ - `name:${name.replace(':', '.')}`, + `name:${name.replaceAll(':', '.')}`, 'api_version:v1', injectionEnabledTag ])