avoid nested continueTrace calls

Author: Lms24

dev-packages/e2e-tests/test-applications/sveltekit/src/hooks.server.ts

@@ -1,25 +1,17 @@
 import { env } from '$env/dynamic/private';
 import * as Sentry from '@sentry/sveltekit';
-import { sequence } from '@sveltejs/kit/hooks';
 
 Sentry.init({
   environment: 'qa', // dynamic sampling bias to keep transactions
   dsn: env.E2E_TEST_DSN,
   debug: true,
   tunnel: `http://localhost:3031/`, // proxy server
   tracesSampleRate: 1.0,
-  beforeSendTransaction: txn => {
-    // console.log('beforeSendTransaction', txn);
-    return txn;
-  },
 });
 
 // not logging anything to console to avoid noise in the test output
 const myErrorHandler = ({ error, event }: any) => {};
 
 export const handleError = Sentry.handleErrorWithSentry(myErrorHandler);
 
-export const handle = sequence(async ({ event, resolve }) => {
-  console.log('XX event issub', event.isSubRequest, Sentry.getActiveSpan());
-  return resolve(event);
-}, Sentry.sentryHandle());
+export const handle = Sentry.sentryHandle();

dev-packages/e2e-tests/test-applications/sveltekit/src/routes/server-load-fetch/+page.ts renamed to dev-packages/e2e-tests/test-applications/sveltekit/src/routes/server-load-fetch/+page.server.ts

@@ -12,6 +12,9 @@ test('server pageload request span has nested request span', async ({ page }) =>
   const serverTxnEvent = await serverTxnEventPromise;
   const spans = serverTxnEvent.spans;
 
+  const outerHttpServerSpan = spans?.find(span => span.op === 'http.server' && span.description === 'GET /server-load-fetch');
+  const innerHttpServerSpan = spans?.find(span => span.op === 'http.server' && span.description === 'GET /api/users');
+
   expect(serverTxnEvent).toMatchObject({
     transaction: 'GET /server-load-fetch',
     tags: { runtime: 'node' },
@@ -25,7 +28,7 @@ test('server pageload request span has nested request span', async ({ page }) =>
     },
   });
 
-  console.log(JSON.stringify(spans, null, 2));
-
   expect(spans).toHaveLength(3);
+  expect(outerHttpServerSpan).toBeDefined();
+  expect(innerHttpServerSpan).toBeDefined();
 });

dev-packages/e2e-tests/test-applications/sveltekit/test/performance.server.test.ts

@@ -56,8 +56,6 @@ export function startSpan<T>(context: StartSpanOptions, callback: (span: Span) =
     // eslint-disable-next-line deprecation/deprecation
     const parentSpan = scope.getSpan() as SentrySpan | undefined;
 
-    console.log(`xx startSpan ${context.op} ${context.name} parentSpan`, parentSpan);
-
     const shouldSkipSpan = context.onlyIfParent && !parentSpan;
     const activeSpan = shouldSkipSpan
       ? new SentryNonRecordingSpan()
@@ -253,7 +251,6 @@ function createChildSpanOrTransaction(
     span = parentSpan.startChild(spanContext);
     addChildSpanToSpan(parentSpan, span);
 
-    console.log('starting child span', spanContext.op);
   } else if (parentSpan) {
     // If we forced a transaction but have a parent span, make sure to continue from the parent span, not the scope
     const dsc = getDynamicSamplingContextFromSpan(parentSpan);
@@ -271,7 +268,6 @@ function createChildSpanOrTransaction(
         ...spanContext.metadata,
       },
     });
-    console.log('starting txn (forced)', spanContext.op);
   } else {
     const { traceId, dsc, parentSpanId, sampled } = {
       ...isolationScope.getPropagationContext(),
@@ -289,7 +285,6 @@ function createChildSpanOrTransaction(
         ...spanContext.metadata,
       },
     });
-    console.log('starting txn', spanContext.op);
   }
 
   // TODO v8: Technically `startTransaction` can return undefined, which is not reflected by the types

packages/core/src/tracing/trace.ts

@@ -157,25 +157,18 @@ export function sentryHandle(handlerOptions?: SentryHandleOptions): Handle {
     // We want the `http.server` span of that nested call to be a child span of the
     // currently active span instead of a new root span to correctly reflect this
     // behavior.
-    // Type-casting to boolean | undefined to reflect that kit <1.21.0 doesn't have this property
-    const isSubRequest = input.event.isSubRequest as boolean | undefined;
+    // As a fallback for Kit < 1.21.0, we check if there is an active span only if there's none,
+    // we create a new execution context.
+    const isSubRequest = typeof input.event.isSubRequest === 'boolean' ? input.event.isSubRequest : !!getActiveSpan();
+
     if (isSubRequest) {
       return instrumentHandle(input, options);
     }
-    if (isSubRequest === false) {
-      return withIsolationScope(() => instrumentHandle(input, options));
-    }
 
-    // Fallback for Sveltekit < 1.21.0
-    // if there is an active span, we know that this handle call is nested and hence
-    // we don't create a new execution context for it.
-    // If we created one, nested server calls would create new root span instead
-    // of adding a child span to the currently active span.
-    if (getActiveSpan()) {
-      return instrumentHandle(input, options);
-    }
     return withIsolationScope(() => {
-      return instrumentHandle(input, options);
+      // We only call continueTrace in the initial top level request to avoid
+      // creating a new root span for the sub request.
+      return continueTrace(getTracePropagationData(input.event), () => instrumentHandle(input, options));
     });
   };
 
