Skip to content

Commit 1718e98

Browse files
onurtemizkanonurtemizkan
authored
feat(remix): Export a manual wrapper for custom Express servers. (#5524)
Remix allows custom server declarations and exports `createRequestHandler` functions from each [adapter](https://remix.run/docs/en/v1/other-api/adapter). In some cases, `createRequestHandler` functions are used before we have a chance to instrument them. Auto-instrumentation wraps the core `createRequestHandler` from `@remix/server-runtime`, which all the adapters use. Each adapter may have their own APIs, so we can't directly re-export our already implemented wrapper. This PR implements and exports a manual wrapper for Express adapters. Also: Moves Remix-related type declarations to its own file, as it seems it will continue to grow.
1 parent ccc51f3 commit 1718e98

File tree

9 files changed

+366
-159
lines changed

9 files changed

+366
-159
lines changed

‎packages/remix/package.json‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -60,10 +60,10 @@
6060
"lint:eslint": "eslint . --cache --cache-location '../../eslintcache/' --format stylish",
6161
"lint:prettier": "prettier --check \"{src,test,scripts}/**/*.ts\"",
6262
"test": "run-s test:unit",
63-
"test:integration": "run-s test:integration:prepare test:integration:client test:integration:server",
64-
"test:integration:clean": "(cd test/integration && rimraf .cache build node_modules)",
65-
"test:integration:ci": "run-s test:integration:prepare test:integration:client:ci test:integration:server",
66-
"test:integration:prepare": "yarn test:integration:clean && (cd test/integration && yarn)",
63+
"test:integration": "run-s test:integration:clean test:integration:prepare test:integration:client test:integration:server",
64+
"test:integration:ci": "run-s test:integration:clean test:integration:prepare test:integration:client:ci test:integration:server",
65+
"test:integration:prepare": "(cd test/integration && yarn)",
66+
"test:integration:clean": "(cd test/integration && rimraf .cache node_modules build)",
6767
"test:integration:client": "yarn playwright install-deps && yarn playwright test test/integration/test/client/",
6868
"test:integration:client:ci": "yarn test:integration:client --browser='all' --reporter='line'",
6969
"test:integration:server": "jest --config=test/integration/jest.config.js test/integration/test/server/",

‎packages/remix/src/index.server.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ export { ErrorBoundary, withErrorBoundary } from '@sentry/react';
1010
export { remixRouterInstrumentation, withSentry } from './performance/client';
1111
export { BrowserTracing, Integrations } from '@sentry/tracing';
1212
export * from '@sentry/node';
13+
export { wrapExpressCreateRequestHandler } from './utils/serverAdapters/express';
1314

1415
function sdkAlreadyInitialized(): boolean {
1516
const hub = getCurrentHub();

‎packages/remix/src/utils/instrumentServer.ts‎

Lines changed: 105 additions & 143 deletions
Original file line numberDiff line numberDiff line change
@@ -1,103 +1,25 @@
11
/* eslint-disable max-lines */
22
import { captureException, getCurrentHub } from '@sentry/node';
3-
import { getActiveTransaction, hasTracingEnabled } from '@sentry/tracing';
3+
import { getActiveTransaction, hasTracingEnabled, Span } from '@sentry/tracing';
4+
import { WrappedFunction } from '@sentry/types';
45
import { addExceptionMechanism, fill, isNodeEnv, loadModule, logger, serializeBaggage } from '@sentry/utils';
56

6-
// Types vendored from @remix-run/[email protected]:
7-
// https://github.com/remix-run/remix/blob/f3691d51027b93caa3fd2cdfe146d7b62a6eb8f2/packages/remix-server-runtime/server.ts
8-
type AppLoadContext = unknown;
9-
type AppData = unknown;
10-
type RequestHandler = (request: Request, loadContext?: AppLoadContext) => Promise<Response>;
11-
type CreateRequestHandlerFunction = (build: ServerBuild, mode?: string) => RequestHandler;
12-
type ServerRouteManifest = RouteManifest<Omit<ServerRoute, 'children'>>;
13-
type Params<Key extends string = string> = {
14-
readonly [key in Key]: string | undefined;
15-
};
16-
17-
interface Route {
18-
index?: boolean;
19-
caseSensitive?: boolean;
20-
id: string;
21-
parentId?: string;
22-
path?: string;
23-
}
24-
interface RouteData {
25-
[routeId: string]: AppData;
26-
}
27-
28-
interface MetaFunction {
29-
(args: { data: AppData; parentsData: RouteData; params: Params; location: Location }): HtmlMetaDescriptor;
30-
}
31-
32-
interface HtmlMetaDescriptor {
33-
[name: string]: null | string | undefined | Record<string, string> | Array<Record<string, string> | string>;
34-
charset?: 'utf-8';
35-
charSet?: 'utf-8';
36-
title?: string;
37-
}
38-
39-
interface ServerRouteModule {
40-
action?: DataFunction;
41-
headers?: unknown;
42-
loader?: DataFunction;
43-
meta?: MetaFunction | HtmlMetaDescriptor;
44-
}
45-
46-
interface ServerRoute extends Route {
47-
children: ServerRoute[];
48-
module: ServerRouteModule;
49-
}
50-
51-
interface RouteManifest<Route> {
52-
[routeId: string]: Route;
53-
}
54-
55-
interface ServerBuild {
56-
entry: {
57-
module: ServerEntryModule;
58-
};
59-
routes: ServerRouteManifest;
60-
assets: unknown;
61-
}
62-
63-
interface HandleDocumentRequestFunction {
64-
(request: Request, responseStatusCode: number, responseHeaders: Headers, context: Record<symbol, unknown>):
65-
| Promise<Response>
66-
| Response;
67-
}
68-
69-
interface HandleDataRequestFunction {
70-
(response: Response, args: DataFunctionArgs): Promise<Response> | Response;
71-
}
72-
73-
interface ServerEntryModule {
74-
default: HandleDocumentRequestFunction;
75-
meta: MetaFunction;
76-
loader: DataFunction;
77-
handleDataRequest?: HandleDataRequestFunction;
78-
}
79-
80-
interface DataFunctionArgs {
81-
request: Request;
82-
context: AppLoadContext;
83-
params: Params;
84-
}
85-
86-
interface DataFunction {
87-
(args: DataFunctionArgs): Promise<Response> | Response | Promise<AppData> | AppData;
88-
}
89-
90-
interface ReactRouterDomPkg {
91-
matchRoutes: (routes: ServerRoute[], pathname: string) => RouteMatch<ServerRoute>[] | null;
92-
}
93-
94-
// Taken from Remix Implementation
95-
// https://github.com/remix-run/remix/blob/97999d02493e8114c39d48b76944069d58526e8d/packages/remix-server-runtime/routeMatching.ts#L6-L10
96-
export interface RouteMatch<Route> {
97-
params: Params;
98-
pathname: string;
99-
route: Route;
100-
}
7+
import {
8+
AppData,
9+
CreateRequestHandlerFunction,
10+
DataFunction,
11+
DataFunctionArgs,
12+
HandleDocumentRequestFunction,
13+
ReactRouterDomPkg,
14+
RequestHandler,
15+
RouteMatch,
16+
ServerBuild,
17+
ServerRoute,
18+
ServerRouteManifest,
19+
} from './types';
20+
21+
// Flag to track if the core request handler is instrumented.
22+
export let isRequestHandlerWrapped = false;
10123

10224
// Taken from Remix Implementation
10325
// 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 {
318240
};
319241
}
320242

321-
function createRoutes(manifest: ServerRouteManifest, parentId?: string): ServerRoute[] {
243+
/**
244+
* Creates routes from the server route manifest
245+
*
246+
* @param manifest
247+
* @param parentId
248+
*/
249+
export function createRoutes(manifest: ServerRouteManifest, parentId?: string): ServerRoute[] {
322250
return Object.entries(manifest)
323251
.filter(([, route]) => route.parentId === parentId)
324252
.map(([id, route]) => ({
@@ -352,33 +280,50 @@ function matchServerRoutes(
352280
}));
353281
}
354282

283+
/**
284+
* Starts a new transaction for the given request to be used by different `RequestHandler` wrappers.
285+
*
286+
* @param request
287+
* @param routes
288+
* @param pkg
289+
*/
290+
export function startRequestHandlerTransaction(
291+
url: URL,
292+
method: string,
293+
routes: ServerRoute[],
294+
pkg?: ReactRouterDomPkg,
295+
): Span | undefined {
296+
const hub = getCurrentHub();
297+
const currentScope = hub.getScope();
298+
const matches = matchServerRoutes(routes, url.pathname, pkg);
299+
300+
const match = matches && getRequestMatch(url, matches);
301+
const name = match === null ? url.pathname : match.route.id;
302+
const source = match === null ? 'url' : 'route';
303+
const transaction = hub.startTransaction({
304+
name,
305+
op: 'http.server',
306+
tags: {
307+
method: method,
308+
},
309+
metadata: {
310+
source,
311+
},
312+
});
313+
314+
if (transaction) {
315+
currentScope?.setSpan(transaction);
316+
}
317+
return transaction;
318+
}
319+
355320
function wrapRequestHandler(origRequestHandler: RequestHandler, build: ServerBuild): RequestHandler {
356321
const routes = createRoutes(build.routes);
357322
const pkg = loadModule<ReactRouterDomPkg>('react-router-dom');
358323
return async function (this: unknown, request: Request, loadContext?: unknown): Promise<Response> {
359-
const hub = getCurrentHub();
360-
const currentScope = hub.getScope();
361-
362324
const url = new URL(request.url);
363-
const matches = matchServerRoutes(routes, url.pathname, pkg);
364-
365-
const match = matches && getRequestMatch(url, matches);
366-
const name = match === null ? url.pathname : match.route.id;
367-
const source = match === null ? 'url' : 'route';
368-
const transaction = hub.startTransaction({
369-
name,
370-
op: 'http.server',
371-
tags: {
372-
method: request.method,
373-
},
374-
metadata: {
375-
source,
376-
},
377-
});
378325

379-
if (transaction) {
380-
currentScope?.setSpan(transaction);
381-
}
326+
const transaction = startRequestHandlerTransaction(url, request.method, routes, pkg);
382327

383328
const res = (await origRequestHandler.call(this, request, loadContext)) as Response;
384329

@@ -416,43 +361,60 @@ function getRequestMatch(url: URL, matches: RouteMatch<ServerRoute>[]): RouteMat
416361
return match;
417362
}
418363

419-
function makeWrappedCreateRequestHandler(
420-
origCreateRequestHandler: CreateRequestHandlerFunction,
421-
): CreateRequestHandlerFunction {
422-
return function (this: unknown, build: ServerBuild, mode: string | undefined): RequestHandler {
423-
const routes: ServerRouteManifest = {};
364+
/**
365+
* Instruments `remix` ServerBuild for performance tracing and error tracking.
366+
*/
367+
export function instrumentBuild(build: ServerBuild): ServerBuild {
368+
const routes: ServerRouteManifest = {};
424369

425-
const wrappedEntry = { ...build.entry, module: { ...build.entry.module } };
370+
const wrappedEntry = { ...build.entry, module: { ...build.entry.module } };
426371

372+
// Not keeping boolean flags like it's done for `requestHandler` functions,
373+
// Because the build can change between build and runtime.
374+
// So if there is a new `loader` or`action` or `documentRequest` after build.
375+
// We should be able to wrap them, as they may not be wrapped before.
376+
if (!(wrappedEntry.module.default as WrappedFunction).__sentry_original__) {
427377
fill(wrappedEntry.module, 'default', makeWrappedDocumentRequestFunction);
378+
}
428379

429-
for (const [id, route] of Object.entries(build.routes)) {
430-
const wrappedRoute = { ...route, module: { ...route.module } };
431-
432-
if (wrappedRoute.module.action) {
433-
fill(wrappedRoute.module, 'action', makeWrappedAction(id));
434-
}
380+
for (const [id, route] of Object.entries(build.routes)) {
381+
const wrappedRoute = { ...route, module: { ...route.module } };
435382

436-
if (wrappedRoute.module.loader) {
437-
fill(wrappedRoute.module, 'loader', makeWrappedLoader(id));
438-
}
383+
if (wrappedRoute.module.action && !(wrappedRoute.module.action as WrappedFunction).__sentry_original__) {
384+
fill(wrappedRoute.module, 'action', makeWrappedAction(id));
385+
}
439386

440-
// Entry module should have a loader function to provide `sentry-trace` and `baggage`
441-
// They will be available for the root `meta` function as `data.sentryTrace` and `data.sentryBaggage`
442-
if (!wrappedRoute.parentId) {
443-
if (!wrappedRoute.module.loader) {
444-
wrappedRoute.module.loader = () => ({});
445-
}
387+
if (wrappedRoute.module.loader && !(wrappedRoute.module.loader as WrappedFunction).__sentry_original__) {
388+
fill(wrappedRoute.module, 'loader', makeWrappedLoader(id));
389+
}
446390

447-
fill(wrappedRoute.module, 'loader', makeWrappedRootLoader);
391+
// Entry module should have a loader function to provide `sentry-trace` and `baggage`
392+
// They will be available for the root `meta` function as `data.sentryTrace` and `data.sentryBaggage`
393+
if (!wrappedRoute.parentId) {
394+
if (!wrappedRoute.module.loader) {
395+
wrappedRoute.module.loader = () => ({});
448396
}
449397

450-
routes[id] = wrappedRoute;
398+
// We want to wrap the root loader regardless of whether it's already wrapped before.
399+
fill(wrappedRoute.module, 'loader', makeWrappedRootLoader);
451400
}
452401

453-
const newBuild = { ...build, routes, entry: wrappedEntry };
402+
routes[id] = wrappedRoute;
403+
}
404+
405+
return { ...build, routes, entry: wrappedEntry };
406+
}
407+
408+
function makeWrappedCreateRequestHandler(
409+
origCreateRequestHandler: CreateRequestHandlerFunction,
410+
): CreateRequestHandlerFunction {
411+
// To track if this wrapper has been applied, before other wrappers.
412+
// Can't track `__sentry_original__` because it's not the same function as the potentially manually wrapped one.
413+
isRequestHandlerWrapped = true;
454414

455-
const requestHandler = origCreateRequestHandler.call(this, newBuild, mode);
415+
return function (this: unknown, build: ServerBuild, ...args: unknown[]): RequestHandler {
416+
const newBuild = instrumentBuild(build);
417+
const requestHandler = origCreateRequestHandler.call(this, newBuild, ...args);
456418

457419
return wrapRequestHandler(requestHandler, newBuild);
458420
};

‎packages/remix/src/utils/serverAdapters/express.ts‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
import { extractRequestData, loadModule } from '@sentry/utils';
2+
3+
import {
4+
createRoutes,
5+
instrumentBuild,
6+
isRequestHandlerWrapped,
7+
startRequestHandlerTransaction,
8+
} from '../instrumentServer';
9+
import {
10+
ExpressCreateRequestHandler,
11+
ExpressCreateRequestHandlerOptions,
12+
ExpressNextFunction,
13+
ExpressRequest,
14+
ExpressRequestHandler,
15+
ExpressResponse,
16+
ReactRouterDomPkg,
17+
ServerBuild,
18+
} from '../types';
19+
20+
function wrapExpressRequestHandler(
21+
origRequestHandler: ExpressRequestHandler,
22+
build: ServerBuild,
23+
): ExpressRequestHandler {
24+
const routes = createRoutes(build.routes);
25+
const pkg = loadModule<ReactRouterDomPkg>('react-router-dom');
26+
27+
// If the core request handler is already wrapped, don't wrap Express handler which uses it.
28+
if (isRequestHandlerWrapped) {
29+
return origRequestHandler;
30+
}
31+
32+
return async function (
33+
this: unknown,
34+
req: ExpressRequest,
35+
res: ExpressResponse,
36+
next: ExpressNextFunction,
37+
): Promise<void> {
38+
const request = extractRequestData(req);
39+
40+
if (!request.url || !request.method) {
41+
return origRequestHandler.call(this, req, res, next);
42+
}
43+
44+
const url = new URL(request.url);
45+
46+
const transaction = startRequestHandlerTransaction(url, request.method, routes, pkg);
47+
48+
await origRequestHandler.call(this, req, res, next);
49+
50+
transaction?.setHttpStatus(res.statusCode);
51+
transaction?.finish();
52+
};
53+
}
54+
55+
/**
56+
* Instruments `createRequestHandler` from `@remix-run/express`
57+
*/
58+
export function wrapExpressCreateRequestHandler(
59+
origCreateRequestHandler: ExpressCreateRequestHandler,
60+
): (options: any) => ExpressRequestHandler {
61+
return function (this: unknown, options: any): ExpressRequestHandler {
62+
const newBuild = instrumentBuild((options as ExpressCreateRequestHandlerOptions).build);
63+
const requestHandler = origCreateRequestHandler.call(this, { ...options, build: newBuild });
64+
65+
return wrapExpressRequestHandler(requestHandler, newBuild);
66+
};
67+
}

0 commit comments

Comments
 (0)