From 4ca18626610c0ee3da38807da82c753b8763af95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rapha=C3=ABl=20Th=C3=A9riault?= <113933910+raphael-theriault-swi@users.noreply.github.com> Date: Thu, 30 Nov 2023 14:42:07 -0800 Subject: [PATCH 1/3] feat(express): record exceptions (#1657) --- .../src/instrumentation.ts | 64 ++++++++++--- .../src/utils.ts | 13 +++ .../test/express.test.ts | 90 ++++++++++++++++++- .../test/utils.test.ts | 23 +++++ 4 files changed, 176 insertions(+), 14 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts index c476649750..61d91b55e6 100644 --- a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts @@ -15,12 +15,23 @@ */ import { getRPCMetadata, RPCType } from '@opentelemetry/core'; -import { trace, context, diag, SpanAttributes } from '@opentelemetry/api'; +import { + trace, + context, + diag, + SpanAttributes, + SpanStatusCode, +} from '@opentelemetry/api'; import type * as express from 'express'; import { ExpressInstrumentationConfig, ExpressRequestInfo } from './types'; import { ExpressLayerType } from './enums/ExpressLayerType'; import { AttributeNames } from './enums/AttributeNames'; -import { getLayerMetadata, storeLayerPath, isLayerIgnored } from './utils'; +import { + asErrorAndMessage, + getLayerMetadata, + isLayerIgnored, + storeLayerPath, +} from './utils'; import { VERSION } from './version'; import { InstrumentationBase, @@ -176,6 +187,7 @@ export class ExpressInstrumentation extends InstrumentationBase< layer[kLayerPatched] = true; this._wrap(layer, 'handle', (original: Function) => { + // TODO: instrument error handlers if (original.length === 4) return original; return function ( this: ExpressLayer, @@ -262,29 +274,55 @@ export class ExpressInstrumentation extends InstrumentationBase< const callbackIdx = args.findIndex(arg => typeof arg === 'function'); if (callbackIdx >= 0) { arguments[callbackIdx] = function () { + // express considers anything but an empty value, "route" or "router" + // passed to its callback to be an error + const maybeError = arguments[0]; + const isError = ![undefined, null, 'route', 'router'].includes( + maybeError + ); + if (isError) { + const [error, message] = asErrorAndMessage(maybeError); + span.recordException(error); + span.setStatus({ + code: SpanStatusCode.ERROR, + message, + }); + } + if (spanHasEnded === false) { spanHasEnded = true; req.res?.removeListener('finish', onResponseFinish); span.end(); } - if (!(req.route && arguments[0] instanceof Error)) { + if (!(req.route && isError)) { (req[_LAYERS_STORE_PROPERTY] as string[]).pop(); } const callback = args[callbackIdx] as Function; return callback.apply(this, arguments); }; } - const result = original.apply(this, arguments); - /** - * At this point if the callback wasn't called, that means either the - * layer is asynchronous (so it will call the callback later on) or that - * the layer directly end the http response, so we'll hook into the "finish" - * event to handle the later case. - */ - if (!spanHasEnded) { - res.once('finish', onResponseFinish); + + try { + return original.apply(this, arguments); + } catch (anyError) { + const [error, message] = asErrorAndMessage(anyError); + span.recordException(error); + span.setStatus({ + code: SpanStatusCode.ERROR, + message, + }); + throw anyError; + } finally { + /** + * At this point if the callback wasn't called, that means either the + * layer is asynchronous (so it will call the callback later on) or that + * the layer directly end the http response, so we'll hook into the "finish" + * event to handle the later case. + */ + if (!spanHasEnded) { + res.once('finish', onResponseFinish); + } } - return result; }; }); } diff --git a/plugins/node/opentelemetry-instrumentation-express/src/utils.ts b/plugins/node/opentelemetry-instrumentation-express/src/utils.ts index d272434422..f543104e49 100644 --- a/plugins/node/opentelemetry-instrumentation-express/src/utils.ts +++ b/plugins/node/opentelemetry-instrumentation-express/src/utils.ts @@ -132,3 +132,16 @@ export const isLayerIgnored = ( return false; }; + +/** + * Converts a user-provided error value into an error and error message pair + * + * @param error - User-provided error value + * @returns Both an Error or string representation of the value and an error message + */ +export const asErrorAndMessage = ( + error: unknown +): [error: string | Error, message: string] => + error instanceof Error + ? [error, error.message] + : [String(error), String(error)]; diff --git a/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts b/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts index 64d6631409..80c0251815 100644 --- a/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts +++ b/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { context, trace } from '@opentelemetry/api'; +import { SpanStatusCode, context, trace } from '@opentelemetry/api'; import { NodeTracerProvider } from '@opentelemetry/sdk-trace-node'; import { AsyncHooksContextManager } from '@opentelemetry/context-async-hooks'; import { @@ -255,6 +255,94 @@ describe('ExpressInstrumentation', () => { ); }); + it('captures sync middleware errors', async () => { + const rootSpan = tracer.startSpan('rootSpan'); + let finishListenerCount: number | undefined; + const httpServer = await serverWithMiddleware(tracer, rootSpan, app => { + app.use((req, res, next) => { + res.on('finish', () => { + finishListenerCount = res.listenerCount('finish'); + }); + next(); + }); + + const errorMiddleware: express.RequestHandler = (req, res, next) => { + throw new Error('message'); + }; + app.use(errorMiddleware); + }); + server = httpServer.server; + port = httpServer.port; + assert.strictEqual(memoryExporter.getFinishedSpans().length, 0); + + await context.with( + trace.setSpan(context.active(), rootSpan), + async () => { + await httpRequest.get(`http://localhost:${port}/toto/tata`); + rootSpan.end(); + assert.strictEqual(finishListenerCount, 3); + + const errorSpan = memoryExporter + .getFinishedSpans() + .find(span => span.name.includes('errorMiddleware')); + assert.notStrictEqual(errorSpan, undefined); + + assert.deepStrictEqual(errorSpan!.status, { + code: SpanStatusCode.ERROR, + message: 'message', + }); + assert.notStrictEqual( + errorSpan!.events.find(event => event.name === 'exception'), + undefined + ); + } + ); + }); + + it('captures async middleware errors', async () => { + const rootSpan = tracer.startSpan('rootSpan'); + let finishListenerCount: number | undefined; + const httpServer = await serverWithMiddleware(tracer, rootSpan, app => { + app.use((req, res, next) => { + res.on('finish', () => { + finishListenerCount = res.listenerCount('finish'); + }); + next(); + }); + + const errorMiddleware: express.RequestHandler = (req, res, next) => { + setTimeout(() => next(new Error('message')), 10); + }; + app.use(errorMiddleware); + }); + server = httpServer.server; + port = httpServer.port; + assert.strictEqual(memoryExporter.getFinishedSpans().length, 0); + + await context.with( + trace.setSpan(context.active(), rootSpan), + async () => { + await httpRequest.get(`http://localhost:${port}/toto/tata`); + rootSpan.end(); + assert.strictEqual(finishListenerCount, 2); + + const errorSpan = memoryExporter + .getFinishedSpans() + .find(span => span.name.includes('errorMiddleware')); + assert.notStrictEqual(errorSpan, undefined); + + assert.deepStrictEqual(errorSpan!.status, { + code: SpanStatusCode.ERROR, + message: 'message', + }); + assert.notStrictEqual( + errorSpan!.events.find(event => event.name === 'exception'), + undefined + ); + } + ); + }); + it('should not create span because there are no parent', async () => { const app = express(); app.use(express.json()); diff --git a/plugins/node/opentelemetry-instrumentation-express/test/utils.test.ts b/plugins/node/opentelemetry-instrumentation-express/test/utils.test.ts index e85a677af1..fc927a9b7f 100644 --- a/plugins/node/opentelemetry-instrumentation-express/test/utils.test.ts +++ b/plugins/node/opentelemetry-instrumentation-express/test/utils.test.ts @@ -142,4 +142,27 @@ describe('Utils', () => { ); }); }); + + describe('asErrorAndMessage', () => { + it('should special case Error instances', () => { + const input = new Error('message'); + const [error, message] = utils.asErrorAndMessage(input); + assert.strictEqual(error, input); + assert.strictEqual(message, 'message'); + }); + + it('should pass strings as-is', () => { + const input = 'error'; + const [error, message] = utils.asErrorAndMessage(input); + assert.strictEqual(error, input); + assert.strictEqual(message, input); + }); + + it('should stringify other types', () => { + const input = 2; + const [error, message] = utils.asErrorAndMessage(input); + assert.strictEqual(error, '2'); + assert.strictEqual(message, '2'); + }); + }); }); From 1c2e8b20ff981873838c1543324e700e2e466dba Mon Sep 17 00:00:00 2001 From: Jaryk Date: Fri, 1 Dec 2023 20:57:56 +0100 Subject: [PATCH 2/3] feat(cucumber): support @cucumber/cucumber@10 (#1830) Co-authored-by: Daniel Dyla --- plugins/node/instrumentation-cucumber/.tav.yml | 8 ++++++-- .../node/instrumentation-cucumber/src/instrumentation.ts | 4 ++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/plugins/node/instrumentation-cucumber/.tav.yml b/plugins/node/instrumentation-cucumber/.tav.yml index c1930c8cb2..a1771a240c 100644 --- a/plugins/node/instrumentation-cucumber/.tav.yml +++ b/plugins/node/instrumentation-cucumber/.tav.yml @@ -1,3 +1,7 @@ '@cucumber/cucumber': - versions: '^8.0.0 || ^9.0.0' - commands: npm test + - versions: '^8.0.0 || ^9.0.0' + node: '>=14' + commands: npm test + - versions: '^10.0.0' + node: '>=18' + commands: npm test diff --git a/plugins/node/instrumentation-cucumber/src/instrumentation.ts b/plugins/node/instrumentation-cucumber/src/instrumentation.ts index cf759ddaae..5ea3305db2 100644 --- a/plugins/node/instrumentation-cucumber/src/instrumentation.ts +++ b/plugins/node/instrumentation-cucumber/src/instrumentation.ts @@ -52,7 +52,7 @@ export class CucumberInstrumentation extends InstrumentationBase { return [ new InstrumentationNodeModuleDefinition( '@cucumber/cucumber', - ['^8.0.0', '^9.0.0'], + ['^8.0.0', '^9.0.0', '^10.0.0'], (moduleExports, moduleVersion) => { this._diag.debug( `Applying patch for @cucumber/cucumber@${moduleVersion}` @@ -86,7 +86,7 @@ export class CucumberInstrumentation extends InstrumentationBase { default: { new (): TestCaseRunner; prototype: TestCaseRunner }; }>( '@cucumber/cucumber/lib/runtime/test_case_runner.js', - ['^8.0.0', '^9.0.0'], + ['^8.0.0', '^9.0.0', '^10.0.0'], (moduleExports, moduleVersion) => { this._diag.debug( `Applying patch for @cucumber/cucumber/lib/runtime/test_case_runner.js@${moduleVersion}` From fb807835e9317891e6f18715e708e9993b8797d8 Mon Sep 17 00:00:00 2001 From: Ben Eggers <64657842+beggers@users.noreply.github.com> Date: Fri, 1 Dec 2023 12:20:23 -0800 Subject: [PATCH 3/3] fix(instrumentation-lambda): soften "unable to init" message and demote to diag.debug (#1836) Co-authored-by: Trent Mick --- .../src/instrumentation.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-aws-lambda/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-aws-lambda/src/instrumentation.ts index ca3d173d82..0b85377d1d 100644 --- a/plugins/node/opentelemetry-instrumentation-aws-lambda/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-aws-lambda/src/instrumentation.ts @@ -101,8 +101,8 @@ export class AwsLambdaInstrumentation extends InstrumentationBase { // _HANDLER and LAMBDA_TASK_ROOT are always defined in Lambda but guard bail out if in the future this changes. if (!taskRoot || !handlerDef) { - diag.error( - 'Unable to initialize instrumentation for lambda. Cannot identify lambda handler or task root.', + this._diag.debug( + 'Skipping lambda instrumentation: no _HANDLER/lambdaHandler or LAMBDA_TASK_ROOT.', { taskRoot, handlerDef } ); return [];