feat(solidstart): Add server action instrumentation helper (#13035)

andreiborza · web-flow · commit 71af30f5150e · 2024-07-30T14:56:49.000+02:00
Can be used like this:

```js
const getUserData = async () =&gt; {
  'use server';
  return await withServerActionInstrumentation('getData', () =&gt; {
    return { prefecture: 'Kanagawa' };
  });
};
```

Can also be used for api routes like this:

```js
export async function GET() {
    return await withServerActionInstrumentation('getUser', () =&gt; {
        return json({ prefecture: 'Akita' })
    })
}
```
diff --git a/dev-packages/e2e-tests/test-applications/solidstart/package.json b/dev-packages/e2e-tests/test-applications/solidstart/package.json
@@ -11,6 +11,7 @@
       "This is currently not an issue outside of our repo. See: https://github.com/nksaraf/vinxi/issues/177"
     ],
     "preview": "HOST=localhost PORT=3030 NODE_OPTIONS='--import ./src/instrument.server.mjs' vinxi dev",
+    "start": "HOST=localhost PORT=3030 NODE_OPTIONS='--import ./src/instrument.server.mjs' vinxi start",
     "test:prod": "TEST_ENV=production playwright test",
     "test:build": "pnpm install && npx playwright install && pnpm build",
     "test:assert": "pnpm test:prod"
@@ -31,7 +32,7 @@
     "jsdom": "^24.0.0",
     "solid-js": "1.8.17",
     "typescript": "^5.4.5",
-    "vinxi": "^0.3.12",
+    "vinxi": "^0.4.0",
     "vite": "^5.2.8",
     "vite-plugin-solid": "^2.10.2",
     "vitest": "^1.5.0"
diff --git a/dev-packages/e2e-tests/test-applications/solidstart/src/entry-client.tsx b/dev-packages/e2e-tests/test-applications/solidstart/src/entry-client.tsx
@@ -12,6 +12,7 @@ Sentry.init({
   tunnel: 'http://localhost:3031/', // proxy server
   // Performance Monitoring
   tracesSampleRate: 1.0, //  Capture 100% of the transactions
+  debug: !!import.meta.env.DEBUG,
 });
 
 mount(() => <StartClient />, document.getElementById('app')!);
diff --git a/dev-packages/e2e-tests/test-applications/solidstart/src/instrument.server.mjs b/dev-packages/e2e-tests/test-applications/solidstart/src/instrument.server.mjs
@@ -5,4 +5,5 @@ Sentry.init({
   environment: 'qa', // dynamic sampling bias to keep transactions
   tracesSampleRate: 1.0, //  Capture 100% of the transactions
   tunnel: 'http://localhost:3031/', // proxy server
+  debug: !!process.env.DEBUG,
 });
