feat(core): Deprecate trace in favor of startSpan (#10012)

Also add a `handleCallbackErrors` utility to replace this. We've been using this in a few places, and since it has a bit of a different usage than `startSpan` I had to add a new utility to properly make this work. The upside is that we now can ensure to use the same implementation for this "maybe promise" handling everywhere! --------- Co-authored-by: Luca Forstner <luca.forstner@sentry.io>
getsentry · Jan 4, 2024 · a696aef · a696aef
1 parent 3acb7e4
commit a696aef
Show file tree

Hide file tree

Showing 29 changed files with 418 additions and 298 deletions.
diff --git a/packages/astro/src/index.server.ts b/packages/astro/src/index.server.ts
@@ -41,6 +41,7 @@ export {
   setTags,
   setUser,
   spanStatusfromHttpCode,
+  // eslint-disable-next-line deprecation/deprecation
   trace,
   withScope,
   autoDiscoverNodePerformanceMonitoringIntegrations,

diff --git a/packages/browser/src/index.ts b/packages/browser/src/index.ts
@@ -48,6 +48,7 @@ export {
   extractTraceparentData,
   getActiveTransaction,
   spanStatusfromHttpCode,
+  // eslint-disable-next-line deprecation/deprecation
   trace,
   makeMultiplexedTransport,
   ModuleMetadata,

diff --git a/packages/bun/src/index.ts b/packages/bun/src/index.ts
@@ -60,6 +60,7 @@ export {
   setTags,
   setUser,
   spanStatusfromHttpCode,
+  // eslint-disable-next-line deprecation/deprecation
   trace,
   withScope,
   captureCheckIn,

diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts
@@ -69,6 +69,7 @@ export { prepareEvent } from './utils/prepareEvent';
 export { createCheckInEnvelope } from './checkin';
 export { hasTracingEnabled } from './utils/hasTracingEnabled';
 export { isSentryRequestUrl } from './utils/isSentryRequestUrl';
+export { handleCallbackErrors } from './utils/handleCallbackErrors';
 export { spanToTraceHeader } from './utils/spanUtils';
 export { DEFAULT_ENVIRONMENT } from './constants';
 export { ModuleMetadata } from './integrations/metadata';

diff --git a/packages/core/src/tracing/index.ts b/packages/core/src/tracing/index.ts
@@ -9,6 +9,7 @@ export { extractTraceparentData, getActiveTransaction } from './utils';
 export { SpanStatus } from './spanstatus';
 export type { SpanStatusType } from './span';
 export {
+  // eslint-disable-next-line deprecation/deprecation
   trace,
   getActiveSpan,
   startSpan,

diff --git a/packages/core/src/tracing/trace.ts b/packages/core/src/tracing/trace.ts
@@ -1,10 +1,11 @@
 import type { Span, TransactionContext } from '@sentry/types';
-import { dropUndefinedKeys, isThenable, logger, tracingContextFromHeaders } from '@sentry/utils';
+import { dropUndefinedKeys, logger, tracingContextFromHeaders } from '@sentry/utils';
 
 import { DEBUG_BUILD } from '../debug-build';
 import { getCurrentScope, withScope } from '../exports';
 import type { Hub } from '../hub';
 import { getCurrentHub } from '../hub';
+import { handleCallbackErrors } from '../utils/handleCallbackErrors';
 import { hasTracingEnabled } from '../utils/hasTracingEnabled';
 
 /**
@@ -18,6 +19,8 @@ import { hasTracingEnabled } from '../utils/hasTracingEnabled';
  *
  * @internal
  * @private
+ *
+ * @deprecated Use `startSpan` instead.
  */
 export function trace<T>(
   context: TransactionContext,
@@ -37,43 +40,18 @@ export function trace<T>(
 
   scope.setSpan(activeSpan);
 
-  function finishAndSetSpan(): void {
-    activeSpan && activeSpan.end();
-    scope.setSpan(parentSpan);
-  }
-
-  let maybePromiseResult: T;
-  try {
-    maybePromiseResult = callback(activeSpan);
-  } catch (e) {
-    activeSpan && activeSpan.setStatus('internal_error');
-    onError(e, activeSpan);
-    finishAndSetSpan();
-    afterFinish();
-    throw e;
-  }
-
-  if (isThenable(maybePromiseResult)) {
-    // @ts-expect-error - the isThenable check returns the "wrong" type here
-    return maybePromiseResult.then(
-      res => {
-        finishAndSetSpan();
-        afterFinish();
-        return res;
-      },
-      e => {
-        activeSpan && activeSpan.setStatus('internal_error');
-        onError(e, activeSpan);
-        finishAndSetSpan();
-        afterFinish();
-        throw e;
-      },
-    );
-  }
-
-  finishAndSetSpan();
-  afterFinish();
-  return maybePromiseResult;
+  return handleCallbackErrors(
+    () => callback(activeSpan),
+    error => {
+      activeSpan && activeSpan.setStatus('internal_error');
+      onError(error, activeSpan);
+    },
+    () => {
+      activeSpan && activeSpan.end();
+      scope.setSpan(parentSpan);
+      afterFinish();
+    },
+  );
 }
 
 /**
@@ -90,43 +68,23 @@ export function trace<T>(
 export function startSpan<T>(context: TransactionContext, callback: (span: Span | undefined) => T): T {
   const ctx = normalizeContext(context);
 
-  // @ts-expect-error - isThenable returns the wrong type
   return withScope(scope => {
     const hub = getCurrentHub();
     const parentSpan = scope.getSpan();
 
     const activeSpan = createChildSpanOrTransaction(hub, parentSpan, ctx);
     scope.setSpan(activeSpan);
 
-    function finishAndSetSpan(): void {
-      activeSpan && activeSpan.end();
-    }
-
-    let maybePromiseResult: T;
-    try {
-      maybePromiseResult = callback(activeSpan);
-    } catch (e) {
-      activeSpan && activeSpan.setStatus('internal_error');
-      finishAndSetSpan();
-      throw e;
-    }
-
-    if (isThenable(maybePromiseResult)) {
-      return maybePromiseResult.then(
-        res => {
-          finishAndSetSpan();
-          return res;
-        },
-        e => {
-          activeSpan && activeSpan.setStatus('internal_error');
-          finishAndSetSpan();
-          throw e;
-        },
-      );
-    }
-
-    finishAndSetSpan();
-    return maybePromiseResult;
+    return handleCallbackErrors(
+      () => callback(activeSpan),
+      () => {
+        // Only update the span status if it hasn't been changed yet
+        if (activeSpan && (!activeSpan.status || activeSpan.status === 'ok')) {
+          activeSpan.setStatus('internal_error');
+        }
+      },
+      () => activeSpan && activeSpan.end(),
+    );
   });
 }
 
@@ -152,7 +110,6 @@ export function startSpanManual<T>(
 ): T {
   const ctx = normalizeContext(context);
 
-  // @ts-expect-error - isThenable returns the wrong type
   return withScope(scope => {
     const hub = getCurrentHub();
     const parentSpan = scope.getSpan();
@@ -164,25 +121,15 @@ export function startSpanManual<T>(
       activeSpan && activeSpan.end();
     }
 
-    let maybePromiseResult: T;
-    try {
-      maybePromiseResult = callback(activeSpan, finishAndSetSpan);
-    } catch (e) {
-      activeSpan && activeSpan.setStatus('internal_error');
-      throw e;
-    }
-
-    if (isThenable(maybePromiseResult)) {
-      return maybePromiseResult.then(
-        res => res,
-        e => {
-          activeSpan && activeSpan.setStatus('internal_error');
-          throw e;
-        },
-      );
-    }
-
-    return maybePromiseResult;
+    return handleCallbackErrors(
+      () => callback(activeSpan, finishAndSetSpan),
+      () => {
+        // Only update the span status if it hasn't been changed yet, and the span is not yet finished
+        if (activeSpan && !activeSpan.endTimestamp && (!activeSpan.status || activeSpan.status === 'ok')) {
+          activeSpan.setStatus('internal_error');
+        }
+      },
+    );
   });
 }
 

diff --git a/packages/core/src/utils/handleCallbackErrors.ts b/packages/core/src/utils/handleCallbackErrors.ts
@@ -0,0 +1,63 @@
+import { isThenable } from '@sentry/utils';
+
+/**
+ * Wrap a callback function with error handling.
+ * If an error is thrown, it will be passed to the `onError` callback and re-thrown.
+ *
+ * If the return value of the function is a promise, it will be handled with `maybeHandlePromiseRejection`.
+ *
+ * If an `onFinally` callback is provided, this will be called when the callback has finished
+ * - so if it returns a promise, once the promise resolved/rejected,
+ * else once the callback has finished executing.
+ * The `onFinally` callback will _always_ be called, no matter if an error was thrown or not.
+ */
+export function handleCallbackErrors<
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  Fn extends () => any,
+>(
+  fn: Fn,
+  onError: (error: unknown) => void,
+  // eslint-disable-next-line @typescript-eslint/no-empty-function
+  onFinally: () => void = () => {},
+): ReturnType<Fn> {
+  let maybePromiseResult: ReturnType<Fn>;
+  try {
+    maybePromiseResult = fn();
+  } catch (e) {
+    onError(e);
+    onFinally();
+    throw e;
+  }
+
+  return maybeHandlePromiseRejection(maybePromiseResult, onError, onFinally);
+}
+
+/**
+ * Maybe handle a promise rejection.
+ * This expects to be given a value that _may_ be a promise, or any other value.
+ * If it is a promise, and it rejects, it will call the `onError` callback.
+ * Other than this, it will generally return the given value as-is.
+ */
+function maybeHandlePromiseRejection<MaybePromise>(
+  value: MaybePromise,
+  onError: (error: unknown) => void,
+  onFinally: () => void,
+): MaybePromise {
+  if (isThenable(value)) {
+    // @ts-expect-error - the isThenable check returns the "wrong" type here
+    return value.then(
+      res => {
+        onFinally();
+        return res;
+      },
+      e => {
+        onError(e);
+        onFinally();
+        throw e;
+      },
+    );
+  }
+
+  onFinally();
+  return value;
+}