diff --git a/.oxlintrc.base.json b/.oxlintrc.base.json index 4e8d59a39db9..d8262ba8c28a 100644 --- a/.oxlintrc.base.json +++ b/.oxlintrc.base.json @@ -146,7 +146,6 @@ "**/integrations/tracing/knex/vendored/**/*.ts", "**/integrations/tracing/mongo/vendored/**/*.ts", "**/integrations/tracing/koa/vendored/**/*.ts", - "**/integrations/tracing/connect/vendored/**/*.ts", "**/integrations/tracing/mysql2/vendored/**/*.ts", "**/integration/aws/vendored/**/*.ts", "**/integrations/tracing/kafka/vendored/**/*.ts", diff --git a/dev-packages/node-integration-tests/suites/tracing/connect/scenario.mjs b/dev-packages/node-integration-tests/suites/tracing/connect/scenario.mjs index 39054ba009c5..4ecabbebd0b1 100644 --- a/dev-packages/node-integration-tests/suites/tracing/connect/scenario.mjs +++ b/dev-packages/node-integration-tests/suites/tracing/connect/scenario.mjs @@ -8,6 +8,13 @@ const port = 5986; const run = async () => { const app = connect(); + // Path-less middleware produces `middleware`-type spans (named and anonymous). + app.use(function middleware1(req, res, next) { + next(); + }); + + app.use((req, res, next) => next()); + app.use('/', function (req, res, next) { res.end('Hello World'); next(); diff --git a/dev-packages/node-integration-tests/suites/tracing/connect/test.ts b/dev-packages/node-integration-tests/suites/tracing/connect/test.ts index 0c37c58f8a4c..fa50ca028ab8 100644 --- a/dev-packages/node-integration-tests/suites/tracing/connect/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/connect/test.ts @@ -22,6 +22,32 @@ describe('connect auto-instrumentation', () => { op: 'request_handler.connect', status: 'ok', }), + + expect.objectContaining({ + data: expect.objectContaining({ + 'connect.name': 'middleware1', + 'connect.type': 'middleware', + 'sentry.origin': 'auto.http.otel.connect', + 'sentry.op': 'middleware.connect', + }), + description: 'middleware1', + origin: 'auto.http.otel.connect', + op: 'middleware.connect', + status: 'ok', + }), + + expect.objectContaining({ + data: expect.objectContaining({ + 'connect.name': 'anonymous', + 'connect.type': 'middleware', + 'sentry.origin': 'auto.http.otel.connect', + 'sentry.op': 'middleware.connect', + }), + description: 'anonymous', + origin: 'auto.http.otel.connect', + op: 'middleware.connect', + status: 'ok', + }), ]), }; @@ -56,7 +82,30 @@ describe('connect auto-instrumentation', () => { test('should report errored transactions.', async () => { const runner = createTestRunner() .ignore('event') - .expect({ transaction: { transaction: 'GET /error' } }) + .expect({ + transaction: { + transaction: 'GET /error', + spans: expect.arrayContaining([ + expect.objectContaining({ + data: expect.objectContaining({ + 'connect.name': 'connectErrorMiddleware', + 'connect.type': 'middleware', + 'sentry.origin': 'auto.http.otel.connect', + 'sentry.op': 'middleware.connect', + }), + description: 'connectErrorMiddleware', + origin: 'auto.http.otel.connect', + op: 'middleware.connect', + }), + + expect.objectContaining({ + description: '/error', + op: 'request_handler.connect', + status: 'internal_error', + }), + ]), + }, + }) .start(); runner.makeRequest('get', '/error'); await runner.completed(); diff --git a/packages/node/src/integrations/tracing/connect/index.ts b/packages/node/src/integrations/tracing/connect/index.ts index b8f5322b087d..0c1ad31bcdfc 100644 --- a/packages/node/src/integrations/tracing/connect/index.ts +++ b/packages/node/src/integrations/tracing/connect/index.ts @@ -1,13 +1,6 @@ import { ConnectInstrumentation } from './vendored/instrumentation'; -import type { IntegrationFn, Span } from '@sentry/core'; -import { - captureException, - defineIntegration, - getClient, - SEMANTIC_ATTRIBUTE_SENTRY_OP, - SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, - spanToJSON, -} from '@sentry/core'; +import type { IntegrationFn } from '@sentry/core'; +import { captureException, defineIntegration } from '@sentry/core'; import { ensureIsWrapped, generateInstrumentOnce } from '@sentry/node-core'; type ConnectApp = { @@ -78,39 +71,5 @@ function connectErrorMiddleware(err: any, req: any, res: any, next: any): void { */ export const setupConnectErrorHandler = (app: ConnectApp): void => { app.use(connectErrorMiddleware); - - // Sadly, ConnectInstrumentation has no requestHook, so we need to add the attributes here - // We register this hook in this method, because if we register it in the integration `setup`, - // it would always run even for users that are not even using connect - const client = getClient(); - if (client) { - client.on('spanStart', span => { - addConnectSpanAttributes(span); - }); - } - ensureIsWrapped(app.use, 'connect'); }; - -function addConnectSpanAttributes(span: Span): void { - const attributes = spanToJSON(span).data; - - // this is one of: middleware, request_handler - const type = attributes['connect.type']; - - // If this is already set, or we have no connect span, no need to process again... - if (attributes[SEMANTIC_ATTRIBUTE_SENTRY_OP] || !type) { - return; - } - - span.setAttributes({ - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.otel.connect', - [SEMANTIC_ATTRIBUTE_SENTRY_OP]: `${type}.connect`, - }); - - // Also update the name, we don't need the "middleware - " prefix - const name = attributes['connect.name']; - if (typeof name === 'string') { - span.updateName(name); - } -} diff --git a/packages/node/src/integrations/tracing/connect/vendored/enums/AttributeNames.ts b/packages/node/src/integrations/tracing/connect/vendored/enums/AttributeNames.ts index 49a7d014d714..81aebb26d227 100644 --- a/packages/node/src/integrations/tracing/connect/vendored/enums/AttributeNames.ts +++ b/packages/node/src/integrations/tracing/connect/vendored/enums/AttributeNames.ts @@ -17,7 +17,6 @@ * - Vendored from: https://github.com/open-telemetry/opentelemetry-js-contrib/tree/15ef7506553f631ea4181391e0c5725a56f0d082/packages/instrumentation-connect * - Upstream version: @opentelemetry/instrumentation-connect@0.61.0 */ -/* eslint-disable */ export enum AttributeNames { CONNECT_TYPE = 'connect.type', @@ -28,8 +27,3 @@ export enum ConnectTypes { MIDDLEWARE = 'middleware', REQUEST_HANDLER = 'request_handler', } - -export enum ConnectNames { - MIDDLEWARE = 'middleware', - REQUEST_HANDLER = 'request handler', -} diff --git a/packages/node/src/integrations/tracing/connect/vendored/instrumentation.ts b/packages/node/src/integrations/tracing/connect/vendored/instrumentation.ts index 911b6753c92e..63dfb41e3d9d 100644 --- a/packages/node/src/integrations/tracing/connect/vendored/instrumentation.ts +++ b/packages/node/src/integrations/tracing/connect/vendored/instrumentation.ts @@ -18,26 +18,27 @@ * - Upstream version: @opentelemetry/instrumentation-connect@0.61.0 * - Minor TypeScript strictness adjustments for this repository's compiler settings */ -/* eslint-disable */ -import { context, Span, SpanOptions } from '@opentelemetry/api'; import type { ServerResponse } from 'http'; -import { AttributeNames, ConnectNames, ConnectTypes } from './enums/AttributeNames'; -import { HandleFunction, NextFunction, Server, PatchedRequest, Use, UseArgs, UseArgs2 } from './internal-types'; -import { SDK_VERSION } from '@sentry/core'; -import { setHttpServerSpanRouteAttribute } from '../../../../utils/setHttpServerSpanRouteAttribute'; +import { AttributeNames, ConnectTypes } from './enums/AttributeNames'; +import type { HandleFunction, NextFunction, PatchedRequest, Server, Use, UseArgs, UseArgs2 } from './internal-types'; +import type { Span } from '@sentry/core'; import { - InstrumentationBase, - InstrumentationConfig, - InstrumentationNodeModuleDefinition, - isWrapped, -} from '@opentelemetry/instrumentation'; + isError, + SDK_VERSION, + SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, + SPAN_STATUS_ERROR, + startInactiveSpan, +} from '@sentry/core'; +import { setHttpServerSpanRouteAttribute } from '../../../../utils/setHttpServerSpanRouteAttribute'; +import type { InstrumentationConfig } from '@opentelemetry/instrumentation'; +import { InstrumentationBase, InstrumentationNodeModuleDefinition, isWrapped } from '@opentelemetry/instrumentation'; import { ATTR_HTTP_ROUTE } from '@opentelemetry/semantic-conventions'; import { replaceCurrentStackRoute, addNewStackLayer, generateRoute } from './utils'; const PACKAGE_NAME = '@sentry/instrumentation-connect'; -export const ANONYMOUS_NAME = 'anonymous'; +const ANONYMOUS_NAME = 'anonymous'; /** Connect instrumentation for OpenTelemetry */ export class ConnectInstrumentation extends InstrumentationBase { @@ -54,25 +55,30 @@ export class ConnectInstrumentation extends InstrumentationBase { } private _patchApp(patchedApp: Server) { + // oxlint-disable-next-line typescript/unbound-method if (!isWrapped(patchedApp.use)) { this._wrap(patchedApp, 'use', this._patchUse.bind(this)); } + // oxlint-disable-next-line typescript/unbound-method if (!isWrapped(patchedApp.handle)) { this._wrap(patchedApp, 'handle', this._patchHandle.bind(this)); } } private _patchConstructor(original: () => Server): () => Server { - const instrumentation = this; - return function (this: Server, ...args: any[]) { - const app = original.apply(this, args) as Server; - instrumentation._patchApp(app); + const patchApp = this._patchApp.bind(this); + return function (this: Server, ...args: unknown[]) { + const app = Reflect.apply(original, this, args) as Server; + patchApp(app); return app; }; } - public _patchNext(next: NextFunction, finishSpan: () => void): NextFunction { - return function nextFunction(this: NextFunction, err?: any): void { + public _patchNext(next: NextFunction, span: Span, finishSpan: () => void): NextFunction { + return function nextFunction(this: NextFunction, err?: unknown): void { + if (isError(err)) { + span.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); + } const result = next.apply(this, [err]); finishSpan(); return result; @@ -80,37 +86,29 @@ export class ConnectInstrumentation extends InstrumentationBase { } public _startSpan(routeName: string, middleWare: HandleFunction): Span { - let connectType: ConnectTypes; - let connectName: string; - let connectTypeName: string; - if (routeName) { - connectType = ConnectTypes.REQUEST_HANDLER; - connectTypeName = ConnectNames.REQUEST_HANDLER; - connectName = routeName; - } else { - connectType = ConnectTypes.MIDDLEWARE; - connectTypeName = ConnectNames.MIDDLEWARE; - connectName = middleWare.name || ANONYMOUS_NAME; - } - const spanName = `${connectTypeName} - ${connectName}`; - const options: SpanOptions = { + const connectType = routeName ? ConnectTypes.REQUEST_HANDLER : ConnectTypes.MIDDLEWARE; + const connectName = routeName || middleWare.name || ANONYMOUS_NAME; + return startInactiveSpan({ + name: connectName, + op: `${connectType}.connect`, attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.otel.connect', [ATTR_HTTP_ROUTE]: routeName.length > 0 ? routeName : '/', [AttributeNames.CONNECT_TYPE]: connectType, [AttributeNames.CONNECT_NAME]: connectName, }, - }; - - return this.tracer.startSpan(spanName, options); + }); } public _patchMiddleware(routeName: string, middleWare: HandleFunction): HandleFunction { - const instrumentation = this; + const isEnabled = this.isEnabled.bind(this); + const startSpan: (routeName: string, middleWare: HandleFunction) => Span = this._startSpan.bind(this); + const patchNext = this._patchNext.bind(this); const isErrorMiddleware = middleWare.length === 4; function patchedMiddleware(this: Use): void { - if (!instrumentation.isEnabled()) { - return (middleWare as any).apply(this, arguments); + if (!isEnabled()) { + return Reflect.apply(middleWare, this, arguments); } const [reqArgIdx, resArgIdx, nextArgIdx] = isErrorMiddleware ? [1, 2, 3] : [0, 1, 2]; const req = arguments[reqArgIdx] as PatchedRequest; @@ -123,31 +121,27 @@ export class ConnectInstrumentation extends InstrumentationBase { setHttpServerSpanRouteAttribute(generateRoute(req)); } - let spanName = ''; - if (routeName) { - spanName = `request handler - ${routeName}`; - } else { - spanName = `middleware - ${middleWare.name || ANONYMOUS_NAME}`; - } - const span = instrumentation._startSpan(routeName, middleWare); - instrumentation._diag.debug('start span', spanName); + const span = startSpan(routeName, middleWare); let spanFinished = false; function finishSpan() { if (!spanFinished) { spanFinished = true; - instrumentation._diag.debug(`finishing span ${(span as any).name}`); span.end(); - } else { - instrumentation._diag.debug(`span ${(span as any).name} - already finished`); } res.removeListener('close', finishSpan); } res.addListener('close', finishSpan); - arguments[nextArgIdx] = instrumentation._patchNext(next, finishSpan); - - return (middleWare as any).apply(this, arguments); + arguments[nextArgIdx] = patchNext(next, span, finishSpan); + + try { + return Reflect.apply(middleWare, this, arguments); + } catch (e) { + span.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); + finishSpan(); + throw e; + } } Object.defineProperty(patchedMiddleware, 'length', { @@ -160,19 +154,19 @@ export class ConnectInstrumentation extends InstrumentationBase { } public _patchUse(original: Server['use']): Use { - const instrumentation = this; + const patchMiddleware = this._patchMiddleware.bind(this); return function (this: Server, ...args: UseArgs): Server { const middleWare = args[args.length - 1] as HandleFunction; const routeName = (args[args.length - 2] || '') as string; - args[args.length - 1] = instrumentation._patchMiddleware(routeName, middleWare); + args[args.length - 1] = patchMiddleware(routeName, middleWare); return original.apply(this, args as UseArgs2); }; } public _patchHandle(original: Server['handle']): Server['handle'] { - const instrumentation = this; + const patchOut = this._patchOut.bind(this); return function (this: Server): ReturnType { const [reqIdx, outIdx] = [0, 2]; const req = arguments[reqIdx] as PatchedRequest; @@ -180,15 +174,15 @@ export class ConnectInstrumentation extends InstrumentationBase { const completeStack = addNewStackLayer(req); if (typeof out === 'function') { - arguments[outIdx] = instrumentation._patchOut(out as NextFunction, completeStack); + arguments[outIdx] = patchOut(out as NextFunction, completeStack); } - return (original as any).apply(this, arguments); + return Reflect.apply(original, this, arguments); }; } public _patchOut(out: NextFunction, completeStack: () => void): NextFunction { - return function nextFunction(this: NextFunction, ...args: any[]): void { + return function nextFunction(this: NextFunction, ...args: unknown[]): void { completeStack(); return Reflect.apply(out, this, args); }; diff --git a/packages/node/src/integrations/tracing/connect/vendored/internal-types.ts b/packages/node/src/integrations/tracing/connect/vendored/internal-types.ts index 981e1d9aa0e4..0087074459d9 100644 --- a/packages/node/src/integrations/tracing/connect/vendored/internal-types.ts +++ b/packages/node/src/integrations/tracing/connect/vendored/internal-types.ts @@ -18,7 +18,6 @@ * - Upstream version: @opentelemetry/instrumentation-connect@0.61.0 * - Some types vendored from @types/connect */ -/* eslint-disable */ import type * as http from 'http'; @@ -26,12 +25,12 @@ export type IncomingMessage = http.IncomingMessage & { originalUrl?: http.IncomingMessage['url'] | undefined; }; -export type NextFunction = (err?: any) => void; +export type NextFunction = (err?: unknown) => void; export type SimpleHandleFunction = (req: IncomingMessage, res: http.ServerResponse) => void; export type NextHandleFunction = (req: IncomingMessage, res: http.ServerResponse, next: NextFunction) => void; export type ErrorHandleFunction = ( - err: any, + err: unknown, req: IncomingMessage, res: http.ServerResponse, next: NextFunction, diff --git a/packages/node/src/integrations/tracing/connect/vendored/utils.ts b/packages/node/src/integrations/tracing/connect/vendored/utils.ts index e4c7575178f9..74393ea17275 100644 --- a/packages/node/src/integrations/tracing/connect/vendored/utils.ts +++ b/packages/node/src/integrations/tracing/connect/vendored/utils.ts @@ -17,10 +17,11 @@ * - Vendored from: https://github.com/open-telemetry/opentelemetry-js-contrib/tree/15ef7506553f631ea4181391e0c5725a56f0d082/packages/instrumentation-connect * - Upstream version: @opentelemetry/instrumentation-connect@0.61.0 */ -/* eslint-disable */ -import { diag } from '@opentelemetry/api'; -import { _LAYERS_STORE_PROPERTY, PatchedRequest } from './internal-types'; +import { debug } from '@sentry/core'; +import type { PatchedRequest } from './internal-types'; +import { _LAYERS_STORE_PROPERTY } from './internal-types'; +import { DEBUG_BUILD } from '../../../../debug-build'; export const addNewStackLayer = (request: PatchedRequest) => { if (Array.isArray(request[_LAYERS_STORE_PROPERTY]) === false) { @@ -37,7 +38,7 @@ export const addNewStackLayer = (request: PatchedRequest) => { if (stackLength === request[_LAYERS_STORE_PROPERTY].length) { request[_LAYERS_STORE_PROPERTY].pop(); } else { - diag.warn('Connect: Trying to pop the stack multiple time'); + DEBUG_BUILD && debug.warn('Connect: Trying to pop the stack multiple time'); } }; }; diff --git a/packages/node/test/integrations/tracing/connect.test.ts b/packages/node/test/integrations/tracing/connect.test.ts new file mode 100644 index 000000000000..2f718e77a4b5 --- /dev/null +++ b/packages/node/test/integrations/tracing/connect.test.ts @@ -0,0 +1,87 @@ +/* + * Tests ported from @opentelemetry/instrumentation-connect@0.61.0 + * Original source: https://github.com/open-telemetry/opentelemetry-js-contrib/tree/main/packages/instrumentation-connect + * Licensed under the Apache License, Version 2.0 + */ + +import { describe, expect, it } from 'vitest'; +import type { PatchedRequest } from '../../../src/integrations/tracing/connect/vendored/internal-types'; +import { _LAYERS_STORE_PROPERTY } from '../../../src/integrations/tracing/connect/vendored/internal-types'; +import { + addNewStackLayer, + generateRoute, + replaceCurrentStackRoute, +} from '../../../src/integrations/tracing/connect/vendored/utils'; + +describe('utils', () => { + describe('addNewStackLayer', () => { + it('should inject new array to symbol property if not exist', () => { + const fakeRequest = {} as PatchedRequest; + + addNewStackLayer(fakeRequest); + + expect(fakeRequest[_LAYERS_STORE_PROPERTY].length).toBe(1); + }); + + it('should append new stack item if private symbol already exists', () => { + const stack = ['/first']; + const fakeRequest = { + [_LAYERS_STORE_PROPERTY]: stack, + } as PatchedRequest; + + addNewStackLayer(fakeRequest); + + expect(fakeRequest[_LAYERS_STORE_PROPERTY]).toBe(stack); + expect(fakeRequest[_LAYERS_STORE_PROPERTY].length).toBe(2); + }); + + it('should return pop method to remove newly add stack', () => { + const fakeRequest = {} as PatchedRequest; + + const pop = addNewStackLayer(fakeRequest); + + expect(pop).toBeDefined(); + + pop(); + + expect(fakeRequest[_LAYERS_STORE_PROPERTY].length).toBe(0); + }); + + it('should prevent pop the same stack item multiple time', () => { + const fakeRequest = {} as PatchedRequest; + + addNewStackLayer(fakeRequest); // add first stack item + const pop = addNewStackLayer(fakeRequest); // add second stack item + + pop(); + pop(); + + expect(fakeRequest[_LAYERS_STORE_PROPERTY].length).toBe(1); + }); + }); + + describe('replaceCurrentStackRoute', () => { + it('should replace the last stack item with new value', () => { + const fakeRequest = { + [_LAYERS_STORE_PROPERTY]: ['/first', '/second'], + } as PatchedRequest; + + replaceCurrentStackRoute(fakeRequest, '/new_route'); + + expect(fakeRequest[_LAYERS_STORE_PROPERTY].length).toBe(2); + expect(fakeRequest[_LAYERS_STORE_PROPERTY][1]).toBe('/new_route'); + }); + }); + + describe('generateRoute', () => { + it('should combine the stack and striped any slash between layer', () => { + const fakeRequest = { + [_LAYERS_STORE_PROPERTY]: ['/first/', '/second', '/third/'], + } as PatchedRequest; + + const route = generateRoute(fakeRequest); + + expect(route).toBe('/first/second/third/'); + }); + }); +});