Skip to content

fix: Fixed error reporting from async Fastify handlers #3100

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
41 changes: 41 additions & 0 deletions bin/trace-to-segment-tree.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
#!/usr/bin/env node
/*
* Copyright 2025 New Relic Corporation. All rights reserved.
* SPDX-License-Identifier: Apache-2.0
*/

'use strict'

// The purpose of this script is to reduce a trace's JSON representation into
// a segment representation that will pass
// `./test/lib/custom-assertions/assert-segments`. To utilize this tool:
//
// 1. Add a debug point at which you would try to compare the current
// transaction's segments.
// 2. Utilizing the debugger, inspect `transaction.trace.toJSON()`.
// 3. Copy the result.
// 4. Write the result to a JSON file, e.g. `/tmp/trace.json`.
// 5. Run this tool and provide the JSON file as the sole parameter.

process.exitCode = 1

if (process.argv.length !== 3) {
console.error('Missing input file. Script should be run like:\n')
console.error(' trace-to-segment-tree.js /path/to/some.json')
process.exit()
}

let input
try {
input = require(process.argv[2])
} catch (error) {
console.error('Could not parse input JSON. It must be a trace array.')
console.error(error)
process.exit()
}

const reduced = require('../test/lib/trace-to-segment-tree.js')(input)
console.log(
JSON.stringify(reduced, null, 2)
)
process.exitCode = 0
4 changes: 3 additions & 1 deletion lib/instrumentation/fastify/spec-builders.js
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ function buildMiddlewareSpecForRouteHandler(shim, path) {
},
params: getParamsFromFastifyRequest,
req: getRequestFromFastify,
promise: true,
route: path
})
}
Expand All @@ -113,7 +114,8 @@ function buildMiddlewareSpecForMiddlewareFunction(shim, name, route) {
next: shim.LAST,
params: getParamsFromFastifyRequest,
req: getRequestFromFastify,
type: shim.MIDDLEWARE
type: shim.MIDDLEWARE,
promise: true
})
}

