feat(core): Update continueTrace to be callback-only (#11044)

mydea · web-flow · commit 2a1a036feae1 · 2024-03-12T12:57:55.000Z
Also update it to only update the propagation context in the callback.
In some places (old node...) we need the legacy behaviour, but for now
we can do that in-place.
diff --git a/packages/bun/src/integrations/bunserver.ts b/packages/bun/src/integrations/bunserver.ts
@@ -86,13 +86,12 @@ function instrumentBunServeOptions(serveOptions: Parameters<typeof Bun.serve>[0]
 
         return continueTrace(
           { sentryTrace: request.headers.get('sentry-trace') || '', baggage: request.headers.get('baggage') },
-          ctx => {
+          () => {
             return startSpan(
               {
                 attributes,
                 op: 'http.server',
                 name: `${request.method} ${parsedUrl.path || '/'}`,
-                ...ctx,
               },
               async span => {
                 try {
diff --git a/packages/core/src/tracing/trace.ts b/packages/core/src/tracing/trace.ts
@@ -8,12 +8,11 @@ import type {
   TransactionContext,
 } from '@sentry/types';
 
-import { dropUndefinedKeys, logger, tracingContextFromHeaders } from '@sentry/utils';
+import { propagationContextFromHeaders } from '@sentry/utils';
 import type { AsyncContextStrategy } from '../asyncContext';
 import { getMainCarrier } from '../asyncContext';
 import { getClient, getCurrentScope, getIsolationScope, withScope } from '../currentScopes';
 
-import { DEBUG_BUILD } from '../debug-build';
 import { getAsyncContextStrategy, getCurrentHub } from '../hub';
 import { handleCallbackErrors } from '../utils/handleCallbackErrors';
 import { hasTracingEnabled } from '../utils/hasTracingEnabled';
@@ -179,105 +178,28 @@ export function startInactiveSpan(context: StartSpanOptions): Span {
   });
 }
 
-interface ContinueTrace {
-  /**
-   * Continue a trace from `sentry-trace` and `baggage` values.
-   * These values can be obtained from incoming request headers,
-   * or in the browser from `<meta name="sentry-trace">` and `<meta name="baggage">` HTML tags.
-   *
-   * @deprecated Use the version of this function taking a callback as second parameter instead:
-   *
-   * ```
-   * Sentry.continueTrace(sentryTrace: '...', baggage: '...' }, () => {
-   *    // ...
-   * })
-   * ```
-   *
-   */
-  ({
-    sentryTrace,
-    baggage,
-  }: {
-    // eslint-disable-next-line deprecation/deprecation
-    sentryTrace: Parameters<typeof tracingContextFromHeaders>[0];
-    // eslint-disable-next-line deprecation/deprecation
-    baggage: Parameters<typeof tracingContextFromHeaders>[1];
-  }): Partial<TransactionContext>;
-
-  /**
-   * Continue a trace from `sentry-trace` and `baggage` values.
-   * These values can be obtained from incoming request headers, or in the browser from `<meta name="sentry-trace">`
-   * and `<meta name="baggage">` HTML tags.
-   *
-   * Spans started with `startSpan`, `startSpanManual` and `startInactiveSpan`, within the callback will automatically
-   * be attached to the incoming trace.
-   *
-   * Deprecation notice: In the next major version of the SDK the provided callback will not receive a transaction
-   * context argument.
-   */
-  <V>(
-    {
-      sentryTrace,
-      baggage,
-    }: {
-      // eslint-disable-next-line deprecation/deprecation
-      sentryTrace: Parameters<typeof tracingContextFromHeaders>[0];
-      // eslint-disable-next-line deprecation/deprecation
-      baggage: Parameters<typeof tracingContextFromHeaders>[1];
-    },
-    // TODO(v8): Remove parameter from this callback.
-    callback: (transactionContext: Partial<TransactionContext>) => V,
-  ): V;
-}
-
-export const continueTrace: ContinueTrace = <V>(
+/**
+ * Continue a trace from `sentry-trace` and `baggage` values.
+ * These values can be obtained from incoming request headers, or in the browser from `<meta name="sentry-trace">`
+ * and `<meta name="baggage">` HTML tags.
+ *
+ * Spans started with `startSpan`, `startSpanManual` and `startInactiveSpan`, within the callback will automatically
+ * be attached to the incoming trace.
+ */
+export const continueTrace = <V>(
   {
     sentryTrace,
     baggage,
   }: {
-    // eslint-disable-next-line deprecation/deprecation
-    sentryTrace: Parameters<typeof tracingContextFromHeaders>[0];
-    // eslint-disable-next-line deprecation/deprecation
-    baggage: Parameters<typeof tracingContextFromHeaders>[1];
+    sentryTrace: Parameters<typeof propagationContextFromHeaders>[0];
+    baggage: Parameters<typeof propagationContextFromHeaders>[1];
   },
-  callback?: (transactionContext: Partial<TransactionContext>) => V,
-): V | Partial<TransactionContext> => {
-  // TODO(v8): Change this function so it doesn't do anything besides setting the propagation context on the current scope:
-  /*
-    return withScope((scope) => {
-      const propagationContext = propagationContextFromHeaders(sentryTrace, baggage);
-      scope.setPropagationContext(propagationContext);
-      return callback();
-    })
-  */
-
-  const currentScope = getCurrentScope();
-
-  // eslint-disable-next-line deprecation/deprecation
-  const { traceparentData, dynamicSamplingContext, propagationContext } = tracingContextFromHeaders(
-    sentryTrace,
-    baggage,
-  );
-
-  currentScope.setPropagationContext(propagationContext);
-
-  if (DEBUG_BUILD && traceparentData) {
-    logger.log(`[Tracing] Continuing trace ${traceparentData.traceId}.`);
-  }
-
-  const transactionContext: Partial<TransactionContext> = {
-    ...traceparentData,
-    metadata: dropUndefinedKeys({
-      dynamicSamplingContext,
-    }),
-  };
-
-  if (!callback) {
-    return transactionContext;
-  }
-
-  return withScope(() => {
-    return callback(transactionContext);
+  callback: () => V,
+): V => {
+  return withScope(scope => {
+    const propagationContext = propagationContextFromHeaders(sentryTrace, baggage);
+    scope.setPropagationContext(propagationContext);
+    return callback();
   });
 };
 
diff --git a/packages/core/test/lib/tracing/trace.test.ts b/packages/core/test/lib/tracing/trace.test.ts
@@ -1112,19 +1112,10 @@ describe('continueTrace', () => {
   });
 
   it('works without trace & baggage data', () => {
-    const expectedContext = {
-      metadata: {},
-    };
-
-    const result = continueTrace({ sentryTrace: undefined, baggage: undefined }, ctx => {
-      expect(ctx).toEqual(expectedContext);
-      return ctx;
+    const scope = continueTrace({ sentryTrace: undefined, baggage: undefined }, () => {
+      return getCurrentScope();
     });
 
-    expect(result).toEqual(expectedContext);
-
-    const scope = getCurrentScope();
-
     expect(scope.getPropagationContext()).toEqual({
       sampled: undefined,
       spanId: expect.any(String),
@@ -1135,30 +1126,16 @@ describe('continueTrace', () => {
   });
 
   it('works with trace data', () => {
-    const expectedContext = {
-      metadata: {
-        dynamicSamplingContext: {},
-      },
-      parentSampled: false,
-      parentSpanId: '1121201211212012',
-      traceId: '12312012123120121231201212312012',
-    };
-
-    const result = continueTrace(
+    const scope = continueTrace(
       {
         sentryTrace: '12312012123120121231201212312012-1121201211212012-0',
         baggage: undefined,
       },
-      ctx => {
-        expect(ctx).toEqual(expectedContext);
-        return ctx;
+      () => {
+        return getCurrentScope();
       },
     );
 
-    expect(result).toEqual(expectedContext);
-
-    const scope = getCurrentScope();
-
     expect(scope.getPropagationContext()).toEqual({
       dsc: {}, // DSC should be an empty object (frozen), because there was an incoming trace
       sampled: false,
@@ -1171,33 +1148,16 @@ describe('continueTrace', () => {
   });
 
   it('works with trace & baggage data', () => {
-    const expectedContext = {
-      metadata: {
-        dynamicSamplingContext: {
-          environment: 'production',
-          version: '1.0',
-        },
-      },
-      parentSampled: true,
-      parentSpanId: '1121201211212012',
-      traceId: '12312012123120121231201212312012',
-    };
-
-    const result = continueTrace(
+    const scope = continueTrace(
       {
         sentryTrace: '12312012123120121231201212312012-1121201211212012-1',
         baggage: 'sentry-version=1.0,sentry-environment=production',
       },
-      ctx => {
-        expect(ctx).toEqual(expectedContext);
-        return ctx;
+      () => {
+        return getCurrentScope();
       },
     );
 
-    expect(result).toEqual(expectedContext);
-
-    const scope = getCurrentScope();
-
     expect(scope.getPropagationContext()).toEqual({
       dsc: {
         environment: 'production',
@@ -1213,33 +1173,16 @@ describe('continueTrace', () => {
   });
 
   it('works with trace & 3rd party baggage data', () => {
-    const expectedContext = {
-      metadata: {
-        dynamicSamplingContext: {
-          environment: 'production',
-          version: '1.0',
-        },
-      },
-      parentSampled: true,
-      parentSpanId: '1121201211212012',
-      traceId: '12312012123120121231201212312012',
-    };
-
-    const result = continueTrace(
+    const scope = continueTrace(
       {
         sentryTrace: '12312012123120121231201212312012-1121201211212012-1',
         baggage: 'sentry-version=1.0,sentry-environment=production,dogs=great,cats=boring',
       },
-      ctx => {
-        expect(ctx).toEqual(expectedContext);
-        return ctx;
+      () => {
+        return getCurrentScope();
       },
     );
 
-    expect(result).toEqual(expectedContext);
-
-    const scope = getCurrentScope();
-
     expect(scope.getPropagationContext()).toEqual({
       dsc: {
         environment: 'production',
@@ -1255,45 +1198,17 @@ describe('continueTrace', () => {
   });
 
   it('returns response of callback', () => {
-    const expectedContext = {
-      metadata: {
-        dynamicSamplingContext: {},
-      },
-      parentSampled: false,
-      parentSpanId: '1121201211212012',
-      traceId: '12312012123120121231201212312012',
-    };
-
     const result = continueTrace(
       {
         sentryTrace: '12312012123120121231201212312012-1121201211212012-0',
         baggage: undefined,
       },
-      ctx => {
-        return { ctx };
+      () => {
+        return 'aha';
       },
     );
 
-    expect(result).toEqual({ ctx: expectedContext });
-  });
-
-  it('works without a callback', () => {
-    const expectedContext = {
-      metadata: {
-        dynamicSamplingContext: {},
-      },
-      parentSampled: false,
-      parentSpanId: '1121201211212012',
-      traceId: '12312012123120121231201212312012',
-    };
-
-    // eslint-disable-next-line deprecation/deprecation
-    const ctx = continueTrace({
-      sentryTrace: '12312012123120121231201212312012-1121201211212012-0',
-      baggage: undefined,
-    });
-
-    expect(ctx).toEqual(expectedContext);
+    expect(result).toEqual('aha');
   });
 });
 
diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts
@@ -58,34 +58,34 @@ export function tracingHandler(): (
       return next();
     }
 
+    // We depend here on the fact that we update the current scope...
+    // so we keep this legacy behavior here for now
+    const scope = getCurrentScope();
+
     const [name, source] = extractPathForTransaction(req, { path: true, method: true });
-    const transaction = continueTrace(
-      { sentryTrace, baggage },
-      ctx =>
-        startInactiveSpan({
-          name,
-          op: 'http.server',
-          origin: 'auto.http.node.tracingHandler',
-          forceTransaction: true,
-          ...ctx,
-          attributes: {
-            [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: source,
-          },
-          metadata: {
-            // eslint-disable-next-line deprecation/deprecation
-            ...ctx.metadata,
-            // The request should already have been stored in `scope.sdkProcessingMetadata` (which will become
-            // `event.sdkProcessingMetadata` the same way the metadata here will) by `sentryRequestMiddleware`, but on the
-            // off chance someone is using `sentryTracingMiddleware` without `sentryRequestMiddleware`, it doesn't hurt to
-            // be sure
-            request: req,
-          },
-        }) as Transaction,
-    );
+    const transaction = continueTrace({ sentryTrace, baggage }, () => {
+      scope.setPropagationContext(getCurrentScope().getPropagationContext());
+      return startInactiveSpan({
+        name,
+        op: 'http.server',
+        origin: 'auto.http.node.tracingHandler',
+        forceTransaction: true,
+        attributes: {
+          [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: source,
+        },
+        metadata: {
+          // The request should already have been stored in `scope.sdkProcessingMetadata` (which will become
+          // `event.sdkProcessingMetadata` the same way the metadata here will) by `sentryRequestMiddleware`, but on the
+          // off chance someone is using `sentryTracingMiddleware` without `sentryRequestMiddleware`, it doesn't hurt to
+          // be sure
+          request: req,
+        },
+      }) as Transaction;
+    });
 
     // We put the transaction on the scope so users can attach children to it
     // eslint-disable-next-line deprecation/deprecation
-    getCurrentScope().setSpan(transaction);
+    scope.setSpan(transaction);
 
     // We also set __sentry_transaction on the response so people can grab the transaction there to add
     // spans to it later.
diff --git a/packages/node/src/integrations/hapi/index.ts b/packages/node/src/integrations/hapi/index.ts
diff --git a/packages/tracing-internal/src/browser/browserTracingIntegration.ts b/packages/tracing-internal/src/browser/browserTracingIntegration.ts
