Remove unnecessary continueTrace calls

Author: andreiborza

dev-packages/e2e-tests/test-applications/solidstart/src/routes/users/[id].tsx

@@ -14,6 +14,7 @@ export default function User() {
   return (
     <div>
       User ID: {params.id}
+      <br />
       Prefecture: {userData()?.prefecture}
     </div>
   );

dev-packages/e2e-tests/test-applications/solidstart/tests/errors.server.test.ts

@@ -25,7 +25,7 @@ test.describe('server-side errors', () => {
           },
         ],
       },
-      transaction: 'getPrefecture',
+      transaction: 'GET /server-error',
     });
   });
 });

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

@@ -1,25 +1,55 @@
 import { expect, test } from '@playwright/test';
 import { waitForTransaction } from '@sentry-internal/test-utils';
+import {
+  SEMANTIC_ATTRIBUTE_SENTRY_OP,
+  SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
+  SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
+} from '@sentry/core';
 
-test('sends a server action transaction', async ({ page }) => {
+test('sends a server action transaction on pageload', async ({ page }) => {
   const transactionPromise = waitForTransaction('solidstart', transactionEvent => {
-    return transactionEvent?.transaction === 'getPrefecture';
+    return transactionEvent?.transaction === 'GET /users/6';
   });
 
   await page.goto('/users/6');
 
   const transaction = await transactionPromise;
 
-  expect(transaction).toMatchObject({
-    transaction: 'getPrefecture',
-    tags: { runtime: 'node' },
-    transaction_info: { source: 'url' },
-    type: 'transaction',
-    contexts: {
-      trace: {
-        op: 'function.server_action',
-        origin: 'manual',
-      },
-    },
+  expect(transaction.spans).toEqual(
+    expect.arrayContaining([
+      expect.objectContaining({
+        description: 'getPrefecture',
+        data: {
+          [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'function.server_action',
+          [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'manual',
+          [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route',
+        },
+      }),
+    ]),
+  );
+});
+
+test('sends a server action transaction on client navigation', async ({ page }) => {
+  const transactionPromise = waitForTransaction('solidstart', transactionEvent => {
+    return transactionEvent?.transaction === 'POST getPrefecture';
   });
+
+  await page.goto('/');
+  await page.locator('#navLink').click();
+  await page.waitForURL('/users/5');
+
+  const transaction = await transactionPromise;
+
+  expect(transaction.spans).toEqual(
+    expect.arrayContaining([
+      expect.objectContaining({
+        description: 'getPrefecture',
+        data: {
+          [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'function.server_action',
+          [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'manual',
+          [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route',
+        },
+      }),
+    ]),
+  );
 });

packages/solidstart/package.json

@@ -72,7 +72,8 @@
     "@sentry/solid": "8.20.0",
     "@sentry/types": "8.20.0",
     "@sentry/utils": "8.20.0",
-    "@sentry/vite-plugin": "2.19.0"
+    "@sentry/vite-plugin": "2.19.0",
+    "@opentelemetry/instrumentation": "^0.52.1"
   },
   "devDependencies": {
     "@solidjs/router": "^0.13.4",

packages/solidstart/src/server/utils.ts

@@ -3,6 +3,21 @@ import { logger } from '@sentry/utils';
 import type { RequestEvent } from 'solid-js/web';
 import { DEBUG_BUILD } from '../common/debug-build';
 
+/** Flush the event queue to ensure that events get sent to Sentry before the response is finished and the lambda ends */
+export async function flushIfServerless(): Promise<void> {
+  const isServerless = !!process.env.LAMBDA_TASK_ROOT || !!process.env.VERCEL;
+
+  if (isServerless) {
+    try {
+      DEBUG_BUILD && logger.log('Flushing events...');
+      await flush(2000);
+      DEBUG_BUILD && logger.log('Done flushing events');
+    } catch (e) {
+      DEBUG_BUILD && logger.log('Error while flushing events:\n', e);
+    }
+  }
+}
+
 /**
  * Takes a request event and extracts traceparent and DSC data
  * from the `sentry-trace` and `baggage` DSC headers.
@@ -19,21 +34,6 @@ export function getTracePropagationData(event: RequestEvent | undefined): {
   return { sentryTrace, baggage };
 }
 
-/** Flush the event queue to ensure that events get sent to Sentry before the response is finished and the lambda ends */
-export async function flushIfServerless(): Promise<void> {
-  const isServerless = !!process.env.LAMBDA_TASK_ROOT || !!process.env.VERCEL;
-
-  if (isServerless) {
-    try {
-      DEBUG_BUILD && logger.log('Flushing events...');
-      await flush(2000);
-      DEBUG_BUILD && logger.log('Done flushing events');
-    } catch (e) {
-      DEBUG_BUILD && logger.log('Error while flushing events:\n', e);
-    }
-  }
-}
-
 /**
  * Determines if a thrown "error" is a redirect Response which Solid Start users can throw to redirect to another route.
  * see: https://docs.solidjs.com/solid-router/reference/data-apis/response-helpers#redirect

packages/solidstart/src/server/withServerActionInstrumentation.ts

@@ -1,46 +1,38 @@
 import { SPAN_STATUS_ERROR, handleCallbackErrors } from '@sentry/core';
-import {
-  SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
-  captureException,
-  continueTrace,
-  startSpan,
-  withIsolationScope,
-} from '@sentry/node';
-import { winterCGRequestToRequestData } from '@sentry/utils';
-import { getRequestEvent } from 'solid-js/web';
-import { flushIfServerless, getTracePropagationData, isRedirect } from './utils';
+import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, captureException, getActiveSpan, spanToJSON, startSpan } from '@sentry/node';
+import { flushIfServerless, isRedirect } from './utils';
 
 /**
- * Wraps a server action (functions that use the 'use server' directive) function body with Sentry Error and Performance instrumentation.
+ * Wraps a server action (functions that use the 'use server' directive)
+ * function body with Sentry Error and Performance instrumentation.
  */
 export async function withServerActionInstrumentation<A extends (...args: unknown[]) => unknown>(
   serverActionName: string,
   callback: A,
 ): Promise<ReturnType<A>> {
-  return withIsolationScope(isolationScope => {
-    const event = getRequestEvent();
+  const activeSpan = getActiveSpan();
 
-    if (event && event.request) {
-      isolationScope.setSDKProcessingMetadata({ request: winterCGRequestToRequestData(event.request) });
-    }
-    isolationScope.setTransactionName(serverActionName);
+  if (activeSpan) {
+    const spanData = spanToJSON(activeSpan).data;
 
-    return continueTrace(getTracePropagationData(event), () => instrumentServerAction(serverActionName, callback));
-  });
-}
+    // In solid start, server function calls are made to `/_server` which doesn't tell us
+    // a lot. We rewrite the span's route to be that of the sever action name but only
+    // if the target is `/_server`, otherwise we'd overwrite pageloads on routes that use
+    // server actions (which are more meaningful, e.g. a request to `GET /users/5` is more
+    // meaningful than overwriting it with `GET doSomeFunctionCall`).
+    if (spanData && !spanData['http.route'] && spanData['http.target'] === '/_server') {
+      activeSpan.setAttribute('http.route', serverActionName);
+      activeSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route');
+    }
+  }
 
-async function instrumentServerAction<A extends (...args: unknown[]) => unknown>(
-  name: string,
-  callback: A,
-): Promise<ReturnType<A>> {
   try {
     return await startSpan(
       {
         op: 'function.server_action',
-        name,
-        forceTransaction: true,
+        name: serverActionName,
         attributes: {
-          [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url',
+          [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route',
         },
       },
       async span => {

packages/solidstart/test/server/withServerActionInstrumentation.test.ts

@@ -12,12 +12,11 @@ import {
 import { NodeClient } from '@sentry/node';
 import { solidRouterBrowserTracingIntegration } from '@sentry/solidstart/solidrouter';
 import { redirect } from '@solidjs/router';
-import { Headers } from 'node-fetch';
 import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
 
 const mockCaptureException = vi.spyOn(SentryNode, 'captureException').mockImplementation(() => '');
 const mockFlush = vi.spyOn(SentryNode, 'flush').mockImplementation(async () => true);
-const mockContinueTrace = vi.spyOn(SentryNode, 'continueTrace');
+const mockGetActiveSpan = vi.spyOn(SentryNode, 'getActiveSpan');
 
 const mockGetRequestEvent = vi.fn();
 vi.mock('solid-js/web', async () => {
@@ -28,6 +27,7 @@ vi.mock('solid-js/web', async () => {
   };
 });
 
+import { SentrySpan } from '@sentry/core';
 import { withServerActionInstrumentation } from '../../src/server';
 
 describe('withServerActionInstrumentation', () => {
@@ -107,53 +107,13 @@ describe('withServerActionInstrumentation', () => {
         description: 'getPrefecture',
         data: expect.objectContaining({
           [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'function.server_action',
-          [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url',
+          [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route',
           [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'manual',
         }),
       }),
     );
   });
 
-  it('calls `continueTrace` with the right sentry-trace and baggage', async () => {
-    const baggage =
-      'sentry-environment=qa,sentry-public_key=12345678,sentry-trace_id=4c9b164c5b5f4a0c8db3ce490b935ea8,sentry-sample_rate=1,sentry-sampled=true';
-    const sentryTrace = '4c9b164c5b5f4a0c8db3ce490b935ea8';
-    mockGetRequestEvent.mockReturnValue({
-      request: {
-        method: 'GET',
-        url: '/_server',
-        headers: new Headers([
-          ['sentry-trace', sentryTrace],
-          ['baggage', baggage],
-        ]),
-      },
-    });
-
-    await serverActionGetPrefecture();
-
-    expect(mockContinueTrace).to.toHaveBeenCalledWith(
-      expect.objectContaining({
-        sentryTrace,
-        baggage,
-      }),
-      expect.any(Function),
-    );
-  });
-
-  it('calls `continueTrace` with no sentry-trace or baggage', async () => {
-    mockGetRequestEvent.mockReturnValue({ request: {} });
-
-    await serverActionGetPrefecture();
-
-    expect(mockContinueTrace).to.toHaveBeenCalledWith(
-      expect.objectContaining({
-        sentryTrace: undefined,
-        baggage: null,
-      }),
-      expect.any(Function),
-    );
-  });
-
   it('calls `flush` if lambda', async () => {
     vi.stubEnv('LAMBDA_TASK_ROOT', '1');
 
@@ -168,7 +128,12 @@ describe('withServerActionInstrumentation', () => {
     expect(mockFlush).toHaveBeenCalledTimes(1);
   });
 
-  it('sets a server action transaction name', async () => {
+  it('sets a server action name on the active span', async () => {
+    const span = new SentrySpan();
+    span.setAttribute('http.target', '/_server');
+    mockGetActiveSpan.mockReturnValue(span);
+    const mockSpanSetAttribute = vi.spyOn(span, 'setAttribute');
+
     const getPrefecture = async function load() {
       return withServerActionInstrumentation('getPrefecture', () => {
         return { prefecture: 'Kagoshima' };
@@ -177,39 +142,26 @@ describe('withServerActionInstrumentation', () => {
 
     await getPrefecture();
 
-    expect(getIsolationScope().getScopeData().transactionName).toEqual('getPrefecture');
+    expect(mockGetActiveSpan).to.toHaveBeenCalledTimes(1);
+    expect(mockSpanSetAttribute).to.toHaveBeenCalledWith('http.route', 'getPrefecture');
+    expect(mockSpanSetAttribute).to.toHaveBeenCalledWith(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route');
   });
 
-  it('sets request data on the isolation scope', async () => {
-    const baggage =
-      'sentry-environment=qa,sentry-public_key=12345678,sentry-trace_id=4c9b164c5b5f4a0c8db3ce490b935ea8,sentry-sample_rate=1,sentry-sampled=true';
-    const sentryTrace = '4c9b164c5b5f4a0c8db3ce490b935ea8';
-    mockGetRequestEvent.mockReturnValue({
-      request: {
-        method: 'POST',
-        url: '/_server',
-        headers: new Headers([
-          ['sentry-trace', sentryTrace],
-          ['baggage', baggage],
-        ]),
-      },
-    });
+  it('does not set a server action name if the active span had a non `/_server` target', async () => {
+    const span = new SentrySpan();
+    span.setAttribute('http.target', '/users/5');
+    mockGetActiveSpan.mockReturnValue(span);
+    const mockSpanSetAttribute = vi.spyOn(span, 'setAttribute');
 
-    await serverActionGetPrefecture();
+    const getPrefecture = async function load() {
+      return withServerActionInstrumentation('getPrefecture', () => {
+        return { prefecture: 'Kagoshima' };
+      });
+    };
 
-    expect(getIsolationScope().getScopeData()).toEqual(
-      expect.objectContaining({
-        sdkProcessingMetadata: {
-          request: {
-            method: 'POST',
-            url: '/_server',
-            headers: {
-              'sentry-trace': sentryTrace,
-              baggage,
-            },
-          },
-        },
-      }),
-    );
+    await getPrefecture();
+
+    expect(mockGetActiveSpan).to.toHaveBeenCalledTimes(1);
+    expect(mockSpanSetAttribute).not.toHaveBeenCalled();
   });
 });