-
Notifications
You must be signed in to change notification settings - Fork 304
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DEBUG-2972] Add initial support for Code Origin for Spans (Fastify entry spans only) #4449
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +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 parsingResources = new WeakMap() | ||
|
||
|
@@ -27,6 +28,11 @@ 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be in the hook instead because subscribers can be enabled/disabled at runtime. |
||
app.addHook('onRoute', onRoute) | ||
} | ||
|
||
app.addHook = wrapAddHook(app.addHook) | ||
|
||
return app | ||
|
@@ -86,8 +92,9 @@ function onRequest (request, reply, done) { | |
|
||
const req = getReq(request) | ||
const res = getRes(reply) | ||
const routeConfig = getRouteConfig(request) | ||
|
||
handleChannel.publish({ req, res }) | ||
handleChannel.publish({ req, res, routeConfig }) | ||
|
||
return done() | ||
} | ||
|
@@ -142,6 +149,10 @@ function getRes (reply) { | |
return reply && (reply.raw || reply.res || reply) | ||
} | ||
|
||
function getRouteConfig (request) { | ||
return request?.routeOptions?.config | ||
} | ||
|
||
function publishError (error, req) { | ||
if (error) { | ||
errorChannel.publish({ error, req }) | ||
|
@@ -150,6 +161,13 @@ function publishError (error, req) { | |
return error | ||
} | ||
|
||
function onRoute (routeOptions) { | ||
codeOriginForSpansChannel.publish({ | ||
routeOptions, | ||
topOfStackFunc: onRoute | ||
}) | ||
} | ||
|
||
addHook({ name: 'fastify', versions: ['>=3'] }, fastify => { | ||
const wrapped = shimmer.wrapFunction(fastify, fastify => wrapFastify(fastify, true)) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,10 @@ | ||
'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' | ||
|
@@ -10,9 +13,18 @@ class FastifyPlugin extends RouterPlugin { | |
constructor (...args) { | ||
super(...args) | ||
|
||
this.addSub('apm:fastify:request:handle', ({ req }) => { | ||
this.addSub('apm:fastify:request:handle', ({ req, routeConfig }) => { | ||
this.setFramework(req, 'fastify', this.config) | ||
const tags = routeConfig?.[kCodeOriginForSpansTagsSym] | ||
if (tags) this.setSpanTags(req, tags) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could this be done in a separate plugin instead? The reasoning is that adding product-specific code directly to plugin increases size and complexity even when those products are not used, whereas we could disable product-specific plugins entirely and not even import them when a product is disabled for example. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would actually also remove the need to modify |
||
}) | ||
|
||
if (this._tracerConfig.codeOriginForSpansEnabled) { | ||
this.addSub('datadog:code-origin-for-spans', ({ routeOptions, topOfStackFunc }) => { | ||
if (!routeOptions.config) routeOptions.config = {} | ||
routeOptions.config[kCodeOriginForSpansTagsSym] = entryTag(topOfStackFunc) | ||
}) | ||
} | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,8 @@ describe('Plugin', () => { | |
|
||
describe('fastify', () => { | ||
withVersions('fastify', 'fastify', (version, _, specificVersion) => { | ||
if (NODE_MAJOR <= 18 && semver.satisfies(specificVersion, '>=5')) return | ||
|
||
beforeEach(() => { | ||
tracer = require('../../dd-trace') | ||
}) | ||
|
@@ -26,14 +28,12 @@ describe('Plugin', () => { | |
|
||
withExports('fastify', version, ['default', 'fastify'], '>=3', getExport => { | ||
describe('without configuration', () => { | ||
if (NODE_MAJOR <= 18 && semver.satisfies(specificVersion, '>=5')) return | ||
|
||
before(() => { | ||
return agent.load(['fastify', 'find-my-way', 'http'], [{}, {}, { client: false }]) | ||
}) | ||
|
||
after(() => { | ||
return agent.close({ ritmReset: false }) | ||
return agent.close({ ritmReset: false, wipe: true }) | ||
}) | ||
|
||
beforeEach(() => { | ||
|
@@ -542,7 +542,193 @@ describe('Plugin', () => { | |
}) | ||
} | ||
}) | ||
|
||
describe('with tracer config codeOriginForSpansEnabled', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this name Node-specific or has it been standardized? Mostly asking because it exposes implementation details in the name. |
||
if (semver.satisfies(specificVersion, '<4')) return // TODO: Why doesn't it work on older versions? | ||
|
||
before(() => { | ||
return agent.load( | ||
['fastify', 'find-my-way', 'http'], | ||
[{}, {}, { client: false }], | ||
{ codeOriginForSpansEnabled: true } | ||
) | ||
}) | ||
|
||
after(() => { | ||
return agent.close({ ritmReset: false, wipe: true }) | ||
}) | ||
|
||
beforeEach(() => { | ||
fastify = getExport() | ||
app = fastify() | ||
|
||
if (semver.intersects(version, '>=3')) { | ||
return app.register(require('../../../versions/middie').get()) | ||
} | ||
}) | ||
|
||
it('should add code_origin tag on entry spans when feature is enabled', done => { | ||
let routeRegisterLine | ||
|
||
// Wrap in a named function to have at least one frame with a function name | ||
function wrapperFunction () { | ||
routeRegisterLine = getNextLineNumber() | ||
app.get('/user', function userHandler (request, reply) { | ||
reply.send() | ||
}) | ||
} | ||
|
||
const callWrapperLine = getNextLineNumber() | ||
wrapperFunction() | ||
|
||
app.listen(() => { | ||
const port = app.server.address().port | ||
|
||
agent | ||
.use(traces => { | ||
const spans = traces[0] | ||
const tags = spans[0].meta | ||
|
||
expect(tags).to.have.property('_dd.code_origin.type', 'entry') | ||
|
||
expect(tags).to.have.property('_dd.code_origin.frames.0.file', __filename) | ||
expect(tags).to.have.property('_dd.code_origin.frames.0.line', routeRegisterLine) | ||
expect(tags).to.have.property('_dd.code_origin.frames.0.method', 'wrapperFunction') | ||
expect(tags).to.not.have.property('_dd.code_origin.frames.0.type') | ||
|
||
expect(tags).to.have.property('_dd.code_origin.frames.1.file', __filename) | ||
expect(tags).to.have.property('_dd.code_origin.frames.1.line', callWrapperLine) | ||
expect(tags).to.not.have.property('_dd.code_origin.frames.1.method') | ||
expect(tags).to.have.property('_dd.code_origin.frames.1.type', 'Context') | ||
|
||
expect(tags).to.not.have.property('_dd.code_origin.frames.2.file') | ||
}) | ||
.then(done) | ||
.catch(done) | ||
|
||
axios | ||
.get(`http://localhost:${port}/user`) | ||
.catch(done) | ||
}) | ||
}) | ||
|
||
it('should point to where actual route handler is configured, not the prefix', done => { | ||
let routeRegisterLine | ||
|
||
app.register(function v1Handler (app, opts, done) { | ||
routeRegisterLine = getNextLineNumber() | ||
app.get('/user', function userHandler (request, reply) { | ||
reply.send() | ||
}) | ||
done() | ||
}, { prefix: '/v1' }) | ||
|
||
app.listen(() => { | ||
const port = app.server.address().port | ||
|
||
agent | ||
.use(traces => { | ||
const spans = traces[0] | ||
const tags = spans[0].meta | ||
|
||
expect(tags).to.have.property('_dd.code_origin.type', 'entry') | ||
|
||
expect(tags).to.have.property('_dd.code_origin.frames.0.file', __filename) | ||
expect(tags).to.have.property('_dd.code_origin.frames.0.line', routeRegisterLine) | ||
expect(tags).to.have.property('_dd.code_origin.frames.0.method', 'v1Handler') | ||
expect(tags).to.not.have.property('_dd.code_origin.frames.0.type') | ||
|
||
expect(tags).to.not.have.property('_dd.code_origin.frames.1.file') | ||
}) | ||
.then(done) | ||
.catch(done) | ||
|
||
axios | ||
.get(`http://localhost:${port}/v1/user`) | ||
.catch(done) | ||
}) | ||
}) | ||
|
||
it('should point to route handler even if passed through a middleware', function testCase (done) { | ||
app.use(function middleware (req, res, next) { | ||
next() | ||
}) | ||
|
||
const routeRegisterLine = getNextLineNumber() | ||
app.get('/user', function userHandler (request, reply) { | ||
reply.send() | ||
}) | ||
|
||
app.listen({ host, port: 0 }, () => { | ||
const port = app.server.address().port | ||
|
||
agent | ||
.use(traces => { | ||
const spans = traces[0] | ||
const tags = spans[0].meta | ||
|
||
expect(tags).to.have.property('_dd.code_origin.type', 'entry') | ||
|
||
expect(tags).to.have.property('_dd.code_origin.frames.0.file', __filename) | ||
expect(tags).to.have.property('_dd.code_origin.frames.0.line', routeRegisterLine) | ||
expect(tags).to.have.property('_dd.code_origin.frames.0.method', 'testCase') | ||
expect(tags).to.have.property('_dd.code_origin.frames.0.type', 'Context') | ||
|
||
expect(tags).to.not.have.property('_dd.code_origin.frames.1.file') | ||
}) | ||
.then(done) | ||
.catch(done) | ||
|
||
axios | ||
.get(`http://localhost:${port}/user`) | ||
.catch(done) | ||
}) | ||
}) | ||
|
||
// TODO: In Fastify, the route is resolved before the middleware is called, so we actually can get the line | ||
// number of where the route handler is defined. However, this might not be the right choice and it might be | ||
// better to point to the middleware. | ||
it.skip('should point to middleware if middleware responds early', function testCase (done) { | ||
const middlewareRegisterLine = getNextLineNumber() | ||
app.use(function middleware (req, res, next) { | ||
res.end() | ||
}) | ||
|
||
app.get('/user', function userHandler (request, reply) { | ||
reply.send() | ||
}) | ||
|
||
app.listen({ host, port: 0 }, () => { | ||
const port = app.server.address().port | ||
|
||
agent | ||
.use(traces => { | ||
const spans = traces[0] | ||
const tags = spans[0].meta | ||
|
||
expect(tags).to.have.property('_dd.code_origin.type', 'entry') | ||
|
||
expect(tags).to.have.property('_dd.code_origin.frames.0.file', __filename) | ||
expect(tags).to.have.property('_dd.code_origin.frames.0.line', middlewareRegisterLine) | ||
expect(tags).to.have.property('_dd.code_origin.frames.0.method', 'testCase') | ||
expect(tags).to.have.property('_dd.code_origin.frames.0.type', 'Context') | ||
|
||
expect(tags).to.not.have.property('_dd.code_origin.frames.1.file') | ||
}) | ||
.then(done) | ||
.catch(done) | ||
|
||
axios | ||
.get(`http://localhost:${port}/user`) | ||
.catch(done) | ||
}) | ||
}) | ||
}) | ||
}) | ||
}) | ||
}) | ||
}) | ||
|
||
function getNextLineNumber () { | ||
return String(Number(new Error().stack.split('\n')[2].match(/:(\d+):/)[1]) + 1) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
'use strict' | ||
|
||
const { getUserLandFrames } = require('./plugins/util/stacktrace') | ||
|
||
const limit = Number(process.env._DD_CODE_ORIGIN_MAX_USER_FRAMES) || 8 | ||
|
||
module.exports = { | ||
entryTag, | ||
exitTag | ||
} | ||
|
||
function entryTag (topOfStackFunc) { | ||
return tag('entry', topOfStackFunc) | ||
} | ||
|
||
function exitTag (topOfStackFunc) { | ||
return tag('exit', topOfStackFunc) | ||
} | ||
|
||
function tag (type, topOfStackFunc) { | ||
const frames = getUserLandFrames(topOfStackFunc, limit) | ||
const tags = { | ||
'_dd.code_origin.type': type | ||
} | ||
for (let i = 0; i < frames.length; i++) { | ||
const frame = frames[i] | ||
tags[`_dd.code_origin.frames.${i}.file`] = frame.file | ||
tags[`_dd.code_origin.frames.${i}.line`] = String(frame.line) | ||
if (frame.method) { | ||
tags[`_dd.code_origin.frames.${i}.method`] = frame.method | ||
} | ||
if (frame.type) { | ||
tags[`_dd.code_origin.frames.${i}.type`] = frame.type | ||
} | ||
} | ||
return tags | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There shouldn't be explicit references to tracing in instrumentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, channels are easier to reason about if they match the event that they represent. For example, different subscribers may be interested in different aspects of routing, so it would be better to have something like a routeChannel instead which would add the route when any subscriber is interested in that.