diff --git a/.agents/skills/convert-otel-integration/EXAMPLES.md b/.agents/skills/convert-otel-integration/EXAMPLES.md new file mode 100644 index 000000000000..2fadbaa7b17f --- /dev/null +++ b/.agents/skills/convert-otel-integration/EXAMPLES.md @@ -0,0 +1,13 @@ +# express + +- OTEL Instrumentation Package: @opentelemetry/instrumentation-express +- Core integration: packages/core/src/integrations/express/ +- Unit tests: packages/core/test/lib/integrations/express/ +- Node integration: packages/node/src/integrations/tracing/express.ts + +# connect + +- OTEL Instrumentation Package: @opentelemetry/instrumentation-connect +- Core integration: packages/core/src/integrations/connect/ +- Unit tests: packages/core/test/lib/integrations/connect/ +- Node integration: packages/node/src/integrations/tracing/connect.ts diff --git a/.agents/skills/convert-otel-integration/SKILL.md b/.agents/skills/convert-otel-integration/SKILL.md new file mode 100644 index 000000000000..7ddb931d8f6b --- /dev/null +++ b/.agents/skills/convert-otel-integration/SKILL.md @@ -0,0 +1,58 @@ +--- +name: convert-otel-integration +description: Add a new portable integration which can be used by all JavaScript SDKs, by converting an `@opentelemetry/instrumentation-*` implementation that was previously limited to only the Node SDK. +argument-hint: +--- + +# Converting an OpenTelemetry Instrumentation to Portable Sentry SDK Integration + +This skill converts the node-specific instrumentation found in an `@opentelemetry/instrumentation-` package, and creates a standalone integration that either leverages existing TracingChannel support, or takes the module exports as an option argument for patching, instead of relying on intercepting the import. + +## Instructions + +### Step 1: Analysis + +- Read the code in `@opentelemetry/instrumentation-` dependency, to see what is being patched. +- Read the code that uses it in `packages/node/src/integrations`. Especially, take note of what functionality is injected in callbacks or options, which provide Sentry-specific behaviors. + +### Step 2: Standalone Implementation + +- Use the integrations listed in `.agents/skills/convert-otel-integration/EXAMPLES.md` as examples for guidance. +- **Create New Integration** + - Create the new portable integration in `packages/core/src/integrations/`. + - This code must encapsulate the relevant patching, but importantly, _not_ any module-loading interception. + - Expect to receive the module's exported object as an argument. + - Capture data using Sentry-specific methods and functions rather than OpenTelemetry APIs. + - Export the new standalone integration methods from the `@sentry/core` package for use in other SDKs. + - **DO NOT** port any "unpatch" or "uninstrument" methods. Once applied, the Sentry patch is permanent, so we can save on bundle size. + - Make sure to include a header on each code file, attributing the source appropriately in accordance with its license, and declaring the new implemetation as a derivative work. +- **Unit Tests** + - Create unit tests at `packages/core/test/lib/integrations/`, with a corresponding `.test.ts` file for each `.ts` file created in `packages/core/src/integrations/`. + - Each created test file should cover its corresponding source file to 100% of statements, lines, and branches, with a minimum of tests. + - Mock internal and external interfaces to target all parts of the new code. +- **Check** + - Ensure that all unit tests pass: `cd packages/core && vitest run test/lib/integrations/` + - Ensure that the build suceeds: `yarn build` + +### Step 3: Node Integration + +- Use the integrations listed in `.agents/skills/convert-otel-integration/EXAMPLES.md` as examples for guidance. +- Locate the file in `packages/node` that uses the `@opentelemetry/instrumentation-` dependency. +- Replace this integration code with a new integration. The new code should create a class that extends `InstrumentationBase<{CONFIG_TYPE}>`, and call the patching method created in the previous step. +- Remove the dependency on `@opentelemetry/instrumentation-` in `packages/node/package.json`. +- Verify that all unit tests in `packages/node` still pass: `cd packages/node ; yarn test`. +- Add integration points if needed to maintain compatibility with existing OpenTelemetry features of the node SDK, but keep any divergence to a minimum. + - If the added functionality for OTEL compatibility is more than 10 lines of custom code, pause and ask for human guidance. + +### Step 4: Integration Tests + +- Run `yarn fix` to detect and address any lint issues. +- Run `yarn build` to build all the code. +- Run `yarn install` to update the lockfile, removing the now-unnecessary otel instrumentation dependency. +- Run `cd dev-packages/node-integration-tests; yarn test` to run node integration tests. +- Debug and fix any issues that this process uncovers. + +### Step 5: Summarize and Record + +- Write a summary of what was done to `.agents/skills/convert-otel-integration/logs/.md`. +- Write a new entry to `.agents/skills/convert-otel-integration/EXAMPLES.md` for the new integration's replaced OTEL intestrumentation package, core integration location, unit test location, and node integration location. diff --git a/.agents/skills/convert-otel-integration/logs/connect.md b/.agents/skills/convert-otel-integration/logs/connect.md new file mode 100644 index 000000000000..1c180110a22e --- /dev/null +++ b/.agents/skills/convert-otel-integration/logs/connect.md @@ -0,0 +1,81 @@ +# Convert OTel Integration: connect + +## Overview + +Converted `@opentelemetry/instrumentation-connect` from a Node-only OTel-based integration to a portable, OTel-free implementation that works in any JavaScript environment. + +## Changes + +### New: `packages/core/src/integrations/connect/index.ts` + +Portable integration derived from the OTel connect instrumentation. Key exports: + +- **`patchConnectModule(connect, options?)`** — wraps the connect factory so every app it creates is automatically patched. Returns the wrapped factory. +- **`patchConnectApp(app, options?)`** — patches an already-created connect app instance's `use` and `handle` methods directly. +- **`setupConnectErrorHandler(app)`** — adds a 4-argument error middleware that captures exceptions via `captureException`. +- **`ConnectIntegrationOptions`** — interface with optional `onRouteResolved` callback, used by platform integrations (e.g. Node.js) to propagate the resolved route to OTel RPC metadata. + +Implementation notes: + +- Route stack tracking is ported from OTel's `utils.js` — a per-request symbol property holds a stack of route path segments that is combined into a full HTTP route string via `generateRoute`. +- Spans are created with `startSpanManual` and Sentry attributes are set directly (`sentry.op`, `sentry.origin`) — no `spanStart` hook needed. +- Origin changed from `'auto.http.otel.connect'` → `'auto.http.connect'` (no longer OTel-based). +- `withActiveSpan(parentSpan, ...)` is called when invoking `next()` so subsequent middleware spans are siblings of the parent rather than children of the current span. +- Middleware arity (`length`) is preserved on the patched function so connect can distinguish error middlewares (4 args) from regular ones (3 args). + +### New: `packages/core/test/lib/integrations/connect/index.test.ts` + +20 unit tests covering: + +- `patchConnectModule` factory wrapping +- Anonymous and named middleware span creation +- `onRouteResolved` callback behavior +- Span lifecycle (ends on `next()` or response `close` event, not both) +- No span when no active parent span +- Error middleware argument positions +- Handle route stack tracking +- Double-patch debug error logging +- `setupConnectErrorHandler` error capture + +### Modified: `packages/core/src/index.ts` + +Added exports: + +```typescript +export { patchConnectModule, setupConnectErrorHandler } from './integrations/connect/index'; +export type { ConnectIntegrationOptions, ConnectModule } from './integrations/connect/index'; +``` + +### Rewritten: `packages/node/src/integrations/tracing/connect.ts` + +Replaced the OTel-instrumentation-based implementation with one that: + +- Defines `ConnectInstrumentation extends InstrumentationBase` whose `init()` method calls `patchConnectModule` via `InstrumentationNodeModuleDefinition`. +- Sets OTel RPC metadata route via the `onRouteResolved` callback (preserving existing OTel route-tracking behavior for Node users). +- `setupConnectErrorHandler` delegates to `coreSetupConnectErrorHandler` and calls `ensureIsWrapped` to verify the instrumentation is active. +- Removed the `spanStart` hook (`addConnectSpanAttributes`) — no longer needed since attributes are set directly when spans are created. + +### Modified: `packages/node/package.json` + +Removed dependency: + +``` +"@opentelemetry/instrumentation-connect": "0.56.0" +``` + +### Modified: `dev-packages/node-integration-tests/suites/tracing/connect/test.ts` + +Updated expected `sentry.origin` value: + +- Before: `'auto.http.otel.connect'` +- After: `'auto.http.connect'` + +## Verification + +- `yarn fix` — no lint or format issues +- `yarn build` — full production build succeeds +- `yarn install` — lockfile updated, removing the OTel connect package +- `packages/core` connect unit tests: **20/20 passed** +- `packages/node` unit tests: **324/325 passed** (1 pre-existing skip) +- `node-integration-tests` connect suite: **6/6 passed** (ESM + CJS) +- Hono test failures confirmed pre-existing on base branch, unrelated to this change 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..99327bcc52c7 100644 --- a/dev-packages/node-integration-tests/suites/tracing/connect/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/connect/test.ts @@ -14,11 +14,11 @@ describe('connect auto-instrumentation', () => { 'connect.name': '/', 'connect.type': 'request_handler', 'http.route': '/', - 'sentry.origin': 'auto.http.otel.connect', + 'sentry.origin': 'auto.http.connect', 'sentry.op': 'request_handler.connect', }), description: '/', - origin: 'auto.http.otel.connect', + origin: 'auto.http.connect', op: 'request_handler.connect', status: 'ok', }), diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 7018ff63fa8a..cf6e48b05966 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -126,6 +126,8 @@ export type { ExpressMiddleware, ExpressErrorMiddleware, } from './integrations/express/types'; +export { patchConnectModule, setupConnectErrorHandler } from './integrations/connect/index'; +export type { ConnectIntegrationOptions, ConnectModule } from './integrations/connect/index'; export { dedupeIntegration } from './integrations/dedupe'; export { extraErrorDataIntegration } from './integrations/extraerrordata'; export { rewriteFramesIntegration } from './integrations/rewriteframes'; diff --git a/packages/core/src/integrations/connect/index.ts b/packages/core/src/integrations/connect/index.ts new file mode 100644 index 000000000000..4f687a563d61 --- /dev/null +++ b/packages/core/src/integrations/connect/index.ts @@ -0,0 +1,304 @@ +/** + * Platform-portable Connect tracing integration. + * + * @module + * + * This Sentry integration is a derivative work based on the OpenTelemetry + * Connect instrumentation. + * + * + * + * Extended under the terms of the Apache 2.0 license linked below: + * + * ---- + * + * Copyright The OpenTelemetry Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { captureException } from '../../exports'; +import { SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN } from '../../semanticAttributes'; +import { startSpanManual, withActiveSpan } from '../../tracing'; +import { getActiveSpan } from '../../utils/spanUtils'; +import { wrapMethod } from '../../utils/object'; +import { DEBUG_BUILD } from '../../debug-build'; +import { debug } from '../../utils/debug-logger'; +import { getDefaultExport } from '../../utils/get-default-export'; + +export const ANONYMOUS_NAME = 'anonymous'; + +// Symbol used to store the route stack on the request object (non-enumerable) +const _LAYERS_STORE_PROPERTY = Symbol('sentry.connect.request-route-stack'); + +// --- Type definitions --- + +export type ConnectRequest = { + method?: string; + url?: string; + [key: symbol]: unknown; +}; + +export type ConnectResponse = { + addListener(event: string, listener: () => void): void; + removeListener(event: string, listener: () => void): void; +}; + +export type ConnectMiddleware = ((...args: unknown[]) => unknown) & { + name?: string; + length: number; +}; + +export type ConnectApp = { + use: (...args: unknown[]) => ConnectApp; + handle: (...args: unknown[]) => unknown; +}; + +// The connect module export is a factory function: connect() returns a ConnectApp +export type ConnectModule = (...args: unknown[]) => ConnectApp; + +export type ConnectModuleExport = ConnectModule | { + default: ConnectModule; +}; + +export interface ConnectIntegrationOptions { + /** + * Optional callback invoked each time a named route handler resolves the + * matched HTTP route. Platform-specific integrations (e.g. Node.js) can use + * this to propagate the resolved route to OTel RPCMetadata. + */ + onRouteResolved?: (route: string) => void; +} + +// --- Internal route stack management --- +// Tracks nested route paths on the request object, mirroring the OTel +// connect instrumentation's approach for building the full HTTP route. + +function addNewStackLayer(req: ConnectRequest): () => void { + let layers = req[_LAYERS_STORE_PROPERTY] as string[] | undefined; + if (!Array.isArray(layers)) { + layers = []; + Object.defineProperty(req, _LAYERS_STORE_PROPERTY, { + enumerable: false, + value: layers, + }); + } + layers.push('/'); + const stackLength = layers.length; + return () => { + if ( + Array.isArray(req[_LAYERS_STORE_PROPERTY]) && + (req[_LAYERS_STORE_PROPERTY] as string[]).length === stackLength + ) { + (req[_LAYERS_STORE_PROPERTY] as string[]).pop(); + } + }; +} + +function replaceCurrentStackRoute(req: ConnectRequest, newRoute: string): void { + if (!newRoute) return; + const layers = req[_LAYERS_STORE_PROPERTY] as string[] | undefined; + if (Array.isArray(layers) && layers.length > 0) { + layers.splice(-1, 1, newRoute); + } +} + +// Combines all stack layers into a single route path, deduplicating slashes: +// ['/api/', '/users', '/:id'] => '/api/users/:id' +function generateRoute(req: ConnectRequest): string { + const layers = req[_LAYERS_STORE_PROPERTY] as string[] | undefined; + /* v8 ignore start */ + if (!Array.isArray(layers) || layers.length === 0) return '/'; + return layers.reduce((acc: string, sub: string) => acc.replace(/\/+$/, '') + sub); +} + +// --- Middleware patching --- + +function patchMiddleware( + routeName: string, + middleware: ConnectMiddleware, + options?: ConnectIntegrationOptions, +): ConnectMiddleware { + // Error middlewares have 4 arguments: (err, req, res, next) + const isErrorMiddleware = middleware.length === 4; + + function patchedMiddleware(this: unknown, ...args: unknown[]): unknown { + const parentSpan = getActiveSpan(); + if (!parentSpan) { + return middleware.apply(this, args); + } + + const [reqArgIdx, resArgIdx, nextArgIdx] = isErrorMiddleware ? [1, 2, 3] : [0, 1, 2]; + const req = args[reqArgIdx] as ConnectRequest; + const res = args[resArgIdx] as ConnectResponse; + const next = args[nextArgIdx] as ((...a: unknown[]) => unknown) | undefined; + + replaceCurrentStackRoute(req, routeName); + + const isRequestHandler = !!routeName; + const connectType = isRequestHandler ? 'request_handler' : 'middleware'; + const connectName = isRequestHandler ? routeName : middleware.name || ANONYMOUS_NAME; + + if (isRequestHandler) { + options?.onRouteResolved?.(generateRoute(req)); + } + + return startSpanManual( + { + name: connectName, + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: `${connectType}.connect`, + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.connect', + 'http.route': routeName.length > 0 ? routeName : '/', + 'connect.type': connectType, + 'connect.name': connectName, + }, + }, + span => { + let spanFinished = false; + + function finishSpan(): void { + if (!spanFinished) { + spanFinished = true; + span.end(); + } + res.removeListener('close', finishSpan); + } + + // End the span when the response closes (handles async middlewares that + // do not call next()) + res.addListener('close', finishSpan); + + if (typeof next === 'function') { + // End this span and restore the parent span context before calling + // next(), so that subsequent middleware spans are siblings (children + // of the parent) rather than nested under this span. + args[nextArgIdx] = function patchedNext(this: unknown, ...nextArgs: unknown[]) { + finishSpan(); + return withActiveSpan(parentSpan, () => next.apply(this, nextArgs)); + }; + } + + return middleware.apply(this, args); + }, + ); + } + + // Preserve the original function's arity so connect can detect error + // middlewares (length === 4) correctly. + Object.defineProperty(patchedMiddleware, 'length', { + value: middleware.length, + writable: false, + configurable: true, + }); + + return patchedMiddleware; +} + +// --- App patching --- + +/** + * Patch an already-created connect app instance. + */ +export function patchConnectApp(app: ConnectApp, options?: ConnectIntegrationOptions): void { + const originalUse = app.use; + try { + wrapMethod(app, 'use', function patchedUse(this: ConnectApp, ...args: unknown[]) { + // connect.use([route,] middleware) — the route is optional + const middleware = args[args.length - 1] as ConnectMiddleware; + /* v8 ignore start */ + const routeName = args.length > 1 ? String(args[args.length - 2] ?? '') : ''; + args[args.length - 1] = patchMiddleware(routeName, middleware, options); + return originalUse.apply(this, args); + }); + } catch (e) { + DEBUG_BUILD && debug.error('Failed to patch connect use method:', e); + } + + const originalHandle = app.handle; + try { + wrapMethod(app, 'handle', function patchedHandle(this: unknown, ...args: unknown[]) { + // handle(req, res[, out]) — 'out' is the fallback called when no + // middleware matches the request (used for nested apps). + const req = args[0] as ConnectRequest; + const out = args[2]; + const completeStack = addNewStackLayer(req); + if (typeof out === 'function') { + args[2] = function patchedOut(this: unknown, ...outArgs: unknown[]) { + completeStack(); + return (out as (...a: unknown[]) => unknown).apply(this, outArgs); + }; + } + return originalHandle.apply(this, args); + }); + } catch (e) { + DEBUG_BUILD && debug.error('Failed to patch connect handle method:', e); + } +} + +/** + * Wrap the connect factory function so that every app created with it is + * automatically patched. + * + * @example + * ```javascript + * import connect from 'connect'; + * import * as Sentry from '@sentry/node'; // or any SDK that extends core + * + * const patchedConnect = Sentry.patchConnectModule(connect); + * const app = patchedConnect(); + * ``` + */ +export function patchConnectModule(connectModule: ConnectModuleExport, options?: ConnectIntegrationOptions): ConnectModule { + const connect = getDefaultExport(connectModule); + return function patchedConnect(this: unknown, ...args: unknown[]) { + const app = connect.apply(this, args) as ConnectApp; + patchConnectApp(app, options); + return app; + } as ConnectModule; +} + +// --- Error handler --- + +function connectErrorMiddleware(err: unknown, _req: unknown, _res: unknown, next: (err: unknown) => void): void { + captureException(err, { + mechanism: { + handled: false, + type: 'auto.middleware.connect', + }, + }); + next(err); +} + +/** + * Add a Connect middleware to capture errors to Sentry. + * + * @param app The Connect app to attach the error handler to + * + * @example + * ```javascript + * const Sentry = require('@sentry/node'); + * const connect = require('connect'); + * + * const app = connect(); + * + * Sentry.setupConnectErrorHandler(app); + * + * // Add your connect routes here + * + * app.listen(3000); + * ``` + */ +export function setupConnectErrorHandler(app: { use: (middleware: unknown) => void }): void { + app.use(connectErrorMiddleware); +} diff --git a/packages/core/src/integrations/express/index.ts b/packages/core/src/integrations/express/index.ts index 4b2d5e2f0677..aed1965b7572 100644 --- a/packages/core/src/integrations/express/index.ts +++ b/packages/core/src/integrations/express/index.ts @@ -33,12 +33,10 @@ import { DEBUG_BUILD } from '../../debug-build'; import type { ExpressApplication, ExpressErrorMiddleware, - ExpressExport, ExpressHandlerOptions, ExpressIntegrationOptions, ExpressLayer, ExpressMiddleware, - ExpressModuleExport, ExpressRequest, ExpressResponse, ExpressRouter, @@ -49,16 +47,13 @@ import type { import { defaultShouldHandleError, getLayerPath, - hasDefaultProp, isExpressWithoutRouterPrototype, isExpressWithRouterPrototype, } from './utils'; import { wrapMethod } from '../../utils/object'; import { patchLayer } from './patch-layer'; import { setSDKProcessingMetadata } from './set-sdk-processing-metadata'; - -const getExpressExport = (express: ExpressModuleExport): ExpressExport => - hasDefaultProp(express) ? express.default : (express as ExpressExport); +import { getDefaultExport } from '../../utils/get-default-export'; /** * This is a portable instrumentatiton function that works in any environment @@ -73,7 +68,7 @@ const getExpressExport = (express: ExpressModuleExport): ExpressExport => */ export const patchExpressModule = (options: ExpressIntegrationOptions) => { // pass in the require() or import() result of express - const express = getExpressExport(options.express); + const express = getDefaultExport(options.express); const routerProto: ExpressRouterv4 | ExpressRouterv5 | undefined = isExpressWithRouterPrototype(express) ? express.Router.prototype // Express v5 : isExpressWithoutRouterPrototype(express) diff --git a/packages/core/src/integrations/express/utils.ts b/packages/core/src/integrations/express/utils.ts index c3473bbab18a..af22a6ea1d97 100644 --- a/packages/core/src/integrations/express/utils.ts +++ b/packages/core/src/integrations/express/utils.ts @@ -30,7 +30,6 @@ import type { SpanAttributes } from '../../types-hoist/span'; import { getStoredLayers } from './request-layer-store'; import type { - ExpressExport, ExpressIntegrationOptions, ExpressLayer, ExpressLayerType, @@ -254,14 +253,6 @@ const isExpressRouterPrototype = (routerProto?: unknown): routerProto is Express export const isExpressWithoutRouterPrototype = (express: unknown): express is ExpressExportv4 => isExpressRouterPrototype((express as ExpressExportv4).Router) && !isExpressWithRouterPrototype(express); -// dynamic puts the default on .default, require or normal import are fine -export const hasDefaultProp = ( - express: unknown, -): express is { - [k: string]: unknown; - default: ExpressExport; -} => !!express && typeof express === 'object' && 'default' in express && typeof express.default === 'function'; - function getStatusCodeFromResponse(error: MiddlewareError): number { const statusCode = error.status || error.statusCode || error.status_code || error.output?.statusCode; return statusCode ? parseInt(statusCode as string, 10) : 500; diff --git a/packages/core/src/utils/get-default-export.ts b/packages/core/src/utils/get-default-export.ts new file mode 100644 index 000000000000..ef37fea574f3 --- /dev/null +++ b/packages/core/src/utils/get-default-export.ts @@ -0,0 +1,24 @@ +/** + * Often we patch a module's default export, but we want to be able to do + * something like this: + * + * ```ts + * patchTheThing(await import('the-thing')); + * ``` + * + * Or like this: + * + * ```ts + * import theThing from 'the-thing'; + * patchTheThing(theThing); + * ``` + */ +export function getDefaultExport(moduleExport: T | { default: T }): T { + return ( + (!!moduleExport && + typeof moduleExport === 'object' && + 'default' in moduleExport && + (moduleExport as { default: T }).default) || + (moduleExport as T) + ); +} diff --git a/packages/core/test/lib/integrations/connect/index.test.ts b/packages/core/test/lib/integrations/connect/index.test.ts new file mode 100644 index 000000000000..a5d5605b0dfa --- /dev/null +++ b/packages/core/test/lib/integrations/connect/index.test.ts @@ -0,0 +1,455 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { + ANONYMOUS_NAME, + patchConnectApp, + patchConnectModule, + setupConnectErrorHandler, + type ConnectApp, + type ConnectModule, + type ConnectRequest, + type ConnectResponse, +} from '../../../../src/integrations/connect/index'; + +// --- Mock Sentry core --- + +const activeSpans: ReturnType[] = []; +let mockParentSpan: ReturnType | null = null; + +function makeMockSpan(name = 'span') { + return { + name, + ended: false, + attributes: {} as Record, + setAttributes(attrs: Record) { + Object.assign(this.attributes, attrs); + }, + end() { + this.ended = true; + }, + }; +} + +vi.mock('../../../../src/utils/spanUtils', () => ({ + getActiveSpan: () => mockParentSpan, +})); + +type StartSpanManualCallback = (span: ReturnType, finish: () => void) => unknown; +const startSpanManualCalls: { options: unknown; cb: StartSpanManualCallback }[] = []; + +vi.mock('../../../../src/tracing', () => ({ + startSpanManual(options: unknown, cb: StartSpanManualCallback) { + const span = makeMockSpan((options as { name: string }).name); + activeSpans.push(span); + startSpanManualCalls.push({ options, cb }); + return cb(span, () => span.end()); + }, + withActiveSpan(_span: unknown, cb: () => unknown) { + return cb(); + }, +})); + +const capturedExceptions: [unknown, unknown][] = []; +vi.mock('../../../../src/exports', () => ({ + captureException(error: unknown, hint: unknown) { + capturedExceptions.push([error, hint]); + return 'eventId'; + }, +})); + +vi.mock('../../../../src/debug-build', () => ({ DEBUG_BUILD: true })); +const debugErrors: [string, unknown][] = []; +vi.mock('../../../../src/utils/debug-logger', () => ({ + debug: { error: (msg: string, e: unknown) => debugErrors.push([msg, e]) }, +})); + +vi.mock('../../../../src/semanticAttributes', () => ({ + SEMANTIC_ATTRIBUTE_SENTRY_OP: 'sentry.op', + SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN: 'sentry.origin', +})); + +// --- Helpers --- + +function makeRequest(): ConnectRequest { + return { method: 'GET', url: '/test' }; +} + +function makeResponse(): ConnectResponse & { listeners: Record void)[]> } { + const listeners: Record void)[]> = {}; + return { + listeners, + addListener(event: string, fn: () => void) { + (listeners[event] ??= []).push(fn); + }, + removeListener(event: string, fn: () => void) { + listeners[event] = (listeners[event] ?? []).filter(l => l !== fn); + }, + }; +} + +function makeApp(): ConnectApp & { stack: Array<(...a: unknown[]) => unknown> } { + const stack: Array<(...a: unknown[]) => unknown> = []; + return { + stack, + use(...args: unknown[]) { + stack.push(args[args.length - 1] as (...a: unknown[]) => unknown); + return this; + }, + handle(req: unknown, res: unknown, out?: unknown) { + for (const fn of stack) { + fn(req, res, out ?? (() => undefined)); + } + return undefined; + }, + } as unknown as ConnectApp & { stack: Array<(...a: unknown[]) => unknown> }; +} + +function makeConnectModule(): ConnectModule { + return function connect() { + return makeApp(); + }; +} + +beforeEach(() => { + activeSpans.length = 0; + startSpanManualCalls.length = 0; + capturedExceptions.length = 0; + debugErrors.length = 0; + mockParentSpan = makeMockSpan('parent'); +}); + +// --- Tests --- + +describe('patchConnectModule', () => { + it('returns a factory that creates patched apps', () => { + const connect = makeConnectModule(); + const patched = patchConnectModule(connect); + const app = patched(); + expect(typeof app.use).toBe('function'); + expect(typeof app.handle).toBe('function'); + }); + + it('patched factory passes args to original factory', () => { + let receivedArgs: unknown[] = []; + const connect = function (...args: unknown[]) { + receivedArgs = args; + return makeApp(); + }; + const patched = patchConnectModule(connect); + patched('arg1', 'arg2'); + expect(receivedArgs).toStrictEqual(['arg1', 'arg2']); + }); + + it('wraps middleware added via use to create spans', () => { + const connect = makeConnectModule(); + const patched = patchConnectModule(connect); + const app = patched(); + + const req = makeRequest(); + const res = makeResponse(); + let middlewareCalled = false; + const middleware = function (_req: unknown, _res: unknown, n: () => void) { + middlewareCalled = true; + n(); + }; + // Clear inferred name so connect treats it as anonymous + Object.defineProperty(middleware, 'name', { value: '', configurable: true }); + + app.use(middleware as unknown as (...args: unknown[]) => ConnectApp); + app.handle(req, res, () => undefined); + + expect(middlewareCalled).toBe(true); + expect(startSpanManualCalls).toHaveLength(1); + expect((startSpanManualCalls[0]!.options as { name: string }).name).toBe(ANONYMOUS_NAME); + }); +}); + +describe('patchConnectApp', () => { + it('wraps anonymous middleware without a route', () => { + const app = makeApp(); + patchConnectApp(app); + + const req = makeRequest(); + const res = makeResponse(); + let middlewareCalled = false; + // Use a function expression so .name === '' (treated as anonymous) + const middleware = function (_req: unknown, _res: unknown, n: () => void) { + middlewareCalled = true; + n(); + }; + + // Clear inferred name so connect treats it as anonymous + Object.defineProperty(middleware, 'name', { value: '', configurable: true }); + + app.use(middleware as unknown as (...args: unknown[]) => ConnectApp); + app.handle(req, res, () => undefined); + expect(middlewareCalled).toBe(true); + + expect(startSpanManualCalls).toHaveLength(1); + const spanOptions = startSpanManualCalls[0]!.options as Record; + expect(spanOptions['name']).toBe(ANONYMOUS_NAME); + expect((spanOptions['attributes'] as Record)['connect.type']).toBe('middleware'); + expect((spanOptions['attributes'] as Record)['connect.name']).toBe(ANONYMOUS_NAME); + expect((spanOptions['attributes'] as Record)['sentry.op']).toBe('middleware.connect'); + expect((spanOptions['attributes'] as Record)['sentry.origin']).toBe('auto.http.connect'); + }); + + it('uses middleware.name when available for anonymous middleware', () => { + const app = makeApp(); + patchConnectApp(app); + + const req = makeRequest(); + const res = makeResponse(); + function myHandler(_req: unknown, _res: unknown, next: () => void) { + next(); + } + + app.use(myHandler as unknown as (...args: unknown[]) => ConnectApp); + app.handle(req, res, () => undefined); + + expect(startSpanManualCalls).toHaveLength(1); + expect((startSpanManualCalls[0]!.options as { name: string }).name).toBe('myHandler'); + }); + + it('wraps named route handler with routeName', () => { + const app = makeApp(); + patchConnectApp(app); + + const req = makeRequest(); + const res = makeResponse(); + const handler = vi.fn((_req: unknown, _res: unknown, n: () => void) => n()); + + app.use('/users', handler as unknown as (...args: unknown[]) => ConnectApp); + app.handle(req, res, () => undefined); + + expect(startSpanManualCalls).toHaveLength(1); + const spanOptions = startSpanManualCalls[0]!.options as Record; + expect(spanOptions['name']).toBe('/users'); + expect((spanOptions['attributes'] as Record)['connect.type']).toBe('request_handler'); + expect((spanOptions['attributes'] as Record)['connect.name']).toBe('/users'); + expect((spanOptions['attributes'] as Record)['sentry.op']).toBe('request_handler.connect'); + expect((spanOptions['attributes'] as Record)['http.route']).toBe('/users'); + }); + + it('calls onRouteResolved when a named route handler is matched', () => { + const app = makeApp(); + const routeResolved: string[] = []; + patchConnectApp(app, { onRouteResolved: r => routeResolved.push(r) }); + + const req = makeRequest(); + const res = makeResponse(); + const handler = vi.fn((_req: unknown, _res: unknown, next: () => void) => next()); + + app.use('/api', handler as unknown as (...args: unknown[]) => ConnectApp); + app.handle(req, res, () => undefined); + + expect(routeResolved).toStrictEqual(['/api']); + }); + + it('does not call onRouteResolved for anonymous middleware', () => { + const app = makeApp(); + const routeResolved: string[] = []; + patchConnectApp(app, { onRouteResolved: r => routeResolved.push(r) }); + + const req = makeRequest(); + const res = makeResponse(); + const middleware = vi.fn((_req: unknown, _res: unknown, next: () => void) => next()); + + app.use(middleware as unknown as (...args: unknown[]) => ConnectApp); + app.handle(req, res, () => undefined); + + expect(routeResolved).toStrictEqual([]); + }); + + it('ends span when next() is called', () => { + const app = makeApp(); + patchConnectApp(app); + + const req = makeRequest(); + const res = makeResponse(); + let capturedNext: (() => void) | undefined; + const middleware = vi.fn((_req: unknown, _res: unknown, next: () => void) => { + capturedNext = next; + }); + + app.use(middleware as unknown as (...args: unknown[]) => ConnectApp); + app.handle(req, res, () => undefined); + + expect(activeSpans[0]!.ended).toBe(false); + capturedNext!(); + expect(activeSpans[0]!.ended).toBe(true); + }); + + it('ends span when response close event fires', () => { + const app = makeApp(); + patchConnectApp(app); + + const req = makeRequest(); + const res = makeResponse(); + const middleware = vi.fn((_req: unknown, _res: unknown, _next: unknown) => { + // intentionally does not call next + }); + + app.use(middleware as unknown as (...args: unknown[]) => ConnectApp); + app.handle(req, res, () => undefined); + + expect(activeSpans[0]!.ended).toBe(false); + // Simulate response close + res.listeners['close']?.forEach(fn => fn()); + expect(activeSpans[0]!.ended).toBe(true); + }); + + it('does not end span twice if both next() and close fire', () => { + const app = makeApp(); + patchConnectApp(app); + + const req = makeRequest(); + const res = makeResponse(); + let capturedNext: (() => void) | undefined; + const middleware = vi.fn((_req: unknown, _res: unknown, next: () => void) => { + capturedNext = next; + }); + + app.use(middleware as unknown as (...args: unknown[]) => ConnectApp); + app.handle(req, res, () => undefined); + + capturedNext!(); + res.listeners['close']?.forEach(fn => fn()); + + // span.end is idempotent in our patchedMiddleware + expect(activeSpans[0]!.ended).toBe(true); + }); + + it('skips span creation when there is no active parent span', () => { + mockParentSpan = null; + const app = makeApp(); + patchConnectApp(app); + + const req = makeRequest(); + const res = makeResponse(); + const middleware = vi.fn((_req: unknown, _res: unknown, next: () => void) => next()); + + app.use(middleware as unknown as (...args: unknown[]) => ConnectApp); + app.handle(req, res, () => undefined); + + expect(startSpanManualCalls).toHaveLength(0); + expect(middleware).toHaveBeenCalledOnce(); + }); + + it('preserves middleware arity (length) so connect can detect error handlers', () => { + const app = makeApp(); + patchConnectApp(app); + + const errorMiddleware = vi.fn(); + Object.defineProperty(errorMiddleware, 'length', { value: 4 }); + + app.use(errorMiddleware as unknown as (...args: unknown[]) => ConnectApp); + + // Verify that the patched middleware in the stack still has length 4 + const { stack } = app as unknown as { stack: Function[] }; + expect(stack[0]!.length).toBe(4); + }); + + it('error middleware uses (err, req, res, next) argument positions', () => { + const app = makeApp(); + patchConnectApp(app); + + const req = makeRequest(); + const res = makeResponse(); + const next = vi.fn(); + const err = new Error('oops'); + let capturedArgs: unknown[] = []; + + // 4-arg error middleware + const errorMiddleware = function (_err: unknown, _req: unknown, _res: unknown, _next: () => void) { + capturedArgs = [...arguments]; + _next(); + }; + + app.use(errorMiddleware as unknown as (...args: unknown[]) => ConnectApp); + + // Simulate connect calling the error middleware + const { stack } = app as unknown as { stack: Function[] }; + (stack[0] as Function)(err, req, res, next); + + expect(capturedArgs[0]).toBe(err); + expect(capturedArgs[1]).toBe(req); + expect(capturedArgs[2]).toBe(res); + expect(typeof capturedArgs[3]).toBe('function'); // patched next + }); + + it('wraps handle to track route stack per request', () => { + const app = makeApp(); + const routeResolved: string[] = []; + patchConnectApp(app, { onRouteResolved: r => routeResolved.push(r) }); + + const req = makeRequest(); + const res = makeResponse(); + + // Simulate nested: handle adds a layer, route handler resolves the route + const handler = vi.fn((_req: unknown, _res: unknown, next: () => void) => next()); + app.use('/api/users', handler as unknown as (...args: unknown[]) => ConnectApp); + app.handle(req, res, () => undefined); + + expect(routeResolved).toStrictEqual(['/api/users']); + }); + + it('emits debug error when patching use fails (already wrapped)', () => { + const app = makeApp(); + patchConnectApp(app); // first patch + patchConnectApp(app); // second patch — should log debug error + expect(debugErrors.some(([msg]) => (msg as string).includes('use'))).toBe(true); + }); + + it('emits debug error when patching handle fails (already wrapped)', () => { + const app = makeApp(); + patchConnectApp(app); // first patch + patchConnectApp(app); // second patch — should log debug error + expect(debugErrors.some(([msg]) => (msg as string).includes('handle'))).toBe(true); + }); + + it('http.route falls back to "/" for middleware without a routeName', () => { + const app = makeApp(); + patchConnectApp(app); + + const req = makeRequest(); + const res = makeResponse(); + const middleware = vi.fn((_req: unknown, _res: unknown, next: () => void) => next()); + + app.use(middleware as unknown as (...args: unknown[]) => ConnectApp); + app.handle(req, res, () => undefined); + + const attrs = (startSpanManualCalls[0]!.options as { attributes: Record }).attributes; + expect(attrs['http.route']).toBe('/'); + }); +}); + +describe('setupConnectErrorHandler', () => { + it('adds a 4-argument error middleware to the app', () => { + const added: unknown[] = []; + const app = { use: (fn: unknown) => added.push(fn) }; + setupConnectErrorHandler(app); + expect(added).toHaveLength(1); + const fn = added[0] as Function; + expect(fn.length).toBe(4); + }); + + it('captures exceptions via captureException', () => { + const added: unknown[] = []; + const app = { use: (fn: unknown) => added.push(fn) }; + setupConnectErrorHandler(app); + + const next = vi.fn(); + const err = new Error('test'); + (added[0] as Function)(err, {}, {}, next); + + expect(capturedExceptions).toStrictEqual([ + [ + err, + { + mechanism: { handled: false, type: 'auto.middleware.connect' }, + }, + ], + ]); + expect(next).toHaveBeenCalledExactlyOnceWith(err); + }); +}); diff --git a/packages/core/test/lib/utils/get-default-export.test.ts b/packages/core/test/lib/utils/get-default-export.test.ts new file mode 100644 index 000000000000..a412677499c7 --- /dev/null +++ b/packages/core/test/lib/utils/get-default-export.test.ts @@ -0,0 +1,27 @@ +import { describe, expect, it } from 'vitest'; +import { getDefaultExport } from '../../../src/utils/get-default-export'; + +describe('getDefaultExport', () => { + it('returns the default export if there is one', () => { + const mod = { + default: () => {} + }; + expect(getDefaultExport(mod)).toBe(mod.default) + }); + it('returns the module export if no default', () => { + const mod = {}; + expect(getDefaultExport(mod)).toBe(mod) + }); + it('returns the module if a function and not plain object', () => { + const mod = Object.assign(function () {}, { + default: () => {} + }); + expect(getDefaultExport(mod)).toBe(mod) + }); + it('returns the module if a default is falsey', () => { + const mod = Object.assign(function () {}, { + default: false, + }); + expect(getDefaultExport(mod)).toBe(mod) + }); +}); diff --git a/packages/node/package.json b/packages/node/package.json index 13230e707f90..d178a96c57bc 100644 --- a/packages/node/package.json +++ b/packages/node/package.json @@ -70,7 +70,6 @@ "@opentelemetry/core": "^2.6.1", "@opentelemetry/instrumentation": "^0.214.0", "@opentelemetry/instrumentation-amqplib": "0.61.0", - "@opentelemetry/instrumentation-connect": "0.57.0", "@opentelemetry/instrumentation-dataloader": "0.31.0", "@opentelemetry/instrumentation-fs": "0.33.0", "@opentelemetry/instrumentation-generic-pool": "0.57.0", diff --git a/packages/node/src/integrations/tracing/connect.ts b/packages/node/src/integrations/tracing/connect.ts index 3fd7d10b28fb..ef23e0af9b61 100644 --- a/packages/node/src/integrations/tracing/connect.ts +++ b/packages/node/src/integrations/tracing/connect.ts @@ -1,29 +1,67 @@ -import { ConnectInstrumentation } from '@opentelemetry/instrumentation-connect'; -import type { IntegrationFn, Span } from '@sentry/core'; +// Automatic instrumentation for Connect using our portable core integration +import type { InstrumentationConfig } from '@opentelemetry/instrumentation'; +import { InstrumentationBase, InstrumentationNodeModuleDefinition } from '@opentelemetry/instrumentation'; +import { context } from '@opentelemetry/api'; +import { getRPCMetadata, RPCType } from '@opentelemetry/core'; + +import type { ConnectIntegrationOptions, ConnectModule, IntegrationFn } from '@sentry/core'; import { - captureException, + patchConnectModule, + setupConnectErrorHandler as coreSetupConnectErrorHandler, + SDK_VERSION, defineIntegration, - getClient, - SEMANTIC_ATTRIBUTE_SENTRY_OP, - SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, - spanToJSON, } from '@sentry/core'; import { ensureIsWrapped, generateInstrumentOnce } from '@sentry/node-core'; type ConnectApp = { - // eslint-disable-next-line @typescript-eslint/no-explicit-any + // oxlint-disable-next-line no-explicit-any use: (middleware: any) => void; }; const INTEGRATION_NAME = 'Connect'; +const SUPPORTED_VERSIONS = ['>=3.0.0 <4']; + +export type ConnectInstrumentationConfig = InstrumentationConfig & Omit; + +export const instrumentConnect = generateInstrumentOnce( + INTEGRATION_NAME, + (options?: ConnectInstrumentationConfig) => new ConnectInstrumentation(options), +); -export const instrumentConnect = generateInstrumentOnce(INTEGRATION_NAME, () => new ConnectInstrumentation()); +export class ConnectInstrumentation extends InstrumentationBase { + public constructor(config: ConnectInstrumentationConfig = {}) { + super('sentry-connect', SDK_VERSION, config); + } + + public init(): InstrumentationNodeModuleDefinition { + let originalConnect: ConnectModule | undefined; -const _connectIntegration = (() => { + return new InstrumentationNodeModuleDefinition( + 'connect', + SUPPORTED_VERSIONS, + connect => { + originalConnect = connect as ConnectModule; + return patchConnectModule(connect as ConnectModule, { + onRouteResolved(route) { + const rpcMetadata = getRPCMetadata(context.active()); + if (route && rpcMetadata?.type === RPCType.HTTP) { + rpcMetadata.route = route; + } + }, + }); + }, + () => { + return originalConnect; + }, + ); + } +} + +const _connectIntegration = ((options?: ConnectInstrumentationConfig) => { return { name: INTEGRATION_NAME, setupOnce() { - instrumentConnect(); + instrumentConnect(options); }, }; }) satisfies IntegrationFn; @@ -46,17 +84,6 @@ const _connectIntegration = (() => { */ export const connectIntegration = defineIntegration(_connectIntegration); -// eslint-disable-next-line @typescript-eslint/no-explicit-any -function connectErrorMiddleware(err: any, req: any, res: any, next: any): void { - captureException(err, { - mechanism: { - handled: false, - type: 'auto.middleware.connect', - }, - }); - next(err); -} - /** * Add a Connect middleware to capture errors to Sentry. * @@ -71,46 +98,12 @@ function connectErrorMiddleware(err: any, req: any, res: any, next: any): void { * * Sentry.setupConnectErrorHandler(app); * - * // Add you connect routes here + * // Add your connect routes here * * app.listen(3000); * ``` */ 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); - }); - } - + coreSetupConnectErrorHandler(app); 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/yarn.lock b/yarn.lock index eb64ead6a9e8..e77001c9049e 100644 --- a/yarn.lock +++ b/yarn.lock @@ -6259,16 +6259,6 @@ "@opentelemetry/instrumentation" "^0.214.0" "@opentelemetry/semantic-conventions" "^1.34.0" -"@opentelemetry/instrumentation-connect@0.57.0": - version "0.57.0" - resolved "https://registry.yarnpkg.com/@opentelemetry/instrumentation-connect/-/instrumentation-connect-0.57.0.tgz#66b58af135ef6d52ad546cb440b808a149118296" - integrity sha512-FMEBChnI4FLN5TE9DHwfH7QpNir1JzXno1uz/TAucVdLCyrG0jTrKIcNHt/i30A0M2AunNBCkcd8Ei26dIPKdg== - dependencies: - "@opentelemetry/core" "^2.0.0" - "@opentelemetry/instrumentation" "^0.214.0" - "@opentelemetry/semantic-conventions" "^1.27.0" - "@types/connect" "3.4.38" - "@opentelemetry/instrumentation-dataloader@0.31.0": version "0.31.0" resolved "https://registry.yarnpkg.com/@opentelemetry/instrumentation-dataloader/-/instrumentation-dataloader-0.31.0.tgz#43bfbe09f99e84eb0d8b6e9f914c2e51a45e6d95" @@ -9286,7 +9276,7 @@ "@types/express-serve-static-core" "*" "@types/node" "*" -"@types/connect@*", "@types/connect@3.4.38": +"@types/connect@*": version "3.4.38" resolved "https://registry.yarnpkg.com/@types/connect/-/connect-3.4.38.tgz#5ba7f3bc4fbbdeaff8dded952e5ff2cc53f8d858" integrity sha512-K6uROf1LD88uDQqJCktA4yzL1YYAK6NgfsI0v/mTgyPKWsX1CnJ0XPSDhViejru1GcRkLWb8RlzFYJRqGUbaug==