diff --git a/dev-packages/node-integration-tests/suites/express/handle-error/test.ts b/dev-packages/node-integration-tests/suites/express/handle-error/test.ts index 4e702360242f..e66850393c78 100644 --- a/dev-packages/node-integration-tests/suites/express/handle-error/test.ts +++ b/dev-packages/node-integration-tests/suites/express/handle-error/test.ts @@ -10,6 +10,15 @@ test('should capture and send Express controller error with txn name if tracesSa .expect({ transaction: { transaction: 'GET /test/express/:id', + contexts: { + trace: { + op: 'http.server', + status: 'internal_error', + data: expect.objectContaining({ + 'http.response.status_code': 500, + }), + }, + }, }, }) .expect({ diff --git a/dev-packages/node-integration-tests/suites/tracing/tracer-start-active-span-error/instrument.mjs b/dev-packages/node-integration-tests/suites/tracing/tracer-start-active-span-error/instrument.mjs new file mode 100644 index 000000000000..62e64bf8dc10 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/tracer-start-active-span-error/instrument.mjs @@ -0,0 +1,8 @@ +import * as Sentry from '@sentry/node'; +import { loggingTransport } from '@sentry-internal/node-integration-tests'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + tracesSampleRate: 1.0, + transport: loggingTransport, +}); diff --git a/dev-packages/node-integration-tests/suites/tracing/tracer-start-active-span-error/scenario.mjs b/dev-packages/node-integration-tests/suites/tracing/tracer-start-active-span-error/scenario.mjs new file mode 100644 index 000000000000..3e1e31f2871e --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/tracer-start-active-span-error/scenario.mjs @@ -0,0 +1,15 @@ +import * as Sentry from '@sentry/node'; + +const tracer = Sentry.getClient().tracer; + +async function run() { + await tracer.startActiveSpan('test span name', async span => { + try { + throw new Error('Test error from tracer.startActiveSpan'); + } finally { + span.end(); + } + }); +} + +run(); diff --git a/dev-packages/node-integration-tests/suites/tracing/tracer-start-active-span-error/test.ts b/dev-packages/node-integration-tests/suites/tracing/tracer-start-active-span-error/test.ts new file mode 100644 index 000000000000..8d79aa952252 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/tracer-start-active-span-error/test.ts @@ -0,0 +1,33 @@ +import { afterAll, describe, expect } from 'vitest'; +import { cleanupChildProcesses, createEsmAndCjsTests } from '../../../utils/runner'; + +describe('tracer.startActiveSpan errors', () => { + afterAll(() => { + cleanupChildProcesses(); + }); + + createEsmAndCjsTests(__dirname, 'scenario.mjs', 'instrument.mjs', (createRunner, test) => { + // When a callback to raw OTel `tracer.startActiveSpan` throws and the error propagates + // (uncaught), the span status is NOT automatically marked as errored. The user's `finally` + // calls `span.end()` before the error propagates, and an OTel span becomes immutable on end. + // + // Users who want auto-status-on-error should use `Sentry.startSpan` instead, or follow the + // OTel-idiomatic pattern: `span.recordException(err); span.setStatus({ code: ERROR })` in a + // `catch` inside the callback. + test('does NOT mark span errored when uncaught error escapes raw tracer.startActiveSpan callback', async () => { + await createRunner() + .expect({ + transaction: { + transaction: 'test span name', + contexts: { + trace: { + status: 'ok', + }, + }, + }, + }) + .start() + .completed(); + }); + }); +}); diff --git a/packages/core/src/server-runtime-client.ts b/packages/core/src/server-runtime-client.ts index d969f3580114..26ae3e80f3b1 100644 --- a/packages/core/src/server-runtime-client.ts +++ b/packages/core/src/server-runtime-client.ts @@ -3,7 +3,6 @@ import { Client } from './client'; import { getIsolationScope } from './currentScopes'; import { DEBUG_BUILD } from './debug-build'; import type { Scope } from './scope'; -import { registerSpanErrorInstrumentation } from './tracing'; import { DEFAULT_TRANSPORT_BUFFER_SIZE } from './transports/base'; import { addUserAgentToTransportHeaders } from './transports/userAgent'; import type { CheckIn, MonitorConfig, SerializedCheckIn } from './types/checkin'; @@ -38,9 +37,6 @@ export class ServerRuntimeClient< * @param options Configuration options for this SDK. */ public constructor(options: O) { - // Server clients always support tracing - registerSpanErrorInstrumentation(); - addUserAgentToTransportHeaders(options); super(options); diff --git a/packages/core/src/tracing/errors.ts b/packages/core/src/tracing/errors.ts index cf7aafbac8ea..bccc4225f61b 100644 --- a/packages/core/src/tracing/errors.ts +++ b/packages/core/src/tracing/errors.ts @@ -33,10 +33,6 @@ export function registerSpanErrorInstrumentation(): void { } } - // The function name will be lost when bundling but we need to be able to identify this listener later to maintain the - // node.js default exit behaviour - errorCallback.tag = 'sentry_tracingErrorCallback'; - errorsInstrumented = true; addGlobalErrorInstrumentationHandler(errorCallback); addGlobalUnhandledRejectionInstrumentationHandler(errorCallback); diff --git a/packages/node-core/src/integrations/onuncaughtexception.ts b/packages/node-core/src/integrations/onuncaughtexception.ts index 8afa70787a5c..ea0cd877e8be 100644 --- a/packages/node-core/src/integrations/onuncaughtexception.ts +++ b/packages/node-core/src/integrations/onuncaughtexception.ts @@ -6,10 +6,6 @@ import { logAndExitProcess } from '../utils/errorhandling'; type OnFatalErrorHandler = (firstError: Error, secondError?: Error) => void; -type TaggedListener = NodeJS.UncaughtExceptionListener & { - tag?: string; -}; - interface OnUncaughtExceptionOptions { /** * Controls if the SDK should register a handler to exit the process on uncaught errors: @@ -83,19 +79,15 @@ export function makeErrorHandler(client: NodeClient, options: OnUncaughtExceptio // exit behaviour of the SDK accordingly: // - If other listeners are attached, do not exit. // - If the only listener attached is ours, exit. - const userProvidedListenersCount = (global.process.listeners('uncaughtException') as TaggedListener[]).filter( - listener => { - // There are 3 listeners we ignore: - return ( - // as soon as we're using domains this listener is attached by node itself - listener.name !== 'domainUncaughtExceptionClear' && - // the handler we register for tracing - listener.tag !== 'sentry_tracingErrorCallback' && - // the handler we register in this integration - (listener as ErrorHandler)._errorHandler !== true - ); - }, - ).length; + const userProvidedListenersCount = global.process.listeners('uncaughtException').filter(listener => { + // There are 3 listeners we ignore: + return ( + // as soon as we're using domains this listener is attached by node itself + listener.name !== 'domainUncaughtExceptionClear' && + // the handler we register in this integration + (listener as ErrorHandler)._errorHandler !== true + ); + }).length; const processWouldExit = userProvidedListenersCount === 0; const shouldApplyFatalHandlingLogic = options.exitEvenIfOtherHandlersAreRegistered || processWouldExit;