Skip to content

feat(remix): Add Remix server SDK #5269

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 4 commits into from
Jun 21, 2022
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
14 changes: 9 additions & 5 deletions packages/remix/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,10 @@
"engines": {
"node": ">=14"
},
"main": "build/esm/index.js",
"module": "build/esm/index.js",
"main": "build/esm/index.server.js",
"module": "build/esm/index.server.js",
"browser": "build/esm/index.client.js",
"types": "build/types/index.d.ts",
"types": "build/types/index.server.d.ts",
"private": true,
"dependencies": {
"@sentry/core": "7.1.1",
Expand Down Expand Up @@ -52,7 +52,7 @@
"build:rollup:watch": "rollup -c rollup.npm.config.js --watch",
"build:types:watch": "tsc -p tsconfig.types.json --watch",
"build:npm": "ts-node ../../scripts/prepack.ts && npm pack ./build",
"circularDepCheck": "madge --circular src/index.ts",
"circularDepCheck": "madge --circular src/index.server.ts",
"clean": "rimraf build coverage sentry-remix-*.tgz",
"fix": "run-s fix:eslint fix:prettier",
"fix:eslint": "eslint . --format stylish --fix",
Expand All @@ -66,5 +66,9 @@
},
"volta": {
"extends": "../../package.json"
}
},
"sideEffects": [
"./esm/index.server.js",
"./src/index.server.ts"
Copy link
Member

Choose a reason for hiding this comment

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

Why do these have side effects?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I took that from Next.JS SDK.

As I understand, we need to export browser / react stuff from index.server.ts to make TypeScript happy on compile time, but it's not guaranteed that it'll be used on runtime. (index.client.ts is used instead on the demo project).

So my guess is that flagging index.server.* as side effects is to prevent a pre-bundler that may fail TS compilation.

]
}
3 changes: 1 addition & 2 deletions packages/remix/rollup.npm.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import { makeBaseNPMConfig, makeNPMConfigVariants } from '../../rollup/index.js'

export default makeNPMConfigVariants(
makeBaseNPMConfig({
// Todo: Replace with -> ['src/index.server.ts', 'src/index.client.tsx'],
entrypoints: 'src/index.ts',
entrypoints: ['src/index.server.ts', 'src/index.client.tsx'],
}),
);
36 changes: 36 additions & 0 deletions packages/remix/src/index.server.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/* eslint-disable import/export */
import { configureScope, getCurrentHub, init as nodeInit } from '@sentry/node';
import { logger } from '@sentry/utils';

import { instrumentServer } from './utils/instrumentServer';
import { buildMetadata } from './utils/metadata';
import { RemixOptions } from './utils/remixOptions';

export { ErrorBoundary, withErrorBoundary } from '@sentry/react';
export { remixRouterInstrumentation, withSentryRouteTracing } from './performance/client';
export { BrowserTracing, Integrations } from '@sentry/tracing';
export * from '@sentry/node';

function sdkAlreadyInitialized(): boolean {
const hub = getCurrentHub();
return !!hub.getClient();
}

/** Initializes Sentry Remix SDK on Node. */
export function init(options: RemixOptions): void {
buildMetadata(options, ['remix', 'node']);

if (sdkAlreadyInitialized()) {
__DEBUG_BUILD__ && logger.log('SDK already initialized');

return;
}

instrumentServer();

nodeInit(options);

configureScope(scope => {
scope.setTag('runtime', 'node');
});
}
2 changes: 0 additions & 2 deletions packages/remix/src/index.ts

This file was deleted.

197 changes: 197 additions & 0 deletions packages/remix/src/utils/instrumentServer.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,197 @@
import { captureException, configureScope, getCurrentHub, startTransaction } from '@sentry/node';
import { getActiveTransaction } from '@sentry/tracing';
import { addExceptionMechanism, fill, loadModule, logger } 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<Response>;
type CreateRequestHandlerFunction = (build: ServerBuild, mode?: string) => RequestHandler;
Copy link
Member

Choose a reason for hiding this comment

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

Are these all vendored in? Can we link to the code + git sha where we did this?

type ServerRouteManifest = RouteManifest<Omit<ServerRoute, 'children'>>;
type Params<Key extends string = string> = {
readonly [key in Key]: string | undefined;
};

interface Route {
index?: boolean;
caseSensitive?: boolean;
id: string;
parentId?: string;
path?: string;
}

interface ServerRouteModule {
action?: DataFunction;
headers?: unknown;
loader?: DataFunction;
}

interface ServerRoute extends Route {
children: ServerRoute[];
module: ServerRouteModule;
}

interface RouteManifest<Route> {
[routeId: string]: Route;
}

interface ServerBuild {
entry: {
module: ServerEntryModule;
};
routes: ServerRouteManifest;
assets: unknown;
}

interface HandleDocumentRequestFunction {
(request: Request, responseStatusCode: number, responseHeaders: Headers, context: Record<symbol, unknown>):
| Promise<Response>
| Response;
}

interface HandleDataRequestFunction {
(response: Response, args: DataFunctionArgs): Promise<Response> | Response;
}

interface ServerEntryModule {
default: HandleDocumentRequestFunction;
handleDataRequest?: HandleDataRequestFunction;
}

interface DataFunctionArgs {
request: Request;
context: AppLoadContext;
params: Params;
}

interface DataFunction {
(args: DataFunctionArgs): Promise<Response> | Response | Promise<AppData> | AppData;
}

