review suggestions, comments, revert kitSpansIntegrations optimization, change root span enhancement time

Author: Lms24

packages/sveltekit/src/server-common/handle.ts

@@ -155,18 +155,9 @@ async function instrumentHandle(
         // https://github.com/getsentry/sentry-javascript/issues/14583
         normalizedRequest: winterCGRequestToRequestData(event.request),
       });
-
-      const res = await resolve(event, {
-        transformPageChunk: addSentryCodeToPage({
-          injectFetchProxyScript: options.injectFetchProxyScript ?? true,
-        }),
-      });
-
       const kitRootSpan = event.tracing?.enabled ? event.tracing?.root : undefined;
 
-      if (sentrySpan) {
-        setHttpStatus(sentrySpan, res.status);
-      } else if (kitRootSpan) {
+      if (kitRootSpan) {
         // Update the root span emitted from SvelteKit to resemble a `http.server` span
         // We're doing this here instead of an event processor to ensure we update the
         // span name as early as possible (for dynamic sampling, et al.)
@@ -188,6 +179,16 @@ async function instrumentHandle(
         });
       }
 
+      const res = await resolve(event, {
+        transformPageChunk: addSentryCodeToPage({
+          injectFetchProxyScript: options.injectFetchProxyScript ?? true,
+        }),
+      });
+
+      if (sentrySpan) {
+        setHttpStatus(sentrySpan, res.status);
+      }
+
       return res;
     };
 

packages/sveltekit/src/server-common/integrations/svelteKitSpans.ts

@@ -8,12 +8,15 @@ import { type SpanJSON, SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_
  */
 export function svelteKitSpansIntegration(): Integration {
   return {
-    name: 'SvelteKitSpansEnhancment',
+    name: 'SvelteKitSpansEnhancement',
     // Using preprocessEvent to ensure the processing happens before user-configured
     // event processors are executed
     preprocessEvent(event) {
       // only iterate over the spans if the root span was emitted by SvelteKit
-      if (event.type === 'transaction' && event.contexts?.trace?.data?.['sveltekit.tracing.original_name']) {
+      // TODO: Right now, we can't optimize this to only check traces with a kit-emitted root span
+      // this is because in Cloudflare, the kit-emitted root span is missing but our cloudflare
+      // SDK emits the http.server span.
+      if (event.type === 'transaction') {
         event.spans?.forEach(_enhanceKitSpan);
       }
     },

packages/sveltekit/src/vite/sentryVitePlugins.ts

@@ -33,7 +33,6 @@ export async function sentrySvelteKit(options: SentrySvelteKitPluginOptions = {}
 
   if (mergedOptions.autoInstrument) {
     // TODO: Once tracing is promoted stable, we need to adjust this check!
-    // We probably need to make a instrumentation.server.ts|js file lookup instead.
     const kitTracingEnabled = !!svelteConfig.kit?.experimental?.tracing?.server;
 
     const pluginOptions: AutoInstrumentSelection = {
@@ -55,6 +54,12 @@ export async function sentrySvelteKit(options: SentrySvelteKitPluginOptions = {}
   const sentryVitePluginsOptions = generateVitePluginOptions(mergedOptions);
 
   if (mergedOptions.autoUploadSourceMaps) {
+    // When source maps are enabled, we need to inject the output directory to get a correct
+    // stack trace, by using this SDK's `rewriteFrames` integration.
+    // This integration picks up the value.
+    // TODO: I don't think this is technically correct. Either we always or never inject the output directory.
+    // Stack traces shouldn't be different, depending on source maps config. With debugIds, we might not even
+    // need to rewrite frames anymore.
     sentryPlugins.push(await makeGlobalValuesInjectionPlugin(svelteConfig, mergedOptions));
   }
 

packages/sveltekit/test/server-common/integrations/svelteKitSpans.test.ts

@@ -7,11 +7,11 @@ describe('svelteKitSpansIntegration', () => {
   it('has a name and a preprocessEventHook', () => {
     const integration = svelteKitSpansIntegration();
 
-    expect(integration.name).toBe('SvelteKitSpansEnhancment');
+    expect(integration.name).toBe('SvelteKitSpansEnhancement');
     expect(typeof integration.preprocessEvent).toBe('function');
   });
 
-  it('enhances spans from SvelteKit, if root span was emitted by SvelteKit', () => {
+  it('enhances spans from SvelteKit', () => {
     const event: TransactionEvent = {
       type: 'transaction',
       contexts: {
@@ -46,39 +46,6 @@ describe('svelteKitSpansIntegration', () => {
     expect(event.spans?.[0]?.data[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]).toBe('auto.http.sveltekit');
   });
 
-  it('does not enhance spans from SvelteKit, if root span was not emitted by SvelteKit', () => {
-    const event: TransactionEvent = {
-      type: 'transaction',
-      contexts: {
-        trace: {
-          span_id: '123',
-          trace_id: 'abc',
-          data: {},
-        },
-      },
-      spans: [
-        {
-          description: 'sveltekit.resolve',
-          data: {
-            someAttribute: 'someValue',
-          },
-          span_id: '123',
-          trace_id: 'abc',
-          start_timestamp: 0,
-        },
-      ],
-    };
-
-    // @ts-expect-error -- passing in an empty option for client but it is unused in the integration
-    svelteKitSpansIntegration().preprocessEvent?.(event, {}, {});
-
-    expect(event.spans).toHaveLength(1);
-    expect(event.spans?.[0]?.op).toBeUndefined();
-    expect(event.spans?.[0]?.origin).toBeUndefined();
-    expect(event.spans?.[0]?.data[SEMANTIC_ATTRIBUTE_SENTRY_OP]).toBeUndefined();
-    expect(event.spans?.[0]?.data[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]).toBeUndefined();
-  });
-
   describe('_enhanceKitSpan', () => {
     it.each([
       ['sveltekit.resolve', 'function.sveltekit.resolve', 'auto.http.sveltekit'],