diff --git a/index.js b/index.js index 6885e29..a75af5e 100644 --- a/index.js +++ b/index.js @@ -44,6 +44,7 @@ const ANONYMOUS_FUNCTION_NAME = 'anonymous' const kInstrumentation = Symbol('fastify otel instance') const kRequestSpan = Symbol('fastify otel request spans') const kRequestContext = Symbol('fastify otel request context') +const kErrorStatusSet = Symbol('fastify otel error status set') const kAddHookOriginal = Symbol('fastify otel addhook original') const kSetNotFoundOriginal = Symbol('fastify otel setnotfound original') const kIgnorePaths = Symbol('fastify otel ignore path') @@ -284,6 +285,7 @@ class FastifyOtelInstrumentation extends InstrumentationBase { }) instance.decorateRequest(kRequestSpan, null) instance.decorateRequest(kRequestContext, null) + instance.decorateRequest(kErrorStatusSet, false) instance.addHook('onRoute', function otelWireRoute (routeOptions) { if (instrumentation[kIgnorePaths]?.(routeOptions) === true) { @@ -460,6 +462,7 @@ class FastifyOtelInstrumentation extends InstrumentationBase { } request[kRequestSpan] = null + request[kErrorStatusSet] = false hookDone() }) @@ -474,7 +477,11 @@ class FastifyOtelInstrumentation extends InstrumentationBase { const span = request[kRequestSpan] if (span != null) { - if (reply.statusCode >= 500) { + // setStatus replaces the status, so setting a bare ERROR here would + // drop the description recorded by recordErrorInSpanHook. A 5xx that + // never threw (e.g. reply.code(500).send()) has no description of its + // own, so it still needs the bare status. + if (reply.statusCode >= 500 && request[kErrorStatusSet] === false) { span.setStatus({ code: SpanStatusCode.ERROR }) } @@ -485,6 +492,7 @@ class FastifyOtelInstrumentation extends InstrumentationBase { } request[kRequestSpan] = null + request[kErrorStatusSet] = false hookDone(null, payload) } @@ -498,6 +506,7 @@ class FastifyOtelInstrumentation extends InstrumentationBase { code: SpanStatusCode.ERROR, message: error.message }) + request[kErrorStatusSet] = true if (instrumentation[kRecordExceptions] !== false) { span.recordException(error) } diff --git a/test/index.test.js b/test/index.test.js index 1cc052d..6596798 100644 --- a/test/index.test.js +++ b/test/index.test.js @@ -1151,6 +1151,104 @@ describe('FastifyInstrumentation', () => { assert.equal(response.status, 500) }) + test('should keep the error description on the request span (5xx)', async t => { + const app = Fastify() + const plugin = instrumentation.plugin() + + await app.register(plugin) + + app.get('/', async function helloworld () { + throw new Error('kaboom') + }) + + await app.listen() + + after(() => app.close()) + + const response = await fetch( + `http://localhost:${app.server.address().port}/` + ) + + const spans = memoryExporter + .getFinishedSpans() + .filter(span => span.instrumentationScope.name === '@fastify/otel') + + const handler = spans.find(span => span.name.startsWith('handler')) + const start = spans.find(span => span.name === 'request') + + assert.equal(spans.length, 2) + assert.equal(response.status, 500) + assert.deepStrictEqual(handler.status, { + code: SpanStatusCode.ERROR, + message: 'kaboom' + }) + assert.deepStrictEqual(start.status, { + code: SpanStatusCode.ERROR, + message: 'kaboom' + }) + }) + + test('should keep the error description on the request span (4xx)', async t => { + const app = Fastify() + const plugin = instrumentation.plugin() + + await app.register(plugin) + + app.get('/', async function helloworld () { + const error = new Error('kaboom') + error.statusCode = 400 + throw error + }) + + await app.listen() + + after(() => app.close()) + + const response = await fetch( + `http://localhost:${app.server.address().port}/` + ) + + const spans = memoryExporter + .getFinishedSpans() + .filter(span => span.instrumentationScope.name === '@fastify/otel') + + const start = spans.find(span => span.name === 'request') + + assert.equal(response.status, 400) + assert.deepStrictEqual(start.status, { + code: SpanStatusCode.ERROR, + message: 'kaboom' + }) + }) + + test('should set an error status on a 5xx reply that did not throw', async t => { + const app = Fastify() + const plugin = instrumentation.plugin() + + await app.register(plugin) + + app.get('/', async function helloworld (request, reply) { + return reply.code(500).send('not an exception') + }) + + await app.listen() + + after(() => app.close()) + + const response = await fetch( + `http://localhost:${app.server.address().port}/` + ) + + const spans = memoryExporter + .getFinishedSpans() + .filter(span => span.instrumentationScope.name === '@fastify/otel') + + const start = spans.find(span => span.name === 'request') + + assert.equal(response.status, 500) + assert.deepStrictEqual(start.status, { code: SpanStatusCode.ERROR }) + }) + test('should create named span (404)', async t => { const app = Fastify() const plugin = instrumentation.plugin()