ref(node): Stop using registerSpanErrorInstrumentation() on server (#21169)

I think this does nothing on node, because the implementation of this
calls this under the hood:

```js
addGlobalErrorInstrumentationHandler(errorCallback);
addGlobalUnhandledRejectionInstrumentationHandler(errorCallback);
```

basically browser-only, using `window.onerror`. I believe this is
handled specifically in node anyhow so should not be used/needed and
allows us to streamline this a bit and potentially move this to a
browser-only place eventually.

This also means we can remove the tag we put here, because this is
actually not needed.

I adjusted a node-integration-test to verify this works there, both with
using our own startSpan implementations (express uses this). With
otel-native spans, it does not work, but it did not work before either -
IMHO this is fine for the time being.

Author: mydea

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({

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,
+});

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();

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();
+    });
+  });
+});

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);

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);

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;