Expand Down
30 changes: 12 additions & 18 deletions test/lib/custom-assertions/assert-segments.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,47 +68,41 @@
* @param {Array} expected Array of strings that represent segment names.
* If an item in the array is another array, it
* represents children of the previous item.
* @param {boolean} options.exact If true, then the expected segments must match
* @param {object} [options] Set of options for the assertion algorithm.
* @param {boolean} [options.exact] If true, then the expected segments must match
* exactly, including their position and children on all
* levels. When false, then only check that each child
* exists.
* @param {array} options.exclude Array of segment names that should be excluded from
* exists. Default: true.
* @param {array} [options.exclude] Array of segment names that should be excluded from
* validation. This is useful, for example, when a
* segment may or may not be created by code that is not
* directly under test. Only used when `exact` is true.
* Default: empty array.
* @param {object} [deps] Injected dependencies.
* @param {object} [deps.assert] Assertion library to use.
*/
module.exports = function assertSegments( // eslint-disable-line sonarjs/cognitive-complexity
trace,
parent,
expected,
options,
{ exact, exclude } = { exact: true, exclude: [] },
{ assert = require('node:assert') } = {}
) {
let child
let childCount = 0

// rather default to what is more likely to fail than have a false test
let exact = true
if (options && options.exact === false) {
exact = options.exact
} else if (options === false) {
exact = false
}

function getChildren(_parent) {
const children = trace.getChildren(_parent.id)
return children.filter(function (item) {
if (exact && options && options.exclude) {
return options.exclude.indexOf(item.name) === -1
if (exact && exclude) {
return exclude.indexOf(item.name) === -1
}
return true
})
}

const children = getChildren(parent)
if (exact) {
if (exact === true) {
for (let i = 0; i < expected.length; ++i) {
const sequenceItem = expected[i]

Expand All @@ -125,16 +119,16 @@ module.exports = function assertSegments( // eslint-disable-line sonarjs/cogniti
childCount
)

// If the next expected item is not array, then check that the current
// child has no children
// If the next expected item is not an array, then check that the
// current child has no children.
if (!Array.isArray(expected[i + 1])) {
assert.ok(
getChildren(child).length === 0,
'segment "' + child.name + '" should not have any children'
)
}
} else if (typeof sequenceItem === 'object') {
assertSegments(trace, child, sequenceItem, options, { assert })
assertSegments(trace, child, sequenceItem, { exact, exclude }, { assert })
}
}

Expand Down
34 changes: 34 additions & 0 deletions test/lib/trace-to-segment-tree.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/*
* Copyright 2025 New Relic Corporation. All rights reserved.
* SPDX-License-Identifier: Apache-2.0
*/

'use strict'

/**
* Given the JSON representation of a transaction's trace, reduce it to
* a tree of named segments.
* @param {object} traceJSON An array based tree representation of a trace.
* Use `transaction.trace.toJSON()` to get said tree.
* @param {object} [options]
* @param {boolean} [options.excludeRoot] Indicates if the root segment
* name should be excluded.
*
* @returns {*[]}
*/
module.exports = function traceToSegmentTree(traceJSON, { excludeRoot = true } = {}) {
const filtered = []
for (const ele of traceJSON) {
if (typeof ele === 'string') {
if (ele === 'ROOT' && excludeRoot === true) continue
filtered.push(ele)
continue
}

if (Array.isArray(ele) === true) {
const toAdd = traceToSegmentTree(ele, { excludeRoot })
filtered.push(toAdd)
}
}
return filtered
}
77 changes: 16 additions & 61 deletions test/versioned/fastify/add-hook.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@ const test = require('node:test')
const assert = require('node:assert')

const { removeModules } = require('../../lib/cache-buster')
const { assertSegments } = require('../../lib/custom-assertions')
const helper = require('../../lib/agent_helper')
const common = require('./common')
const traceToSegmentTree = require('../../lib/trace-to-segment-tree')

// all of these events fire before the route handler
// See: https://www.fastify.io/docs/latest/Lifecycle/
Expand All @@ -30,17 +30,10 @@ const AFTER_TX_HOOKS = ['onResponse']

const ALL_HOOKS = [...REQUEST_HOOKS, ...AFTER_HANDLER_HOOKS, ...AFTER_TX_HOOKS]

/**
* Helper to return the list of expected segments
*
* @param {Array} hooks lifecyle hook names to build segment names from
* @returns {Array} formatted list of expected segments
*/
function getExpectedSegments(hooks) {
return hooks.map((hookName) => {
return `Nodejs/Middleware/Fastify/${hookName}/testHook`
})
}
const nonErrorHooksSegmentsSnap = require('./snapshots/non-error-hooks-segments.json')
const nonErrorHooksSegmentsSecAgentSnap = require('./snapshots/non-error-hooks-segments-secagent.json')
const errorHooksSegmentsSnap = require('./snapshots/error-hooks-segments.json')
const errorHooksSegmentsSegAgentSnap = require('./snapshots/error-hooks-segments-secagent.json')

test.beforeEach((ctx) => {
ctx.nr = {}
Expand Down Expand Up @@ -83,32 +76,12 @@ test('non-error hooks', async (t) => {
transaction.getName(),
'transaction name matched'
)
// all the hooks are siblings of the route handler
// except the AFTER_HANDLER_HOOKS which are children of the route handler
let expectedSegments
if (helper.isSecurityAgentEnabled(agent)) {
expectedSegments = [
'WebTransaction/WebFrameworkUri/Fastify/GET//add-hook',
[
'Nodejs/Middleware/Fastify/onRequest/<anonymous>',
[
...getExpectedSegments(REQUEST_HOOKS),
'Nodejs/Middleware/Fastify/routeHandler//add-hook',
getExpectedSegments(AFTER_HANDLER_HOOKS)
]
]
]
} else {
expectedSegments = [
'WebTransaction/WebFrameworkUri/Fastify/GET//add-hook',
[
...getExpectedSegments(REQUEST_HOOKS),
'Nodejs/Middleware/Fastify/routeHandler//add-hook',
getExpectedSegments(AFTER_HANDLER_HOOKS)
]
]
}
assertSegments(transaction.trace, transaction.trace.root, expectedSegments)

const expectedSegments = helper.isSecurityAgentEnabled(agent)
? nonErrorHooksSegmentsSecAgentSnap
: nonErrorHooksSegmentsSnap
const actualSegments = traceToSegmentTree(transaction.trace.toJSON())
assert.deepStrictEqual(actualSegments, expectedSegments)

txPassed = true
})
Expand Down Expand Up @@ -146,29 +119,11 @@ test('error hook', async function errorHookTest(t) {
'transaction name matched'
)
// all the hooks are siblings of the route handler
let expectedSegments
if (helper.isSecurityAgentEnabled(agent)) {
expectedSegments = [
'WebTransaction/WebFrameworkUri/Fastify/GET//error',
[
'Nodejs/Middleware/Fastify/onRequest/<anonymous>',
[
'Nodejs/Middleware/Fastify/errorRoute//error',
[`Nodejs/Middleware/Fastify/${hookName}/testHook`]
]
]
]
} else {
expectedSegments = [
'WebTransaction/WebFrameworkUri/Fastify/GET//error',
[
'Nodejs/Middleware/Fastify/errorRoute//error',
[`Nodejs/Middleware/Fastify/${hookName}/testHook`]
]
]
}

assertSegments(transaction.trace, transaction.trace.root, expectedSegments)
const expectedSegments = helper.isSecurityAgentEnabled(agent)
? errorHooksSegmentsSegAgentSnap
: errorHooksSegmentsSnap
const foundSegments = traceToSegmentTree(transaction.trace.toJSON())
assert.deepStrictEqual(foundSegments, expectedSegments)

txPassed = true
})
Expand Down
2 changes: 1 addition & 1 deletion test/versioned/fastify/common.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ common.setupRoutes = (fastify) => {
}, {})

/**
* Registeres a named route handler for testing adding hooks for
* Registers a named route handler for testing adding hooks for
* every request lifecycle
*/
fastify.get('/add-hook', function routeHandler(request, reply) {
Expand Down
Loading