[@opentelemetry/instrumentation-fastify] Breaks use of preClose with fastify

[SeanReece]

What version of OpenTelemetry are you using?

@opentelemetry/[email protected]
@opentelemetry/[email protected]

What version of Node are you using?

v20.9.0

What did you do?

When adding the hook preClose hook to fastify the application crashes when the preClose hook is called with this error:

TypeError: Cannot read properties of undefined (reading 'Symbol(opentelemetry.instrumentation.fastify.request_active_span)')
    at startSpan (/Users/me/src/test/node_modules/@opentelemetry/instrumentation-fastify/build/src/utils.js:30:24)
    at Object.<anonymous> (/Users/me/src/test/node_modules/@opentelemetry/instrumentation-fastify/build/src/instrumentation.js:74:48)
    at /Users/me/src/test/node_modules/fastify/lib/hooks.js:166:24
    at _encapsulateThreeParam (/Users/me/src/test/node_modules/avvio/boot.js:562:7)
    at Boot.timeoutCall (/Users/me/src/test/node_modules/avvio/boot.js:458:5)
    at Boot.callWithCbOrNextTick (/Users/me/src/test/node_modules/avvio/boot.js:440:19)
    at Object.push (/Users/me/src/test/node_modules/fastq/queue.js:110:14)
    at Boot.ready (/Users/me/src/test/node_modules/avvio/boot.js:336:18)
    at server.<computed> (/Users/me/src/test/node_modules/avvio/boot.js:59:21)
    at next (/Users/me/src/test/node_modules/fastify/lib/hooks.js:145:5)

onClose works as expected, but it appears that preClose needs to be added to the list of applicationHooks here:

opentelemetry-js-contrib/plugins/node/opentelemetry-instrumentation-fastify/src/constants.ts

Line 21 in 1b0caa6

export const applicationHookNames = [

Reproducible Example

const { NodeSDK } = require('@opentelemetry/sdk-node')
const { HttpInstrumentation } = require('@opentelemetry/instrumentation-http')
const { FastifyInstrumentation } = require('@opentelemetry/instrumentation-fastify')

const sdk = new NodeSDK({
  serviceName: 'testotel',
  instrumentations: [new HttpInstrumentation(), new FastifyInstrumentation()],
})

sdk.start()

const fastify = require('fastify')({
  logger: true,
})

// Declare a route
fastify.get('/', function (request, reply) {
  reply.send({ hello: 'world' })
})

fastify.addHook('preClose', () => {
  // This will never be called
  console.log('Closing')
})

fastify.listen({ port: 3000 }, function (err, address) {
  if (err) {
    fastify.log.error(err)
    process.exit(1)
  }
  // Lets close right away to trigger preClose
  fastify.close()
})

What did you expect to see?

I expect preClose should work as expected.

What did you see instead?

Application crashes when preClose is called

Additional context

[dyladan]

Code owner @pichlermarc is out today but please look when you get back

[trentm]

I can take this one, if you like.

In addition to skipping wrapping preClose, I think the instrumentation should also skip wrapping onRequestAbort (https://fastify.dev/docs/latest/Reference/Hooks#onrequestabort) because it doesn't receive the (request, reply, ...) arguments that _wrapHandler expects.

Initially I didn't notice the existing applicationHookNames guard and implemented my own guard -- but I made an allow-list, rather than the applicationHookNames deny-list. @pichlermarc Would you like a patch that changes to an allow-list of hook names to wrap? We could add onRequestAbort to the deny-list -- and perhaps rename that constant because onRequestAbort isn't an "application" hook.