diff --git a/packages/remix/package.json b/packages/remix/package.json index ea314a25ac5d..83d14ae872e2 100644 --- a/packages/remix/package.json +++ b/packages/remix/package.json @@ -60,10 +60,10 @@ "lint:eslint": "eslint . --cache --cache-location '../../eslintcache/' --format stylish", "lint:prettier": "prettier --check \"{src,test,scripts}/**/*.ts\"", "test": "run-s test:unit", - "test:integration": "run-s test:integration:prepare test:integration:client test:integration:server", - "test:integration:clean": "(cd test/integration && rimraf .cache build node_modules)", - "test:integration:ci": "run-s test:integration:prepare test:integration:client:ci test:integration:server", - "test:integration:prepare": "yarn test:integration:clean && (cd test/integration && yarn)", + "test:integration": "run-s test:integration:clean test:integration:prepare test:integration:client test:integration:server", + "test:integration:ci": "run-s test:integration:clean test:integration:prepare test:integration:client:ci test:integration:server", + "test:integration:prepare": "(cd test/integration && yarn)", + "test:integration:clean": "(cd test/integration && rimraf .cache node_modules build)", "test:integration:client": "yarn playwright install-deps && yarn playwright test test/integration/test/client/", "test:integration:client:ci": "yarn test:integration:client --browser='all' --reporter='line'", "test:integration:server": "jest --config=test/integration/jest.config.js test/integration/test/server/", diff --git a/packages/remix/src/index.server.ts b/packages/remix/src/index.server.ts index 7a113cf2ef44..2576c9a293f1 100644 --- a/packages/remix/src/index.server.ts +++ b/packages/remix/src/index.server.ts @@ -10,6 +10,7 @@ export { ErrorBoundary, withErrorBoundary } from '@sentry/react'; export { remixRouterInstrumentation, withSentry } from './performance/client'; export { BrowserTracing, Integrations } from '@sentry/tracing'; export * from '@sentry/node'; +export { wrapExpressCreateRequestHandler } from './utils/serverAdapters/express'; function sdkAlreadyInitialized(): boolean { const hub = getCurrentHub(); diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index 9ff16e2f716e..b8fce1738171 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -1,103 +1,25 @@ /* eslint-disable max-lines */ import { captureException, getCurrentHub } from '@sentry/node'; -import { getActiveTransaction, hasTracingEnabled } from '@sentry/tracing'; +import { getActiveTransaction, hasTracingEnabled, Span } from '@sentry/tracing'; +import { WrappedFunction } from '@sentry/types'; import { addExceptionMechanism, fill, isNodeEnv, loadModule, logger, serializeBaggage } from '@sentry/utils'; -// Types vendored from @remix-run/server-runtime@1.6.0: -// https://github.com/remix-run/remix/blob/f3691d51027b93caa3fd2cdfe146d7b62a6eb8f2/packages/remix-server-runtime/server.ts -type AppLoadContext = unknown; -type AppData = unknown; -type RequestHandler = (request: Request, loadContext?: AppLoadContext) => Promise; -type CreateRequestHandlerFunction = (build: ServerBuild, mode?: string) => RequestHandler; -type ServerRouteManifest = RouteManifest>; -type Params = { - readonly [key in Key]: string | undefined; -}; - -interface Route { - index?: boolean; - caseSensitive?: boolean; - id: string; - parentId?: string; - path?: string; -} -interface RouteData { - [routeId: string]: AppData; -} - -interface MetaFunction { - (args: { data: AppData; parentsData: RouteData; params: Params; location: Location }): HtmlMetaDescriptor; -} - -interface HtmlMetaDescriptor { - [name: string]: null | string | undefined | Record | Array | string>; - charset?: 'utf-8'; - charSet?: 'utf-8'; - title?: string; -} - -interface ServerRouteModule { - action?: DataFunction; - headers?: unknown; - loader?: DataFunction; - meta?: MetaFunction | HtmlMetaDescriptor; -} - -interface ServerRoute extends Route { - children: ServerRoute[]; - module: ServerRouteModule; -} - -interface RouteManifest { - [routeId: string]: Route; -} - -interface ServerBuild { - entry: { - module: ServerEntryModule; - }; - routes: ServerRouteManifest; - assets: unknown; -} - -interface HandleDocumentRequestFunction { - (request: Request, responseStatusCode: number, responseHeaders: Headers, context: Record): - | Promise - | Response; -} - -interface HandleDataRequestFunction { - (response: Response, args: DataFunctionArgs): Promise | Response; -} - -interface ServerEntryModule { - default: HandleDocumentRequestFunction; - meta: MetaFunction; - loader: DataFunction; - handleDataRequest?: HandleDataRequestFunction; -} - -interface DataFunctionArgs { - request: Request; - context: AppLoadContext; - params: Params; -} - -interface DataFunction { - (args: DataFunctionArgs): Promise | Response | Promise | AppData; -} - -interface ReactRouterDomPkg { - matchRoutes: (routes: ServerRoute[], pathname: string) => RouteMatch[] | null; -} - -// Taken from Remix Implementation -// https://github.com/remix-run/remix/blob/97999d02493e8114c39d48b76944069d58526e8d/packages/remix-server-runtime/routeMatching.ts#L6-L10 -export interface RouteMatch { - params: Params; - pathname: string; - route: Route; -} +import { + AppData, + CreateRequestHandlerFunction, + DataFunction, + DataFunctionArgs, + HandleDocumentRequestFunction, + ReactRouterDomPkg, + RequestHandler, + RouteMatch, + ServerBuild, + ServerRoute, + ServerRouteManifest, +} from './types'; + +// Flag to track if the core request handler is instrumented. +export let isRequestHandlerWrapped = false; // Taken from Remix Implementation // https://github.com/remix-run/remix/blob/32300ec6e6e8025602cea63e17a2201989589eab/packages/remix-server-runtime/responses.ts#L60-L77 @@ -318,7 +240,13 @@ function makeWrappedRootLoader(origLoader: DataFunction): DataFunction { }; } -function createRoutes(manifest: ServerRouteManifest, parentId?: string): ServerRoute[] { +/** + * Creates routes from the server route manifest + * + * @param manifest + * @param parentId + */ +export function createRoutes(manifest: ServerRouteManifest, parentId?: string): ServerRoute[] { return Object.entries(manifest) .filter(([, route]) => route.parentId === parentId) .map(([id, route]) => ({ @@ -352,33 +280,50 @@ function matchServerRoutes( })); } +/** + * Starts a new transaction for the given request to be used by different `RequestHandler` wrappers. + * + * @param request + * @param routes + * @param pkg + */ +export function startRequestHandlerTransaction( + url: URL, + method: string, + routes: ServerRoute[], + pkg?: ReactRouterDomPkg, +): Span | undefined { + const hub = getCurrentHub(); + const currentScope = hub.getScope(); + const matches = matchServerRoutes(routes, url.pathname, pkg); + + const match = matches && getRequestMatch(url, matches); + const name = match === null ? url.pathname : match.route.id; + const source = match === null ? 'url' : 'route'; + const transaction = hub.startTransaction({ + name, + op: 'http.server', + tags: { + method: method, + }, + metadata: { + source, + }, + }); + + if (transaction) { + currentScope?.setSpan(transaction); + } + return transaction; +} + function wrapRequestHandler(origRequestHandler: RequestHandler, build: ServerBuild): RequestHandler { const routes = createRoutes(build.routes); const pkg = loadModule('react-router-dom'); return async function (this: unknown, request: Request, loadContext?: unknown): Promise { - const hub = getCurrentHub(); - const currentScope = hub.getScope(); - const url = new URL(request.url); - const matches = matchServerRoutes(routes, url.pathname, pkg); - - const match = matches && getRequestMatch(url, matches); - const name = match === null ? url.pathname : match.route.id; - const source = match === null ? 'url' : 'route'; - const transaction = hub.startTransaction({ - name, - op: 'http.server', - tags: { - method: request.method, - }, - metadata: { - source, - }, - }); - if (transaction) { - currentScope?.setSpan(transaction); - } + const transaction = startRequestHandlerTransaction(url, request.method, routes, pkg); const res = (await origRequestHandler.call(this, request, loadContext)) as Response; @@ -416,43 +361,60 @@ function getRequestMatch(url: URL, matches: RouteMatch[]): RouteMat return match; } -function makeWrappedCreateRequestHandler( - origCreateRequestHandler: CreateRequestHandlerFunction, -): CreateRequestHandlerFunction { - return function (this: unknown, build: ServerBuild, mode: string | undefined): RequestHandler { - const routes: ServerRouteManifest = {}; +/** + * Instruments `remix` ServerBuild for performance tracing and error tracking. + */ +export function instrumentBuild(build: ServerBuild): ServerBuild { + const routes: ServerRouteManifest = {}; - const wrappedEntry = { ...build.entry, module: { ...build.entry.module } }; + const wrappedEntry = { ...build.entry, module: { ...build.entry.module } }; + // Not keeping boolean flags like it's done for `requestHandler` functions, + // Because the build can change between build and runtime. + // So if there is a new `loader` or`action` or `documentRequest` after build. + // We should be able to wrap them, as they may not be wrapped before. + if (!(wrappedEntry.module.default as WrappedFunction).__sentry_original__) { fill(wrappedEntry.module, 'default', makeWrappedDocumentRequestFunction); + } - for (const [id, route] of Object.entries(build.routes)) { - const wrappedRoute = { ...route, module: { ...route.module } }; - - if (wrappedRoute.module.action) { - fill(wrappedRoute.module, 'action', makeWrappedAction(id)); - } + for (const [id, route] of Object.entries(build.routes)) { + const wrappedRoute = { ...route, module: { ...route.module } }; - if (wrappedRoute.module.loader) { - fill(wrappedRoute.module, 'loader', makeWrappedLoader(id)); - } + if (wrappedRoute.module.action && !(wrappedRoute.module.action as WrappedFunction).__sentry_original__) { + fill(wrappedRoute.module, 'action', makeWrappedAction(id)); + } - // Entry module should have a loader function to provide `sentry-trace` and `baggage` - // They will be available for the root `meta` function as `data.sentryTrace` and `data.sentryBaggage` - if (!wrappedRoute.parentId) { - if (!wrappedRoute.module.loader) { - wrappedRoute.module.loader = () => ({}); - } + if (wrappedRoute.module.loader && !(wrappedRoute.module.loader as WrappedFunction).__sentry_original__) { + fill(wrappedRoute.module, 'loader', makeWrappedLoader(id)); + } - fill(wrappedRoute.module, 'loader', makeWrappedRootLoader); + // Entry module should have a loader function to provide `sentry-trace` and `baggage` + // They will be available for the root `meta` function as `data.sentryTrace` and `data.sentryBaggage` + if (!wrappedRoute.parentId) { + if (!wrappedRoute.module.loader) { + wrappedRoute.module.loader = () => ({}); } - routes[id] = wrappedRoute; + // We want to wrap the root loader regardless of whether it's already wrapped before. + fill(wrappedRoute.module, 'loader', makeWrappedRootLoader); } - const newBuild = { ...build, routes, entry: wrappedEntry }; + routes[id] = wrappedRoute; + } + + return { ...build, routes, entry: wrappedEntry }; +} + +function makeWrappedCreateRequestHandler( + origCreateRequestHandler: CreateRequestHandlerFunction, +): CreateRequestHandlerFunction { + // To track if this wrapper has been applied, before other wrappers. + // Can't track `__sentry_original__` because it's not the same function as the potentially manually wrapped one. + isRequestHandlerWrapped = true; - const requestHandler = origCreateRequestHandler.call(this, newBuild, mode); + return function (this: unknown, build: ServerBuild, ...args: unknown[]): RequestHandler { + const newBuild = instrumentBuild(build); + const requestHandler = origCreateRequestHandler.call(this, newBuild, ...args); return wrapRequestHandler(requestHandler, newBuild); }; diff --git a/packages/remix/src/utils/serverAdapters/express.ts b/packages/remix/src/utils/serverAdapters/express.ts new file mode 100644 index 000000000000..b67ec5187705 --- /dev/null +++ b/packages/remix/src/utils/serverAdapters/express.ts @@ -0,0 +1,67 @@ +import { extractRequestData, loadModule } from '@sentry/utils'; + +import { + createRoutes, + instrumentBuild, + isRequestHandlerWrapped, + startRequestHandlerTransaction, +} from '../instrumentServer'; +import { + ExpressCreateRequestHandler, + ExpressCreateRequestHandlerOptions, + ExpressNextFunction, + ExpressRequest, + ExpressRequestHandler, + ExpressResponse, + ReactRouterDomPkg, + ServerBuild, +} from '../types'; + +function wrapExpressRequestHandler( + origRequestHandler: ExpressRequestHandler, + build: ServerBuild, +): ExpressRequestHandler { + const routes = createRoutes(build.routes); + const pkg = loadModule('react-router-dom'); + + // If the core request handler is already wrapped, don't wrap Express handler which uses it. + if (isRequestHandlerWrapped) { + return origRequestHandler; + } + + return async function ( + this: unknown, + req: ExpressRequest, + res: ExpressResponse, + next: ExpressNextFunction, + ): Promise { + const request = extractRequestData(req); + + if (!request.url || !request.method) { + return origRequestHandler.call(this, req, res, next); + } + + const url = new URL(request.url); + + const transaction = startRequestHandlerTransaction(url, request.method, routes, pkg); + + await origRequestHandler.call(this, req, res, next); + + transaction?.setHttpStatus(res.statusCode); + transaction?.finish(); + }; +} + +/** + * Instruments `createRequestHandler` from `@remix-run/express` + */ +export function wrapExpressCreateRequestHandler( + origCreateRequestHandler: ExpressCreateRequestHandler, +): (options: any) => ExpressRequestHandler { + return function (this: unknown, options: any): ExpressRequestHandler { + const newBuild = instrumentBuild((options as ExpressCreateRequestHandlerOptions).build); + const requestHandler = origCreateRequestHandler.call(this, { ...options, build: newBuild }); + + return wrapExpressRequestHandler(requestHandler, newBuild); + }; +} diff --git a/packages/remix/src/utils/types.ts b/packages/remix/src/utils/types.ts new file mode 100644 index 000000000000..e4c4ca6f35e7 --- /dev/null +++ b/packages/remix/src/utils/types.ts @@ -0,0 +1,167 @@ +/* eslint-disable max-lines */ +/* eslint-disable @typescript-eslint/ban-types */ +// Types vendored from @remix-run/server-runtime@1.6.0: +// https://github.com/remix-run/remix/blob/f3691d51027b93caa3fd2cdfe146d7b62a6eb8f2/packages/remix-server-runtime/server.ts +import type * as Express from 'express'; +import type { ComponentType } from 'react'; + +export type AppLoadContext = any; +export type AppData = any; +export type RequestHandler = (request: Request, loadContext?: AppLoadContext) => Promise; +export type CreateRequestHandlerFunction = (this: unknown, build: ServerBuild, ...args: any[]) => RequestHandler; +export type ServerRouteManifest = RouteManifest>; +export type Params = { + readonly [key in Key]: string | undefined; +}; + +export type ExpressRequest = Express.Request; +export type ExpressResponse = Express.Response; +export type ExpressNextFunction = Express.NextFunction; + +export interface Route { + index?: boolean; + caseSensitive?: boolean; + id: string; + parentId?: string; + path?: string; +} + +export interface EntryRoute extends Route { + hasAction: boolean; + hasLoader: boolean; + hasCatchBoundary: boolean; + hasErrorBoundary: boolean; + imports?: string[]; + module: string; +} + +export interface RouteData { + [routeId: string]: AppData; +} + +export interface MetaFunction { + (args: { data: AppData; parentsData: RouteData; params: Params; location: Location }): HtmlMetaDescriptor; +} + +export interface HtmlMetaDescriptor { + [name: string]: null | string | undefined | Record | Array | string>; + charset?: 'utf-8'; + charSet?: 'utf-8'; + title?: string; +} + +export type CatchBoundaryComponent = ComponentType<{}>; +export type RouteComponent = ComponentType<{}>; +export type ErrorBoundaryComponent = ComponentType<{ error: Error }>; +export type RouteHandle = any; +export interface LinksFunction { + (): any[]; +} +export interface EntryRouteModule { + CatchBoundary?: CatchBoundaryComponent; + ErrorBoundary?: ErrorBoundaryComponent; + default: RouteComponent; + handle?: RouteHandle; + links?: LinksFunction; + meta?: MetaFunction | HtmlMetaDescriptor; +} + +export interface ActionFunction { + (args: DataFunctionArgs): Promise | Response | Promise | AppData; +} + +export interface LoaderFunction { + (args: DataFunctionArgs): Promise | Response | Promise | AppData; +} + +export interface HeadersFunction { + (args: { loaderHeaders: Headers; parentHeaders: Headers; actionHeaders: Headers }): Headers | HeadersInit; +} + +export interface ServerRouteModule extends EntryRouteModule { + action?: ActionFunction; + headers?: HeadersFunction | { [name: string]: string }; + loader?: LoaderFunction; +} + +export interface ServerRoute extends Route { + children: ServerRoute[]; + module: ServerRouteModule; +} + +export interface RouteManifest { + [routeId: string]: Route; +} + +export interface ServerBuild { + entry: { + module: ServerEntryModule; + }; + routes: ServerRouteManifest; + assets: AssetsManifest; + publicPath?: string; + assetsBuildDirectory?: string; +} + +export interface HandleDocumentRequestFunction { + (request: Request, responseStatusCode: number, responseHeaders: Headers, context: EntryContext): + | Promise + | Response; +} + +export interface HandleDataRequestFunction { + (response: Response, args: DataFunctionArgs): Promise | Response; +} + +interface ServerEntryModule { + default: HandleDocumentRequestFunction; + handleDataRequest?: HandleDataRequestFunction; +} + +export interface DataFunctionArgs { + request: Request; + context: AppLoadContext; + params: Params; +} + +export interface DataFunction { + (args: DataFunctionArgs): Promise | Response | Promise | AppData; +} + +export interface ReactRouterDomPkg { + matchRoutes: (routes: ServerRoute[], pathname: string) => RouteMatch[] | null; +} + +// Taken from Remix Implementation +// https://github.com/remix-run/remix/blob/97999d02493e8114c39d48b76944069d58526e8d/packages/remix-server-runtime/routeMatching.ts#L6-L10 +export interface RouteMatch { + params: Params; + pathname: string; + route: Route; +} + +export interface EntryContext { + [name: string]: any; +} + +export interface AssetsManifest { + entry: { + imports: string[]; + module: string; + }; + routes: RouteManifest; + url: string; + version: string; +} + +export type ExpressRequestHandler = (req: any, res: any, next: any) => Promise; + +export type ExpressCreateRequestHandler = (this: unknown, options: any) => ExpressRequestHandler; + +export interface ExpressCreateRequestHandlerOptions { + build: ServerBuild; + getLoadContext?: GetLoadContextFunction; + mode?: string; +} + +type GetLoadContextFunction = (req: any, res: any) => any; diff --git a/packages/remix/test/integration/app/routes/action-json-response/$id.tsx b/packages/remix/test/integration/app/routes/action-json-response/$id.tsx index 4763e37282d4..588c340b2c23 100644 --- a/packages/remix/test/integration/app/routes/action-json-response/$id.tsx +++ b/packages/remix/test/integration/app/routes/action-json-response/$id.tsx @@ -13,7 +13,7 @@ export const action: ActionFunction = async ({ params: { id } }) => { } if (id === '-2') { - // Note: This GET request triggers to the `Loader` of the URL, not the `Action`. + // Note: This GET request triggers the `Loader` of the URL, not the `Action`. throw redirect('/action-json-response/-1'); } diff --git a/packages/remix/test/integration/test/server/action.test.ts b/packages/remix/test/integration/test/server/action.test.ts index af350e8cb7ad..a1fd810af020 100644 --- a/packages/remix/test/integration/test/server/action.test.ts +++ b/packages/remix/test/integration/test/server/action.test.ts @@ -8,9 +8,10 @@ import { jest.spyOn(console, 'error').mockImplementation(); -describe('Remix API Actions', () => { +// Repeat tests for each adapter +describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', adapter => { it('correctly instruments a parameterized Remix API action', async () => { - const baseURL = await runServer(); + const baseURL = await runServer(adapter); const url = `${baseURL}/action-json-response/123123`; const envelope = await getEnvelopeRequest(url, 'post'); const transaction = envelope[2]; @@ -39,7 +40,7 @@ describe('Remix API Actions', () => { }); it('reports an error thrown from the action', async () => { - const baseURL = await runServer(); + const baseURL = await runServer(adapter); const url = `${baseURL}/action-json-response/-1`; const [transaction, event] = await getMultipleEnvelopeRequest(url, 2, 'post'); @@ -76,7 +77,7 @@ describe('Remix API Actions', () => { }); it('handles a thrown 500 response', async () => { - const baseURL = await runServer(); + const baseURL = await runServer(adapter); const url = `${baseURL}/action-json-response/-2`; const [transaction_1, event, transaction_2] = await getMultipleEnvelopeRequest(url, 3, 'post'); diff --git a/packages/remix/test/integration/test/server/loader.test.ts b/packages/remix/test/integration/test/server/loader.test.ts index 267658debbfb..be0d2c06c609 100644 --- a/packages/remix/test/integration/test/server/loader.test.ts +++ b/packages/remix/test/integration/test/server/loader.test.ts @@ -8,12 +8,17 @@ import { jest.spyOn(console, 'error').mockImplementation(); -describe('Remix API Loaders', () => { +// Repeat tests for each adapter +describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', adapter => { it('reports an error thrown from the loader', async () => { - const baseURL = await runServer(); + const baseURL = await runServer(adapter); const url = `${baseURL}/loader-json-response/-2`; - const [transaction, event] = await getMultipleEnvelopeRequest(url, 2); + let [transaction, event] = await getMultipleEnvelopeRequest(url, 2); + + // The event envelope is returned before the transaction envelope when using express adapter. + // We can update this when we merge the envelope filtering utility. + adapter === 'express' && ([event, transaction] = [transaction, event]); assertSentryTransaction(transaction[2], { contexts: { @@ -47,7 +52,7 @@ describe('Remix API Loaders', () => { }); it('correctly instruments a parameterized Remix API loader', async () => { - const baseURL = await runServer(); + const baseURL = await runServer(adapter); const url = `${baseURL}/loader-json-response/123123`; const envelope = await getEnvelopeRequest(url); const transaction = envelope[2]; @@ -75,7 +80,7 @@ describe('Remix API Loaders', () => { }); it('handles a thrown 500 response', async () => { - const baseURL = await runServer(); + const baseURL = await runServer(adapter); const url = `${baseURL}/loader-json-response/-1`; const [transaction_1, event, transaction_2] = await getMultipleEnvelopeRequest(url, 3); diff --git a/packages/remix/test/integration/test/server/utils/helpers.ts b/packages/remix/test/integration/test/server/utils/helpers.ts index dcce78644415..1833ea878e1c 100644 --- a/packages/remix/test/integration/test/server/utils/helpers.ts +++ b/packages/remix/test/integration/test/server/utils/helpers.ts @@ -1,6 +1,7 @@ import express from 'express'; import { createRequestHandler } from '@remix-run/express'; import { getPortPromise } from 'portfinder'; +import { wrapExpressCreateRequestHandler } from '@sentry/remix'; export * from '../../../../../../node-integration-tests/utils'; @@ -8,11 +9,14 @@ export * from '../../../../../../node-integration-tests/utils'; * Runs a test server * @returns URL */ -export async function runServer(): Promise { +export async function runServer(adapter: string = 'builtin'): Promise { + const requestHandlerFactory = + adapter === 'express' ? wrapExpressCreateRequestHandler(createRequestHandler) : createRequestHandler; + const app = express(); const port = await getPortPromise(); - app.all('*', createRequestHandler({ build: require('../../../build') })); + app.all('*', requestHandlerFactory({ build: require('../../../build') })); const server = app.listen(port, () => { setTimeout(() => {