function makeWrappedDataFunction(origFn: DataFunction, name: 'action' | 'loader'): DataFunction {
return async function (this: unknown, args: DataFunctionArgs): Promise<Response | AppData> {
let res: Response | AppData;
const activeTransaction = getActiveTransaction();
const currentScope = getCurrentHub().getScope();

Copy link
Member

Choose a reason for hiding this comment

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

Instead of using optional chaining, let's just early return if there is no activeTransaction. Same if there is no currentScope

if (!activeTransaction || !currentScope) {
return origFn.call(this, args);
}

try {
const span = activeTransaction.startChild({
op: `remix.server.${name}`,
description: activeTransaction.name,
tags: {
name,
},
});

if (span) {
// Assign data function to hub to be able to see `db` transactions (if any) as children.
currentScope.setSpan(span);
}

res = await origFn.call(this, args);

currentScope.setSpan(activeTransaction);
span.finish();
} catch (err) {
configureScope(scope => {
scope.addEventProcessor(event => {
addExceptionMechanism(event, {
type: 'instrument',
handled: true,
data: {
function: name,
},
});

return event;
});
});

captureException(err);

// Rethrow for other handlers
throw err;
}

return res;
};
}

function makeWrappedAction(origAction: DataFunction): DataFunction {
return makeWrappedDataFunction(origAction, 'action');
}

function makeWrappedLoader(origAction: DataFunction): DataFunction {
return makeWrappedDataFunction(origAction, 'loader');
}

function wrapRequestHandler(origRequestHandler: RequestHandler): RequestHandler {
return async function (this: unknown, request: Request, loadContext?: unknown): Promise<Response> {
const currentScope = getCurrentHub().getScope();
const transaction = startTransaction({
name: request.url,
op: 'http.server',
tags: {
method: request.method,
},
});

if (transaction) {
currentScope?.setSpan(transaction);
}

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

transaction?.setHttpStatus(res.status);
transaction?.finish();

return res;
};
}

function makeWrappedCreateRequestHandler(
origCreateRequestHandler: CreateRequestHandlerFunction,
): CreateRequestHandlerFunction {
return function (this: unknown, build: ServerBuild, mode: string | undefined): RequestHandler {
const routes: ServerRouteManifest = {};

for (const [id, route] of Object.entries(build.routes)) {
const wrappedRoute = { ...route, module: { ...route.module } };

if (wrappedRoute.module.action) {
fill(wrappedRoute.module, 'action', makeWrappedAction);
}

if (wrappedRoute.module.loader) {
fill(wrappedRoute.module, 'loader', makeWrappedLoader);
}

routes[id] = wrappedRoute;
}

const requestHandler = origCreateRequestHandler.call(this, { ...build, routes }, mode);

return wrapRequestHandler(requestHandler);
};
}

/**
* Monkey-patch Remix's `createRequestHandler` from `@remix-run/server-runtime`
* which Remix Adapters (https://remix.run/docs/en/v1/api/remix) use underneath.
*/
export function instrumentServer(): void {
const pkg = loadModule<{ createRequestHandler: CreateRequestHandlerFunction }>('@remix-run/server-runtime');

if (!pkg) {
__DEBUG_BUILD__ && logger.warn('Remix SDK was unable to require `@remix-run/server-runtime` package.');

return;
}

fill(pkg, 'createRequestHandler', makeWrappedCreateRequestHandler);
}
3 changes: 2 additions & 1 deletion packages/remix/src/utils/remixOptions.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { NodeOptions } from '@sentry/node';
import { BrowserOptions } from '@sentry/react';
import { Options } from '@sentry/types';

export type RemixOptions = Options | BrowserOptions;
export type RemixOptions = Options | BrowserOptions | NodeOptions;
62 changes: 62 additions & 0 deletions packages/remix/test/index.server.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
import * as SentryNode from '@sentry/node';
import { getCurrentHub } from '@sentry/node';
import { getGlobalObject } from '@sentry/utils';

import { init } from '../src/index.server';

const global = getGlobalObject();

const nodeInit = jest.spyOn(SentryNode, 'init');

describe('Server init()', () => {
afterEach(() => {
jest.clearAllMocks();
global.__SENTRY__.hub = undefined;
});

it('inits the Node SDK', () => {
expect(nodeInit).toHaveBeenCalledTimes(0);
init({});
expect(nodeInit).toHaveBeenCalledTimes(1);
expect(nodeInit).toHaveBeenLastCalledWith(
expect.objectContaining({
_metadata: {
sdk: {
name: 'sentry.javascript.remix',
version: expect.any(String),
packages: [
{
name: 'npm:@sentry/remix',
version: expect.any(String),
},
{
name: 'npm:@sentry/node',
version: expect.any(String),
},
],
},
},
}),
);
});

it("doesn't reinitialize the node SDK if already initialized", () => {
expect(nodeInit).toHaveBeenCalledTimes(0);
init({});
expect(nodeInit).toHaveBeenCalledTimes(1);
init({});
expect(nodeInit).toHaveBeenCalledTimes(1);
});

it('sets runtime on scope', () => {
const currentScope = getCurrentHub().getScope();

// @ts-ignore need access to protected _tags attribute
expect(currentScope._tags).toEqual({});

init({});

// @ts-ignore need access to protected _tags attribute
expect(currentScope._tags).toEqual({ runtime: 'node' });
});
});