From ec2975da6ba14f11d13fa36a0082904861508fa5 Mon Sep 17 00:00:00 2001 From: Thomas Watson Date: Thu, 10 Oct 2024 09:06:42 +0200 Subject: [PATCH] Address review comments --- CODEOWNERS | 1 + .../datadog-instrumentations/src/fastify.js | 13 +++--------- .../datadog-plugin-code-origin/src/fastify.js | 20 +++++++++++++++++++ .../datadog-plugin-code-origin/src/index.js | 17 ++++++++++++++++ .../src/tags.js} | 2 +- packages/datadog-plugin-fastify/src/index.js | 14 +------------ packages/datadog-plugin-web/src/index.js | 4 ---- packages/dd-trace/src/plugins/index.js | 1 + packages/dd-trace/src/plugins/util/web.js | 9 --------- .../dd-trace/test/plugins/util/web.spec.js | 16 --------------- 10 files changed, 44 insertions(+), 53 deletions(-) create mode 100644 packages/datadog-plugin-code-origin/src/fastify.js create mode 100644 packages/datadog-plugin-code-origin/src/index.js rename packages/{dd-trace/src/code_origin.js => datadog-plugin-code-origin/src/tags.js} (90%) diff --git a/CODEOWNERS b/CODEOWNERS index 8f7d53e03b0..59c46ca37fb 100644 --- a/CODEOWNERS +++ b/CODEOWNERS @@ -4,6 +4,7 @@ /packages/dd-trace/test/appsec/ @DataDog/asm-js /integration-tests/debugger/ @DataDog/dd-trace-js @DataDog/debugger +/packages/datadog-plugin-code-origin/ @DataDog/dd-trace-js @DataDog/debugger /packages/dd-trace/src/debugger/ @DataDog/dd-trace-js @DataDog/debugger /packages/dd-trace/test/debugger/ @DataDog/dd-trace-js @DataDog/debugger diff --git a/packages/datadog-instrumentations/src/fastify.js b/packages/datadog-instrumentations/src/fastify.js index 5849abc3056..726e8284f92 100644 --- a/packages/datadog-instrumentations/src/fastify.js +++ b/packages/datadog-instrumentations/src/fastify.js @@ -5,7 +5,7 @@ const { addHook, channel, AsyncResource } = require('./helpers/instrument') const errorChannel = channel('apm:fastify:middleware:error') const handleChannel = channel('apm:fastify:request:handle') -const codeOriginForSpansChannel = channel('datadog:code-origin-for-spans') +const routeAddedChannel = channel('apm:fastify:route:added') const parsingResources = new WeakMap() @@ -17,6 +17,7 @@ function wrapFastify (fastify, hasParsingEvents) { if (!app || typeof app.addHook !== 'function') return app + app.addHook('onRoute', onRoute) app.addHook('onRequest', onRequest) app.addHook('preHandler', preHandler) @@ -28,11 +29,6 @@ function wrapFastify (fastify, hasParsingEvents) { app.addHook('preHandler', preValidation) } - // No need to add the onRoute hook unless Code Origin for Spans is enabled - if (codeOriginForSpansChannel.hasSubscribers) { - app.addHook('onRoute', onRoute) - } - app.addHook = wrapAddHook(app.addHook) return app @@ -162,10 +158,7 @@ function publishError (error, req) { } function onRoute (routeOptions) { - codeOriginForSpansChannel.publish({ - routeOptions, - topOfStackFunc: onRoute - }) + routeAddedChannel.publish({ routeOptions, onRoute }) } addHook({ name: 'fastify', versions: ['>=3'] }, fastify => { diff --git a/packages/datadog-plugin-code-origin/src/fastify.js b/packages/datadog-plugin-code-origin/src/fastify.js new file mode 100644 index 00000000000..a67904206b6 --- /dev/null +++ b/packages/datadog-plugin-code-origin/src/fastify.js @@ -0,0 +1,20 @@ +'use strict' + +const { entryTag } = require('./tags') +const web = require('../../dd-trace/src/plugins/util/web') + +const kCodeOriginForSpansTagsSym = Symbol('datadog.codeOriginForSpansTags') + +module.exports = function (plugin) { + plugin.addSub('apm:fastify:request:handle', ({ req, routeConfig }) => { + const tags = routeConfig?.[kCodeOriginForSpansTagsSym] + if (!tags) return + const context = web.getContext(req) + context.span?.addTags(tags) + }) + + plugin.addSub('apm:fastify:route:added', ({ routeOptions, onRoute }) => { + if (!routeOptions.config) routeOptions.config = {} + routeOptions.config[kCodeOriginForSpansTagsSym] = entryTag(onRoute) + }) +} diff --git a/packages/datadog-plugin-code-origin/src/index.js b/packages/datadog-plugin-code-origin/src/index.js new file mode 100644 index 00000000000..4cefa74a89c --- /dev/null +++ b/packages/datadog-plugin-code-origin/src/index.js @@ -0,0 +1,17 @@ +'use strict' + +const fastify = require('./fastify') +const RouterPlugin = require('../../datadog-plugin-router/src') + +class CodeOriginForSpansPlugin extends RouterPlugin { + static get id () { + return 'code-origin-for-spans' + } + + constructor (...args) { + super(...args) + fastify(this) + } +} + +module.exports = CodeOriginForSpansPlugin diff --git a/packages/dd-trace/src/code_origin.js b/packages/datadog-plugin-code-origin/src/tags.js similarity index 90% rename from packages/dd-trace/src/code_origin.js rename to packages/datadog-plugin-code-origin/src/tags.js index cb829247163..a17ca112c3f 100644 --- a/packages/dd-trace/src/code_origin.js +++ b/packages/datadog-plugin-code-origin/src/tags.js @@ -1,6 +1,6 @@ 'use strict' -const { getUserLandFrames } = require('./plugins/util/stacktrace') +const { getUserLandFrames } = require('../../dd-trace/src/plugins/util/stacktrace') const limit = Number(process.env._DD_CODE_ORIGIN_MAX_USER_FRAMES) || 8 diff --git a/packages/datadog-plugin-fastify/src/index.js b/packages/datadog-plugin-fastify/src/index.js index 5b8e2811f64..6b4768279f8 100644 --- a/packages/datadog-plugin-fastify/src/index.js +++ b/packages/datadog-plugin-fastify/src/index.js @@ -1,10 +1,7 @@ 'use strict' -const { entryTag } = require('../../dd-trace/src/code_origin') const RouterPlugin = require('../../datadog-plugin-router/src') -const kCodeOriginForSpansTagsSym = Symbol('datadog.codeOriginForSpansTags') - class FastifyPlugin extends RouterPlugin { static get id () { return 'fastify' @@ -13,18 +10,9 @@ class FastifyPlugin extends RouterPlugin { constructor (...args) { super(...args) - this.addSub('apm:fastify:request:handle', ({ req, routeConfig }) => { + this.addSub('apm:fastify:request:handle', ({ req }) => { this.setFramework(req, 'fastify', this.config) - const tags = routeConfig?.[kCodeOriginForSpansTagsSym] - if (tags) this.setSpanTags(req, tags) }) - - if (this._tracerConfig.codeOriginForSpansEnabled) { - this.addSub('datadog:code-origin-for-spans', ({ routeOptions, topOfStackFunc }) => { - if (!routeOptions.config) routeOptions.config = {} - routeOptions.config[kCodeOriginForSpansTagsSym] = entryTag(topOfStackFunc) - }) - } } } diff --git a/packages/datadog-plugin-web/src/index.js b/packages/datadog-plugin-web/src/index.js index 605683fad74..688d5dc2781 100644 --- a/packages/datadog-plugin-web/src/index.js +++ b/packages/datadog-plugin-web/src/index.js @@ -15,10 +15,6 @@ class WebPlugin extends Plugin { setFramework (req, name, config) { web.setFramework(req, name, config) } - - setSpanTags (req, tags) { - web.setSpanTags(req, tags) - } } module.exports = WebPlugin diff --git a/packages/dd-trace/src/plugins/index.js b/packages/dd-trace/src/plugins/index.js index 660dfc1993c..b961327cf3c 100644 --- a/packages/dd-trace/src/plugins/index.js +++ b/packages/dd-trace/src/plugins/index.js @@ -27,6 +27,7 @@ module.exports = { get bunyan () { return require('../../../datadog-plugin-bunyan/src') }, get 'cassandra-driver' () { return require('../../../datadog-plugin-cassandra-driver/src') }, get child_process () { return require('../../../datadog-plugin-child_process/src') }, + get 'code-origin' () { return require('../../../datadog-plugin-code-origin/src') }, get connect () { return require('../../../datadog-plugin-connect/src') }, get couchbase () { return require('../../../datadog-plugin-couchbase/src') }, get cypress () { return require('../../../datadog-plugin-cypress/src') }, diff --git a/packages/dd-trace/src/plugins/util/web.js b/packages/dd-trace/src/plugins/util/web.js index 1e9c32589d2..c9cdf1990aa 100644 --- a/packages/dd-trace/src/plugins/util/web.js +++ b/packages/dd-trace/src/plugins/util/web.js @@ -68,15 +68,6 @@ const web = { web.setConfig(req, config) }, - setSpanTags (req, tags) { - const context = this.patch(req) - const span = context.span - - if (!span) return - - span.addTags(tags) - }, - setConfig (req, config) { const context = contexts.get(req) const span = context.span diff --git a/packages/dd-trace/test/plugins/util/web.spec.js b/packages/dd-trace/test/plugins/util/web.spec.js index 397838f4574..821d2d8d537 100644 --- a/packages/dd-trace/test/plugins/util/web.spec.js +++ b/packages/dd-trace/test/plugins/util/web.spec.js @@ -612,22 +612,6 @@ describe('plugins/util/web', () => { }) }) - describe('setSpanTags', () => { - it('should add expected tags', () => { - web.instrument(tracer, config, req, res, 'test.request', span => { - web.setSpanTags(req, { foo: 'bar' }) - - const tags = span.context()._tags - - res.end() - - expect(tags).to.include({ - foo: 'bar' - }) - }) - }) - }) - describe('enterRoute', () => { beforeEach(() => { config = web.normalizeConfig(config)