feat(remix): Refactor to use new performance APIs (#10980)

This removes all usage of `startTransaction` and `span.startChild()` in
remix.

Instead, we now use `startSpan`, which works reasonably well as we
always have callbacks anyhow.
I also updated it to use `continueTrace` for trace propagation.

Author: mydea

packages/remix/src/utils/instrumentServer.ts

@@ -1,20 +1,24 @@
 /* eslint-disable max-lines */
 import {
+  SEMANTIC_ATTRIBUTE_SENTRY_OP,
   SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
   SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
+  continueTrace,
   getActiveSpan,
   getActiveTransaction,
   getClient,
-  getCurrentScope,
   getDynamicSamplingContextFromSpan,
+  getRootSpan,
+  handleCallbackErrors,
   hasTracingEnabled,
   setHttpStatus,
   spanToJSON,
   spanToTraceHeader,
+  startSpan,
   withIsolationScope,
 } from '@sentry/core';
-import { captureException, getCurrentHub } from '@sentry/node-experimental';
-import type { Hub, Transaction, TransactionSource, WrappedFunction } from '@sentry/types';
+import { captureException } from '@sentry/node-experimental';
+import type { Span, TransactionSource, WrappedFunction } from '@sentry/types';
 import {
   addExceptionMechanism,
   dynamicSamplingContextToSentryBaggageHeader,
@@ -24,7 +28,6 @@ import {
   loadModule,
   logger,
   objectify,
-  tracingContextFromHeaders,
 } from '@sentry/utils';
 
 import { DEBUG_BUILD } from './debug-build';
@@ -186,46 +189,50 @@ function makeWrappedDocumentRequestFunction(remixVersion?: number) {
       context: EntryContext,
       loadContext?: Record<string, unknown>,
     ): Promise<Response> {
-      let res: Response;
-      // eslint-disable-next-line deprecation/deprecation
-      const activeTransaction = getActiveTransaction();
+      const activeSpan = getActiveSpan();
+      const rootSpan = activeSpan && getRootSpan(activeSpan);
 
-      try {
-        // eslint-disable-next-line deprecation/deprecation
-        const span = activeTransaction?.startChild({
-          op: 'function.remix.document_request',
-          origin: 'auto.function.remix',
-          name: spanToJSON(activeTransaction).description,
+      const name = rootSpan ? spanToJSON(rootSpan).description : undefined;
+
+      return startSpan(
+        {
+          // If we don't have a root span, `onlyIfParent` will lead to the span not being created anyhow
+          // So we don't need to care too much about the fallback name, it's just for typing purposes....
+          name: name || '<unknown>',
+          onlyIfParent: true,
           attributes: {
             method: request.method,
             url: request.url,
+            [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.function.remix',
+            [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'function.remix.document_request',
           },
-        });
-
-        res = await origDocumentRequestFunction.call(
-          this,
-          request,
-          responseStatusCode,
-          responseHeaders,
-          context,
-          loadContext,
-        );
-
-        span?.end();
-      } catch (err) {
-        const isRemixV1 = !FUTURE_FLAGS?.v2_errorBoundary && remixVersion !== 2;
-
-        // This exists to capture the server-side rendering errors on Remix v1
-        // On Remix v2, we capture SSR errors at `handleError`
-        // We also skip primitives here, as we can't dedupe them, and also we don't expect any primitive SSR errors.
-        if (isRemixV1 && !isPrimitive(err)) {
-          await captureRemixServerException(err, 'documentRequest', request);
-        }
-
-        throw err;
-      }
-
-      return res;
+        },
+        () => {
+          return handleCallbackErrors(
+            () => {
+              return origDocumentRequestFunction.call(
+                this,
+                request,
+                responseStatusCode,
+                responseHeaders,
+                context,
+                loadContext,
+              );
+            },
+            err => {
+              const isRemixV1 = !FUTURE_FLAGS?.v2_errorBoundary && remixVersion !== 2;
+
+              // This exists to capture the server-side rendering errors on Remix v1
+              // On Remix v2, we capture SSR errors at `handleError`
+              // We also skip primitives here, as we can't dedupe them, and also we don't expect any primitive SSR errors.
+              if (isRemixV1 && !isPrimitive(err)) {
+                // eslint-disable-next-line @typescript-eslint/no-floating-promises
+                captureRemixServerException(err, 'documentRequest', request);
+              }
+            },
+          );
+        },
+      );
     };
   };
 }
@@ -242,47 +249,32 @@ function makeWrappedDataFunction(
       return origFn.call(this, args);
     }
 
-    let res: Response | AppData;
-    // eslint-disable-next-line deprecation/deprecation
-    const activeTransaction = getActiveTransaction();
-    const currentScope = getCurrentScope();
-
-    try {
-      // eslint-disable-next-line deprecation/deprecation
-      const span = activeTransaction?.startChild({
+    return startSpan(
+      {
         op: `function.remix.${name}`,
         origin: 'auto.ui.remix',
         name: id,
         attributes: {
           name,
         },
-      });
-
-      if (span) {
-        // Assign data function to hub to be able to see `db` transactions (if any) as children.
-        // eslint-disable-next-line deprecation/deprecation
-        currentScope.setSpan(span);
-      }
-
-      res = await origFn.call(this, args);
-
-      // eslint-disable-next-line deprecation/deprecation
-      currentScope.setSpan(activeTransaction);
-      span?.end();
-    } catch (err) {
-      const isRemixV2 = FUTURE_FLAGS?.v2_errorBoundary || remixVersion === 2;
-
-      // On Remix v2, we capture all unexpected errors (except the `Route Error Response`s / Thrown Responses) in `handleError` function.
-      // This is both for consistency and also avoid duplicates such as primitives like `string` or `number` being captured twice.
-      // Remix v1 does not have a `handleError` function, so we capture all errors here.
-      if (isRemixV2 ? isResponse(err) : true) {
-        await captureRemixServerException(err, name, args.request);
-      }
-
-      throw err;
-    }
-
-    return res;
+      },
+      () => {
+        return handleCallbackErrors(
+          () => origFn.call(this, args),
+          err => {
+            const isRemixV2 = FUTURE_FLAGS?.v2_errorBoundary || remixVersion === 2;
+
+            // On Remix v2, we capture all unexpected errors (except the `Route Error Response`s / Thrown Responses) in `handleError` function.
+            // This is both for consistency and also avoid duplicates such as primitives like `string` or `number` being captured twice.
+            // Remix v1 does not have a `handleError` function, so we capture all errors here.
+            if (isRemixV2 ? isResponse(err) : true) {
+              // eslint-disable-next-line @typescript-eslint/no-floating-promises
+              captureRemixServerException(err, name, args.request);
+            }
+          },
+        );
+      },
+    );
   };
 }
 
