Skip to content
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.

Sign up for GitHub

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

fix: Updated koa instrumentation to properly get the matched route name and to handle changes in @koa/[email protected] #2486

Merged
merged 1 commit into from
Aug 16, 2024
Merged

fix: Updated koa instrumentation to properly get the matched route name and to handle changes in @koa/[email protected] #2486

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 2 additions & 20 deletions lib/instrumentation/koa/instrumentation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,10 +115,8 @@ function wrapMatchedRoute(shim, context) {
Object.defineProperty(context, '_matchedRoute', {
get: () => context[symbols.koaMatchedRoute],
set: (val) => {
const match = getLayerForTransactionName(context)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no reason to find the layer as this setter passes in which layer was matched


// match should never be undefined given _matchedRoute was set
if (match) {
if (val) {
const currentSegment = shim.getActiveSegment()

// Segment/Transaction may be null, see:
Expand All @@ -131,7 +129,7 @@ function wrapMatchedRoute(shim, context) {
transaction.nameState.popPath()
}

transaction.nameState.appendPath(match.path)
transaction.nameState.appendPath(val)
transaction.nameState.markPath()
}
}
Expand Down Expand Up @@ -169,22 +167,6 @@ function wrapResponseStatus(shim, context) {
})
}

function getLayerForTransactionName(context) {
// Context.matched might be null
// See https://github.com/newrelic/node-newrelic-koa/pull/29
if (!context.matched) {
return null
}
for (let i = context.matched.length - 1; i >= 0; i--) {
const layer = context.matched[i]
if (layer.opts.end) {
return layer
}
}

return null
}

function getInheritedPropertyDescriptor(obj, property) {
let proto = obj
let descriptor = null
Expand Down
2 changes: 1 addition & 1 deletion test/versioned/koa/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@
"samples": 5
},
"@koa/router": {
"versions": ">=11.0.2 <13.0.0",
"versions": ">=11.0.2",
"samples": 5
}
},
Expand Down
37 changes: 18 additions & 19 deletions test/versioned/koa/router-common.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,7 +422,7 @@ module.exports = (pkg) => {
)
})

