Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Nov 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 29 additions & 2 deletions packages/astro/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
8 changes: 7 additions & 1 deletion packages/astro/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Expand All @@ -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"
},
Expand Down
2 changes: 1 addition & 1 deletion packages/astro/rollup.npm.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
5 changes: 4 additions & 1 deletion packages/astro/src/index.server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 };
16 changes: 15 additions & 1 deletion packages/astro/src/integration/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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 [email protected]
// Users on older versions of astro will need to add the middleware manually.
const supportsAddMiddleware = typeof addMiddleware === 'function';

if (supportsAddMiddleware && isSSR && shouldAddMiddleware) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just thinking, will this break if people update the SDK & astro to the latest version and already have the middleware added manually - e.g. will it then add the middleware twice? or do middlewares (or our middleware) handle this scenario?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point!

will this break if people update the SDK

Technically, we're still in alpha so I'd be fine with this. Furthermore, middleware wasn't yet documented anywhere except for the readme.

However, for general robustness, we can probably detect and handle double wrapping by writing a flag to the locals object that's passed into the middleware handlers. I'll check take a look.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this is not documented yet, IMHO let's just break it! no need to introduce additional complexity then 👍

addMiddleware({
order: 'pre',
entrypoint: '@sentry/astro/middleware',
});
}
},
},
};
Expand Down
16 changes: 16 additions & 0 deletions packages/astro/src/integration/middleware/index.ts
Original file line number Diff line number Diff line change
@@ -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);
};
26 changes: 25 additions & 1 deletion packages/astro/src/integration/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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;
18 changes: 17 additions & 1 deletion packages/astro/src/server/middleware.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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;

Expand Down
84 changes: 84 additions & 0 deletions packages/astro/test/integration/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});
33 changes: 33 additions & 0 deletions packages/astro/test/integration/middleware/index.test.ts
Original file line number Diff line number Diff line change
@@ -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();
});
});
Loading