@@ -190,36 +183,32 @@ async function instrumentHandle(
     return resolve(event);
   }
 
-  const { sentryTrace, baggage } = getTracePropagationData(event);
-
-  return continueTrace({ sentryTrace, baggage }, async () => {
-    try {
-      const resolveResult = await startSpan(
-        {
-          op: 'http.server',
-          attributes: {
-            [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.sveltekit',
-            [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: event.route?.id ? 'route' : 'url',
-            'http.method': event.request.method,
-          },
-          name: `${event.request.method} ${event.route?.id || event.url.pathname}`,
-        },
-        async (span?: Span) => {
-          const res = await resolve(event, {
-            transformPageChunk: addSentryCodeToPage(options),
-          });
-          if (span) {
-            setHttpStatus(span, res.status);
-          }
-          return res;
+  try {
+    const resolveResult = await startSpan(
+      {
+        op: 'http.server',
+        attributes: {
+          [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.sveltekit',
+          [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: event.route?.id ? 'route' : 'url',
+          'http.method': event.request.method,
         },
-      );
-      return resolveResult;
-    } catch (e: unknown) {
-      sendErrorToSentry(e);
-      throw e;
-    } finally {
-      await flushIfServerless();
-    }
-  });
+        name: `${event.request.method} ${event.route?.id || event.url.pathname}`,
+      },
+      async (span?: Span) => {
+        const res = await resolve(event, {
+          transformPageChunk: addSentryCodeToPage(options),
+        });
+        if (span) {
+          setHttpStatus(span, res.status);
+        }
+        return res;
+      },
+    );
+    return resolveResult;
+  } catch (e: unknown) {
+    sendErrorToSentry(e);
+    throw e;
+  } finally {
+    await flushIfServerless();
+  }
 }

packages/sveltekit/src/server/handle.ts

@@ -2,15 +2,14 @@ import {
   SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
   SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
   captureException,
-  continueTrace,
   startSpan,
 } from '@sentry/node';
 import { addNonEnumerableProperty, objectify } from '@sentry/utils';
 import type { LoadEvent, ServerLoadEvent } from '@sveltejs/kit';
 
 import type { SentryWrappedFlag } from '../common/utils';
 import { isHttpError, isRedirect } from '../common/utils';
-import { flushIfServerless, getTracePropagationData } from './utils';
+import { flushIfServerless } from './utils';
 
 type PatchedLoadEvent = LoadEvent & SentryWrappedFlag;
 type PatchedServerLoadEvent = ServerLoadEvent & SentryWrappedFlag;
@@ -132,30 +131,26 @@ export function wrapServerLoadWithSentry<T extends (...args: any) => any>(origSe
       // https://github.com/sveltejs/kit/blob/e133aba479fa9ba0e7f9e71512f5f937f0247e2c/packages/kit/src/runtime/server/page/load_data.js#L111C3-L124
       const routeId = event.route && (Object.getOwnPropertyDescriptor(event.route, 'id')?.value as string | undefined);
 
-      const { sentryTrace, baggage } = getTracePropagationData(event);
-
-      return continueTrace({ sentryTrace, baggage }, async () => {
-        try {
-          // We need to await before returning, otherwise we won't catch any errors thrown by the load function
-          return await startSpan(
-            {
-              op: 'function.sveltekit.server.load',
-              attributes: {
-                [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.function.sveltekit',
-                [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: routeId ? 'route' : 'url',
-                'http.method': event.request.method,
-              },
-              name: routeId ? routeId : event.url.pathname,
+      try {
+        // We need to await before returning, otherwise we won't catch any errors thrown by the load function
+        return await startSpan(
+          {
+            op: 'function.sveltekit.server.load',
+            attributes: {
+              [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.function.sveltekit',
+              [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: routeId ? 'route' : 'url',
+              'http.method': event.request.method,
             },
-            () => wrappingTarget.apply(thisArg, args),
-          );
-        } catch (e: unknown) {
-          sendErrorToSentry(e);
-          throw e;
-        } finally {
-          await flushIfServerless();
-        }
-      });
+            name: routeId ? routeId : event.url.pathname,
+          },
+          () => wrappingTarget.apply(thisArg, args),
+        );
+      } catch (e: unknown) {
+        sendErrorToSentry(e);
+        throw e;
+      } finally {
+        await flushIfServerless();
+      }
     },
   });
 }