t.test('using multipler routers', (t) => {
t.test('using multiple routers', (t) => {
t.beforeEach(testSetup)
t.afterEach(tearDown)
t.autoend()
Expand Down Expand Up @@ -546,23 +546,20 @@ module.exports = (pkg) => {
nestedRouter.get('/:second', function terminalMiddleware(ctx) {
ctx.body = 'this is a test'
})
nestedRouter.get('/second', function secondMiddleware(ctx) {
Copy link
Member Author

@bizob2828 bizob2828 Aug 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this because this middleware is never called but we had logic in lib/instrumentation/koa/instrumentation.js that was using this layer route name to name the transaction which was incorrect. I was also able to simplify that logic because we can just use what's being passed in to assign to the transaction namestate path

ctx.body = 'want this to set the name'
})
router.use('/:first', nestedRouter.routes())
app.use(router.routes())

agent.on('transactionFinished', (tx) => {
t.assertSegments(tx.trace.root, [
'WebTransaction/WebFrameworkUri/Koa/GET//:first/second',
'WebTransaction/WebFrameworkUri/Koa/GET//:first/:second',
[
'Nodejs/Middleware/Koa/appLevelMiddleware',
['Koa/Router: /', [getNestedSpanName('terminalMiddleware')]]
]
])
t.equal(
tx.name,
'WebTransaction/WebFrameworkUri/Koa/GET//:first/second',
'WebTransaction/WebFrameworkUri/Koa/GET//:first/:second',
'should be named after last matched route'
)
t.end()
Expand All @@ -581,23 +578,20 @@ module.exports = (pkg) => {
router.get('/:second', function terminalMiddleware(ctx) {
ctx.body = 'this is a test'
})
router.get('/second', function secondMiddleware(ctx) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is where that test was introduced: newrelic/node-newrelic-koa@4493aab but like i said before it's not actually hitting this handler

ctx.body = 'want this to set the name'
})
router.prefix('/:first')
app.use(router.routes())

agent.on('transactionFinished', (tx) => {
t.assertSegments(tx.trace.root, [
'WebTransaction/WebFrameworkUri/Koa/GET//:first/second',
'WebTransaction/WebFrameworkUri/Koa/GET//:first/:second',
[
'Nodejs/Middleware/Koa/appLevelMiddleware',
['Koa/Router: /', ['Nodejs/Middleware/Koa/terminalMiddleware//:first/:second']]
]
])
t.equal(
tx.name,
'WebTransaction/WebFrameworkUri/Koa/GET//:first/second',
'WebTransaction/WebFrameworkUri/Koa/GET//:first/:second',
'should be named after the last matched path'
)
t.end()
Expand All @@ -607,6 +601,11 @@ module.exports = (pkg) => {
})

t.test('using allowedMethods', (t) => {
// `@koa/[email protected]` changed the allowedMethods middleware function from named to arrow function
// update span name for assertions
Comment on lines +604 to +605
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonderful change. Who would want to see function names in stack traces?

const allowedMethodsFnName = semver.gte(pkgVersion, '13.0.0')
? '<anonymous>'
: 'allowedMethods'
t.autoend()

t.test('with throw: true', (t) => {
Expand All @@ -622,7 +621,7 @@ module.exports = (pkg) => {
agent.on('transactionFinished', (tx) => {
t.assertSegments(tx.trace.root, [
'WebTransaction/WebFrameworkUri/Koa/GET/(method not allowed)',
['Koa/Router: /', ['Nodejs/Middleware/Koa/allowedMethods']]
['Koa/Router: /', [`Nodejs/Middleware/Koa/${allowedMethodsFnName}`]]
])
t.equal(
tx.name,
Expand All @@ -645,7 +644,7 @@ module.exports = (pkg) => {
agent.on('transactionFinished', (tx) => {
t.assertSegments(tx.trace.root, [
'WebTransaction/WebFrameworkUri/Koa/GET/(not implemented)',
['Koa/Router: /', ['Nodejs/Middleware/Koa/allowedMethods']]
['Koa/Router: /', [`Nodejs/Middleware/Koa/${allowedMethodsFnName}`]]
])
t.equal(
tx.name,
Expand Down Expand Up @@ -683,7 +682,7 @@ module.exports = (pkg) => {
'WebTransaction/NormalizedUri/*',
[
'Nodejs/Middleware/Koa/errorHandler',
['Koa/Router: /', ['Nodejs/Middleware/Koa/allowedMethods']]
['Koa/Router: /', [`Nodejs/Middleware/Koa/${allowedMethodsFnName}`]]
]
])
t.equal(
Expand Down Expand Up @@ -722,7 +721,7 @@ module.exports = (pkg) => {
'WebTransaction/WebFrameworkUri/Koa/GET/(method not allowed)',
[
'Nodejs/Middleware/Koa/baseMiddleware',
['Koa/Router: /', ['Nodejs/Middleware/Koa/allowedMethods']]
['Koa/Router: /', [`Nodejs/Middleware/Koa/${allowedMethodsFnName}`]]
]
])
t.equal(
Expand Down Expand Up @@ -753,7 +752,7 @@ module.exports = (pkg) => {
agent.on('transactionFinished', (tx) => {
t.assertSegments(tx.trace.root, [
'WebTransaction/WebFrameworkUri/Koa/GET/(method not allowed)',
['Koa/Router: /', ['Nodejs/Middleware/Koa/allowedMethods']]
['Koa/Router: /', [`Nodejs/Middleware/Koa/${allowedMethodsFnName}`]]
])
t.equal(
tx.name,
Expand All @@ -777,7 +776,7 @@ module.exports = (pkg) => {
agent.on('transactionFinished', (tx) => {
t.assertSegments(tx.trace.root, [
'WebTransaction/WebFrameworkUri/Koa/GET/(not implemented)',
['Koa/Router: /', ['Nodejs/Middleware/Koa/allowedMethods']]
['Koa/Router: /', [`Nodejs/Middleware/Koa/${allowedMethodsFnName}`]]
])
t.equal(
tx.name,
Expand Down Expand Up @@ -811,7 +810,7 @@ module.exports = (pkg) => {
'WebTransaction/WebFrameworkUri/Koa/GET/(method not allowed)',
[
'Nodejs/Middleware/Koa/appLevelMiddleware',
['Koa/Router: /', ['Nodejs/Middleware/Koa/allowedMethods']]
['Koa/Router: /', [`Nodejs/Middleware/Koa/${allowedMethodsFnName}`]]
]
])
t.equal(
Expand Down Expand Up @@ -845,7 +844,7 @@ module.exports = (pkg) => {
'WebTransaction/WebFrameworkUri/Koa/GET/(not implemented)',
[
'Nodejs/Middleware/Koa/appLevelMiddleware',
['Koa/Router: /', ['Nodejs/Middleware/Koa/allowedMethods']]
['Koa/Router: /', [`Nodejs/Middleware/Koa/${allowedMethodsFnName}`]]
]
])
t.equal(
Expand Down
Loading