diff --git a/dev-packages/e2e-tests/test-applications/solidstart/src/routes/index.tsx b/dev-packages/e2e-tests/test-applications/solidstart/src/routes/index.tsx
@@ -11,6 +11,9 @@ export default function Home() {
         <li>
           <A href="/client-error">Client error</A>
         </li>
+        <li>
+          <A href="/server-error">Server error</A>
+        </li>
         <li>
           <A id="navLink" href="/users/5">
             User 5
diff --git a/dev-packages/e2e-tests/test-applications/solidstart/src/routes/server-error.tsx b/dev-packages/e2e-tests/test-applications/solidstart/src/routes/server-error.tsx
@@ -0,0 +1,17 @@
+import { withServerActionInstrumentation } from '@sentry/solidstart';
+import { createAsync } from '@solidjs/router';
+
+const getPrefecture = async () => {
+  'use server';
+  return await withServerActionInstrumentation('getPrefecture', () => {
+    throw new Error('Error thrown from Solid Start E2E test app server route');
+
+    return { prefecture: 'Kanagawa' };
+  });
+};
+
+export default function ServerErrorPage() {
+  const data = createAsync(() => getPrefecture());
+
+  return <div>Prefecture: {data()?.prefecture}</div>;
+}
diff --git a/dev-packages/e2e-tests/test-applications/solidstart/src/routes/users/[id].tsx b/dev-packages/e2e-tests/test-applications/solidstart/src/routes/users/[id].tsx
@@ -1,6 +1,21 @@
-import { useParams } from '@solidjs/router';
+import { withServerActionInstrumentation } from '@sentry/solidstart';
+import { createAsync, useParams } from '@solidjs/router';
 
+const getPrefecture = async () => {
+  'use server';
+  return await withServerActionInstrumentation('getPrefecture', () => {
+    return { prefecture: 'Ehime' };
+  });
+};
 export default function User() {
   const params = useParams();
-  return <div>User ID: {params.id}</div>;
+  const userData = createAsync(() => getPrefecture());
+
+  return (
+    <div>
+      User ID: {params.id}
+      <br />
+      Prefecture: {userData()?.prefecture}
+    </div>
+  );
 }
diff --git a/dev-packages/e2e-tests/test-applications/solidstart/tests/errors.server.test.ts b/dev-packages/e2e-tests/test-applications/solidstart/tests/errors.server.test.ts
@@ -0,0 +1,31 @@
+import { expect, test } from '@playwright/test';
+import { waitForError } from '@sentry-internal/test-utils';
+
+test.describe('server-side errors', () => {
+  test('captures server action error', async ({ page }) => {
+    const errorEventPromise = waitForError('solidstart', errorEvent => {
+      return errorEvent?.exception?.values?.[0]?.value === 'Error thrown from Solid Start E2E test app server route';
+    });
+
+    await page.goto(`/server-error`);
+
+    const error = await errorEventPromise;
+
+    expect(error.tags).toMatchObject({ runtime: 'node' });
+    expect(error).toMatchObject({
+      exception: {
+        values: [
+          {
+            type: 'Error',
+            value: 'Error thrown from Solid Start E2E test app server route',
+            mechanism: {
+              type: 'solidstart',
+              handled: false,
+            },
+          },
+        ],
+      },
+      transaction: 'GET /server-error',
+    });
+  });
+});
diff --git a/dev-packages/e2e-tests/test-applications/solidstart/tests/performance.server.test.ts b/dev-packages/e2e-tests/test-applications/solidstart/tests/performance.server.test.ts
@@ -0,0 +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 on pageload', async ({ page }) => {
+  const transactionPromise = waitForTransaction('solidstart', transactionEvent => {
+    return transactionEvent?.transaction === 'GET /users/6';
+  });
+
+  await page.goto('/users/6');
+
+  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]: 'auto.function.solidstart',
+          [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'component',
+        },
+      }),
+    ]),
+  );
+});
+
+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]: 'auto.function.solidstart',
+          [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'component',
+        },
+      }),
+    ]),
+  );
+});
diff --git a/packages/solidstart/package.json b/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",
diff --git a/packages/solidstart/rollup.npm.config.mjs b/packages/solidstart/rollup.npm.config.mjs
@@ -16,7 +16,7 @@ export default makeNPMConfigVariants(
     // prevent this internal code from ending up in our built package (this doesn't happen automatially because
     // the name doesn't match an SDK dependency)
     packageSpecificConfig: {
-      external: ['solid-js', '@sentry/solid', '@sentry/solid/solidrouter'],
+      external: ['solid-js/web', 'solid-js', '@sentry/solid', '@sentry/solid/solidrouter'],
       output: {
         dynamicImportInCjs: true,
       },
diff --git a/packages/solidstart/src/index.types.ts b/packages/solidstart/src/index.types.ts
@@ -1,5 +1,5 @@
 // We export everything from both the client part of the SDK and from the server part.
-// Some of the exports collide, which is not allowed, unless we redifine the colliding
+// Some of the exports collide, which is not allowed, unless we redefine the colliding
 // exports in this file - which we do below.
 export * from './client';
 export * from './server';
diff --git a/packages/solidstart/src/server/index.ts b/packages/solidstart/src/server/index.ts
@@ -126,3 +126,5 @@ export { withSentryErrorBoundary } from '@sentry/solid';
 // -------------------------
 // Solid Start SDK exports:
 export { init } from './sdk';
+
+export * from './withServerActionInstrumentation';
diff --git a/packages/solidstart/src/server/utils.ts b/packages/solidstart/src/server/utils.ts
@@ -0,0 +1,33 @@
+import { flush } from '@sentry/node';
+import { logger } from '@sentry/utils';
+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);
+    }
+  }
+}
+
+/**
+ * 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
+ * @param error the potential redirect error
+ */
+export function isRedirect(error: unknown): boolean {
+  if (error == null || !(error instanceof Response)) {
+    return false;
+  }
+
+  const hasValidLocation = typeof error.headers.get('location') === 'string';
+  const hasValidStatus = error.status >= 300 && error.status <= 308;
+  return hasValidLocation && hasValidStatus;
+}
diff --git a/packages/solidstart/src/server/withServerActionInstrumentation.ts b/packages/solidstart/src/server/withServerActionInstrumentation.ts
@@ -0,0 +1,58 @@
+import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SPAN_STATUS_ERROR, handleCallbackErrors } from '@sentry/core';
+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.
+ */
+export async function withServerActionInstrumentation<A extends (...args: unknown[]) => unknown>(
+  serverActionName: string,
+  callback: A,
+): Promise<ReturnType<A>> {
+  const activeSpan = getActiveSpan();
+
+  if (activeSpan) {
+    const spanData = spanToJSON(activeSpan).data;
+
+    // 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, 'component');
+    }
+  }
+
+  try {
+    return await startSpan(
+      {
+        op: 'function.server_action',
+        name: serverActionName,
+        attributes: {
+          [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.function.solidstart',
+          [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'component',
+        },
+      },
+      async span => {
+        const result = await handleCallbackErrors(callback, error => {
+          if (!isRedirect(error)) {
+            span.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' });
+            captureException(error, {
+              mechanism: {
+                handled: false,
+                type: 'solidstart',
+              },
+            });
+          }
+        });
+
+        return result;
+      },
+    );
+  } finally {
+    await flushIfServerless();
+  }
+}
diff --git a/packages/solidstart/test/server/withServerActionInstrumentation.test.ts b/packages/solidstart/test/server/withServerActionInstrumentation.test.ts
diff --git a/scripts/node-unit-tests.ts b/scripts/node-unit-tests.ts
