feat(astro): Automatically add Sentry middleware in Astro integration (#9532)

Lms24 · web-flow · commit 61bcf73ef28b · 2023-11-27T14:26:48.000+01:00
This change adds automatic registration of our Astro middleware. This is possible since Astro 3.5.2 by [adding middleware](https://docs.astro.build/en/reference/integrations-reference/#addmiddleware-option) entry points in the astro integration's setup hook. This is backwards compatible with previous Astro versions because we can simply check if the `addMiddleware` function exists and only make use of it if it does.
diff --git a/packages/astro/README.md b/packages/astro/README.md
@@ -56,7 +56,11 @@ Follow [this guide](https://docs.sentry.io/product/accounts/auth-tokens/#organiz
 SENTRY_AUTH_TOKEN="your-token"
 ```
 
-Complete the setup by adding the Sentry middleware to your `src/middleware.js` file:
+### Server Instrumentation
+
+For Astro apps configured for (hybrid) Server Side Rendering (SSR), the Sentry integration will automatically add middleware to your server to instrument incoming requests **if you're using Astro 3.5.0 or newer**.
+
+If you're using Astro <3.5.0, complete the setup by adding the Sentry middleware to your `src/middleware.js` file:
 
 ```javascript
 // src/middleware.js
@@ -69,7 +73,30 @@ export const onRequest = sequence(
 );
 ```
 
-This middleware creates server-side spans to monitor performance on the server for page load and endpoint requests.
+The Sentry middleware enhances the data collected by Sentry on the server side by:
+- Enabeling distributed tracing between client and server
+- Collecting performance spans for incoming requests
+- Enhancing captured errors with additional information
+
+#### Disable Automatic Server Instrumentation
+
+You can opt out of using the automatic sentry server instrumentation in your `astro.config.mjs` file:
+
+```javascript
+import { defineConfig } from "astro/config";
+import sentry from "@sentry/astro";
+
+export default defineConfig({
+  integrations: [
+    sentry({
+      dsn: "__DSN__",
+      autoInstrumentation: {
+        requestHandler: false,
+      }
+    }),
+  ],
+});
+```
 
 
 ## Configuration
diff --git a/packages/astro/package.json b/packages/astro/package.json
@@ -28,6 +28,12 @@
       "import": "./build/esm/index.client.js",
       "require": "./build/cjs/index.server.js",
       "types": "./build/types/index.types.d.ts"
+    },
+    "./middleware": {
+      "node": "./build/esm/integration/middleware/index.js",
+      "import": "./build/esm/integration/middleware/index.js",
+      "require": "./build/cjs/integration/middleware/index.js",
+      "types": "./build/types/integration/middleware/index.types.d.ts"
     }
   },
   "publishConfig": {
@@ -45,7 +51,7 @@
     "@sentry/vite-plugin": "^2.8.0"
   },
   "devDependencies": {
-    "astro": "^3.2.3",
+    "astro": "^3.5.0",
     "rollup": "^3.20.2",
     "vite": "4.0.5"
   },
diff --git a/packages/astro/rollup.npm.config.js b/packages/astro/rollup.npm.config.js
@@ -2,7 +2,7 @@ import { makeBaseNPMConfig, makeNPMConfigVariants } from '../../rollup/index.js'
 
 const variants = makeNPMConfigVariants(
   makeBaseNPMConfig({
-    entrypoints: ['src/index.server.ts', 'src/index.client.ts'],
+    entrypoints: ['src/index.server.ts', 'src/index.client.ts', 'src/integration/middleware/index.ts'],
     packageSpecificConfig: {
       output: {
         dynamicImportInCjs: true,
diff --git a/packages/astro/src/index.server.ts b/packages/astro/src/index.server.ts
@@ -4,6 +4,7 @@
 // on the top - level namespace.
 
 import { sentryAstro } from './integration';
+import { handleRequest } from './server/middleware';
 
 // Hence, we export everything from the Node SDK explicitly:
 export {
@@ -64,6 +65,8 @@ export {
 export * from '@sentry/node';
 
 export { init } from './server/sdk';
-export { handleRequest } from './server/middleware';
 
 export default sentryAstro;
+
+// This exports the `handleRequest` middleware for manual usage
+export { handleRequest };
diff --git a/packages/astro/src/integration/index.ts b/packages/astro/src/integration/index.ts
@@ -14,7 +14,7 @@ export const sentryAstro = (options: SentryOptions = {}): AstroIntegration => {
     name: PKG_NAME,
     hooks: {
       // eslint-disable-next-line complexity
-      'astro:config:setup': async ({ updateConfig, injectScript, config }) => {
+      'astro:config:setup': async ({ updateConfig, injectScript, addMiddleware, config }) => {
         // The third param here enables loading of all env vars, regardless of prefix
         // see: https://main.vitejs.dev/config/#using-environment-variables-in-config
 
@@ -73,6 +73,20 @@ export const sentryAstro = (options: SentryOptions = {}): AstroIntegration => {
           options.debug && console.log('[sentry-astro] Using default server init.');
           injectScript('page-ssr', buildServerSnippet(options || {}));
         }
+
+        const isSSR = config && (config.output === 'server' || config.output === 'hybrid');
+        const shouldAddMiddleware = options.autoInstrumentation?.requestHandler !== false;
+
+        // Guarding calling the addMiddleware function because it was only introduced in astro@3.5.0
+        // Users on older versions of astro will need to add the middleware manually.
+        const supportsAddMiddleware = typeof addMiddleware === 'function';
+
+        if (supportsAddMiddleware && isSSR && shouldAddMiddleware) {
+          addMiddleware({
+            order: 'pre',
+            entrypoint: '@sentry/astro/middleware',
+          });
+        }
       },
     },
   };
diff --git a/packages/astro/src/integration/middleware/index.ts b/packages/astro/src/integration/middleware/index.ts
@@ -0,0 +1,16 @@
+import type { MiddlewareResponseHandler } from 'astro';
+
+import { handleRequest } from '../../server/middleware';
+
+/**
+ * This export is used by our integration to automatically add the middleware
+ * to astro ^3.5.0 projects.
+ *
+ * It's not possible to pass options at this moment, so we'll call our middleware
+ * factory function with the default options. Users can deactiveate the automatic
+ * middleware registration in our integration and manually add it in their own
+ * `/src/middleware.js` file.
+ */
+export const onRequest: MiddlewareResponseHandler = (ctx, next) => {
+  return handleRequest()(ctx, next);
+};
diff --git a/packages/astro/src/integration/types.ts b/packages/astro/src/integration/types.ts
@@ -71,6 +71,29 @@ type SourceMapsOptions = {
   };
 };
 
+type InstrumentationOptions = {
+  /**
+   * Options for automatic instrumentation of your application.
+   */
+  autoInstrumentation?: {
+    /**
+     * If this flag is `true` and your application is configured for SSR (or hybrid) mode,
+     * the Sentry integration will automatically add middleware to:
+     *
+     * - capture server performance data and spans for incoming server requests
+     * - enable distributed tracing between server and client
+     * - annotate server errors with more information
+     *
+     * This middleware will only be added automatically in Astro 3.5.0 and newer.
+     * For older versions, add the `Sentry.handleRequest` middleware manually
+     * in your `src/middleware.js` file.
+     *
+     * @default true in SSR/hybrid mode, false in SSG/static mode
+     */
+    requestHandler?: boolean;
+  };
+};
+
 /**
  * A subset of Sentry SDK options that can be set via the `sentryAstro` integration.
  * Some options (e.g. integrations) are set by default and cannot be changed here.
@@ -83,4 +106,5 @@ type SourceMapsOptions = {
 export type SentryOptions = SdkInitPaths &
   Pick<Options, 'dsn' | 'release' | 'environment' | 'sampleRate' | 'tracesSampleRate' | 'debug'> &
   Pick<BrowserOptions, 'replaysSessionSampleRate' | 'replaysOnErrorSampleRate'> &
-  SourceMapsOptions;
+  SourceMapsOptions &
+  InstrumentationOptions;
diff --git a/packages/astro/src/server/middleware.ts b/packages/astro/src/server/middleware.ts
@@ -1,6 +1,11 @@
 import { captureException, configureScope, getCurrentHub, startSpan } from '@sentry/node';
 import type { Hub, Span } from '@sentry/types';
-import { objectify, stripUrlQueryAndFragment, tracingContextFromHeaders } from '@sentry/utils';
+import {
+  addNonEnumerableProperty,
+  objectify,
+  stripUrlQueryAndFragment,
+  tracingContextFromHeaders,
+} from '@sentry/utils';
 import type { APIContext, MiddlewareResponseHandler } from 'astro';
 
 import { getTracingMetaTags } from './meta';
@@ -47,10 +52,21 @@ function sendErrorToSentry(e: unknown): unknown {
   return objectifiedErr;
 }
 
+type AstroLocalsWithSentry = Record<string, unknown> & {
+  __sentry_wrapped__?: boolean;
+};
+
 export const handleRequest: (options?: MiddlewareOptions) => MiddlewareResponseHandler = (
   options = { trackClientIp: false, trackHeaders: false },
 ) => {
   return async (ctx, next) => {
+    // Make sure we don't accidentally double wrap (e.g. user added middleware and integration auto added it)
+    const locals = ctx.locals as AstroLocalsWithSentry;
+    if (locals && locals.__sentry_wrapped__) {
+      return next();
+    }
+    addNonEnumerableProperty(locals, '__sentry_wrapped__', true);
+
     const method = ctx.request.method;
     const headers = ctx.request.headers;
 
diff --git a/packages/astro/test/integration/index.test.ts b/packages/astro/test/integration/index.test.ts
@@ -122,4 +122,88 @@ describe('sentryAstro integration', () => {
     expect(injectScript).toHaveBeenCalledWith('page', expect.stringContaining('my-client-init-path.js'));
     expect(injectScript).toHaveBeenCalledWith('page-ssr', expect.stringContaining('my-server-init-path.js'));
   });
+
+  it.each(['server', 'hybrid'])(
+    'adds middleware by default if in %s mode and `addMiddleware` is available',
+    async mode => {
+      const integration = sentryAstro({});
+      const addMiddleware = vi.fn();
+      const updateConfig = vi.fn();
+      const injectScript = vi.fn();
+
+      expect(integration.hooks['astro:config:setup']).toBeDefined();
+      // @ts-expect-error - the hook exists and we only need to pass what we actually use
+      await integration.hooks['astro:config:setup']({
+        // @ts-expect-error - we only need to pass what we actually use
+        config: { output: mode },
+        addMiddleware,
+        updateConfig,
+        injectScript,
+      });
+
+      expect(addMiddleware).toHaveBeenCalledTimes(1);
+      expect(addMiddleware).toHaveBeenCalledWith({
+        order: 'pre',
+        entrypoint: '@sentry/astro/middleware',
+      });
+    },
+  );
+
+  it.each([{ output: 'static' }, { output: undefined }])(
+    "doesn't add middleware if in static mode (config %s)",
+    async config => {
+      const integration = sentryAstro({});
+      const addMiddleware = vi.fn();
+      const updateConfig = vi.fn();
+      const injectScript = vi.fn();
+
+      expect(integration.hooks['astro:config:setup']).toBeDefined();
+      // @ts-expect-error - the hook exists and we only need to pass what we actually use
+      await integration.hooks['astro:config:setup']({
+        config,
+        addMiddleware,
+        updateConfig,
+        injectScript,
+      });
+
+      expect(addMiddleware).toHaveBeenCalledTimes(0);
+    },
+  );
+
+  it("doesn't add middleware if disabled by users", async () => {
+    const integration = sentryAstro({ autoInstrumentation: { requestHandler: false } });
+    const addMiddleware = vi.fn();
+    const updateConfig = vi.fn();
+    const injectScript = vi.fn();
+
+    expect(integration.hooks['astro:config:setup']).toBeDefined();
+    // @ts-expect-error - the hook exists and we only need to pass what we actually use
+    await integration.hooks['astro:config:setup']({
+      // @ts-expect-error - we only need to pass what we actually use
+      config: { output: 'server' },
+      addMiddleware,
+      updateConfig,
+      injectScript,
+    });
+
+    expect(addMiddleware).toHaveBeenCalledTimes(0);
+  });
+
+  it("doesn't add middleware (i.e. crash) if `addMiddleware` is N/A", async () => {
+    const integration = sentryAstro({ autoInstrumentation: { requestHandler: false } });
+    const updateConfig = vi.fn();
+    const injectScript = vi.fn();
+
+    expect(integration.hooks['astro:config:setup']).toBeDefined();
+    // @ts-expect-error - the hook exists and we only need to pass what we actually use
+    await integration.hooks['astro:config:setup']({
+      // @ts-expect-error - we only need to pass what we actually use
+      config: { output: 'server' },
+      updateConfig,
+      injectScript,
+    });
+
+    expect(updateConfig).toHaveBeenCalledTimes(1);
+    expect(injectScript).toHaveBeenCalledTimes(2);
+  });
 });
diff --git a/packages/astro/test/integration/middleware/index.test.ts b/packages/astro/test/integration/middleware/index.test.ts
@@ -0,0 +1,33 @@
+import { vi } from 'vitest';
+
+import { onRequest } from '../../../src/integration/middleware';
+
+vi.mock('../../../src/server/meta', () => ({
+  getTracingMetaTags: () => ({
+    sentryTrace: '<meta name="sentry-trace" content="123">',
+    baggage: '<meta name="baggage" content="abc">',
+  }),
+}));
+
+describe('Integration middleware', () => {
+  it('exports an onRequest middleware request handler', async () => {
+    expect(typeof onRequest).toBe('function');
+
+    const next = vi.fn().mockReturnValue(Promise.resolve(new Response(null, { status: 200, headers: new Headers() })));
+    const ctx = {
+      request: {
+        method: 'GET',
+        url: '/users/123/details',
+        headers: new Headers(),
+      },
+      url: new URL('https://myDomain.io/users/123/details'),
+      params: {
+        id: '123',
+      },
+    };
+    // @ts-expect-error - a partial ctx object is fine here
+    const res = await onRequest(ctx, next);
+
+    expect(res).toBeDefined();
+  });
+});
diff --git a/yarn.lock b/yarn.lock