@@ -382,47 +374,44 @@ export function createRoutes(manifest: ServerRouteManifest, parentId?: string):
 }
 
 /**
- * Starts a new transaction for the given request to be used by different `RequestHandler` wrappers.
+ * Starts a new active span for the given request to be used by different `RequestHandler` wrappers.
  */
-export function startRequestHandlerTransaction(
-  hub: Hub,
-  name: string,
-  source: TransactionSource,
-  request: {
-    headers: {
-      'sentry-trace': string;
-      baggage: string;
-    };
+export function startRequestHandlerSpan<T>(
+  {
+    name,
+    source,
+    sentryTrace,
+    baggage,
+    method,
+  }: {
+    name: string;
+    source: TransactionSource;
+    sentryTrace: string;
+    baggage: string;
     method: string;
   },
-): Transaction {
-  // eslint-disable-next-line deprecation/deprecation
-  const { traceparentData, dynamicSamplingContext, propagationContext } = tracingContextFromHeaders(
-    request.headers['sentry-trace'],
-    request.headers.baggage,
-  );
-  // eslint-disable-next-line deprecation/deprecation
-  hub.getScope().setPropagationContext(propagationContext);
-
-  // TODO: Refactor this to `startSpan()`
-  // eslint-disable-next-line deprecation/deprecation
-  const transaction = hub.startTransaction({
-    name,
-    op: 'http.server',
-    attributes: {
-      [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.remix',
-      [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: source,
-      method: request.method,
+  callback: (span: Span) => T,
+): T {
+  return continueTrace(
+    {
+      sentryTrace,
+      baggage,
     },
-    ...traceparentData,
-    metadata: {
-      dynamicSamplingContext: traceparentData && !dynamicSamplingContext ? {} : dynamicSamplingContext,
+    () => {
+      return startSpan(
+        {
+          name,
+          attributes: {
+            [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.remix',
+            [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: source,
+            [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'http.server',
+            method,
+          },
+        },
+        callback,
+      );
     },
-  });
-
-  // eslint-disable-next-line deprecation/deprecation
-  hub.getScope().setSpan(transaction);
-  return transaction;
+  );
 }
 
 /**
@@ -445,8 +434,6 @@ function wrapRequestHandler(origRequestHandler: RequestHandler, build: ServerBui
     }
 
     return withIsolationScope(async isolationScope => {
-      // eslint-disable-next-line deprecation/deprecation
-      const hub = getCurrentHub();
       const options = getClient()?.getOptions();
 
       let normalizedRequest: Record<string, unknown> = request;
@@ -473,23 +460,24 @@ function wrapRequestHandler(origRequestHandler: RequestHandler, build: ServerBui
         return origRequestHandler.call(this, request, loadContext);
       }
 
-      const transaction = startRequestHandlerTransaction(hub, name, source, {
-        headers: {
-          'sentry-trace': request.headers.get('sentry-trace') || '',
+      return startRequestHandlerSpan(
+        {
+          name,
+          source,
+          sentryTrace: request.headers.get('sentry-trace') || '',
           baggage: request.headers.get('baggage') || '',
+          method: request.method,
         },
-        method: request.method,
-      });
-
-      const res = (await origRequestHandler.call(this, request, loadContext)) as Response;
+        async span => {
+          const res = (await origRequestHandler.call(this, request, loadContext)) as Response;
 
-      if (isResponse(res)) {
-        setHttpStatus(transaction, res.status);
-      }
-
-      transaction.end();
+          if (isResponse(res)) {
+            setHttpStatus(span, res.status);
+          }
 
-      return res;
+          return res;
+        },
+      );
     });
   };
 }

packages/remix/src/utils/serverAdapters/express.ts

@@ -1,10 +1,10 @@
 import { getClient, getCurrentHub, hasTracingEnabled, setHttpStatus, withIsolationScope } from '@sentry/core';
 import { flush } from '@sentry/node-experimental';
-import type { Hub, Transaction } from '@sentry/types';
+import type { Hub, Span } from '@sentry/types';
 import { extractRequestData, fill, isString, logger } from '@sentry/utils';
 
 import { DEBUG_BUILD } from '../debug-build';
-import { createRoutes, getTransactionName, instrumentBuild, startRequestHandlerTransaction } from '../instrumentServer';
+import { createRoutes, getTransactionName, instrumentBuild, startRequestHandlerSpan } from '../instrumentServer';
 import type {
   AppLoadContext,
   ExpressCreateRequestHandler,
@@ -89,19 +89,24 @@ function startRequestHandlerTransactionWithRoutes(
   next: ExpressNextFunction,
   hub: Hub,
   url: URL,
-): Transaction | undefined {
+): unknown {
   const [name, source] = getTransactionName(routes, url);
-  const transaction = startRequestHandlerTransaction(hub, name, source, {
-    headers: {
-      'sentry-trace': (req.headers && isString(req.headers['sentry-trace']) && req.headers['sentry-trace']) || '',
+
+  return startRequestHandlerSpan(
+    {
+      name,
+      source,
+      sentryTrace: (req.headers && isString(req.headers['sentry-trace']) && req.headers['sentry-trace']) || '',
       baggage: (req.headers && isString(req.headers.baggage) && req.headers.baggage) || '',
+      method: req.method,
+    },
+    span => {
+      // save a link to the transaction on the response, so that even if there's an error (landing us outside of
+      // the domain), we can still finish it (albeit possibly missing some scope data)
+      (res as AugmentedExpressResponse).__sentrySpan = span;
+      return origRequestHandler.call(this, req, res, next);
     },
-    method: req.method,
-  });
-  // save a link to the transaction on the response, so that even if there's an error (landing us outside of
-  // the domain), we can still finish it (albeit possibly missing some scope data)
-  (res as AugmentedExpressResponse).__sentryTransaction = transaction;
-  return origRequestHandler.call(this, req, res, next);
+  );
 }
 
 function wrapGetLoadContext(origGetLoadContext: () => AppLoadContext): GetLoadContextFunction {
@@ -168,7 +173,7 @@ export function wrapExpressCreateRequestHandler(
 }
 
 export type AugmentedExpressResponse = ExpressResponse & {
-  __sentryTransaction?: Transaction;
+  __sentrySpan?: Span;
 };
 
 type ResponseEndMethod = AugmentedExpressResponse['end'];
@@ -201,16 +206,16 @@ function wrapEndMethod(origEnd: ResponseEndMethod): WrappedResponseEndMethod {
  * @param res The outgoing response for this request, on which the transaction is stored
  */
 async function finishSentryProcessing(res: AugmentedExpressResponse): Promise<void> {
-  const { __sentryTransaction: transaction } = res;
+  const { __sentrySpan: span } = res;
 
-  if (transaction) {
-    setHttpStatus(transaction, res.statusCode);
+  if (span) {
+    setHttpStatus(span, res.statusCode);
 
     // Push `transaction.finish` to the next event loop so open spans have a better chance of finishing before the
     // transaction closes, and make sure to wait until that's done before flushing events
     await new Promise<void>(resolve => {
       setImmediate(() => {
-        transaction.end();
+        span.end();
         resolve();
       });
     });