From 7ab448fe19ff9faaea5131c4b6af1e89130d4adf Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Thu, 16 Jun 2022 17:05:48 +0100 Subject: [PATCH 1/4] feat(remix): Add Remix server SDK --- packages/remix/package.json | 14 +- packages/remix/rollup.npm.config.js | 3 +- packages/remix/src/flags.ts | 18 ++ packages/remix/src/index.server.ts | 34 ++++ packages/remix/src/index.ts | 2 - packages/remix/src/utils/instrumentServer.ts | 195 +++++++++++++++++++ packages/remix/src/utils/remixOptions.ts | 3 +- packages/remix/test/index.server.test.ts | 62 ++++++ 8 files changed, 321 insertions(+), 10 deletions(-) create mode 100644 packages/remix/src/flags.ts create mode 100644 packages/remix/src/index.server.ts delete mode 100644 packages/remix/src/index.ts create mode 100644 packages/remix/src/utils/instrumentServer.ts create mode 100644 packages/remix/test/index.server.test.ts diff --git a/packages/remix/package.json b/packages/remix/package.json index 43d9ca5f9918..b1fc3c461e2d 100644 --- a/packages/remix/package.json +++ b/packages/remix/package.json @@ -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", @@ -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", @@ -66,5 +66,9 @@ }, "volta": { "extends": "../../package.json" - } + }, + "sideEffects": [ + "./esm/index.server.js", + "./src/index.server.ts" + ] } diff --git a/packages/remix/rollup.npm.config.js b/packages/remix/rollup.npm.config.js index af604639855a..4689937a652c 100644 --- a/packages/remix/rollup.npm.config.js +++ b/packages/remix/rollup.npm.config.js @@ -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'], }), ); diff --git a/packages/remix/src/flags.ts b/packages/remix/src/flags.ts new file mode 100644 index 000000000000..fb99adbc2aa7 --- /dev/null +++ b/packages/remix/src/flags.ts @@ -0,0 +1,18 @@ +/* + * This file defines flags and constants that can be modified during compile time in order to facilitate tree shaking + * for users. + * + * Debug flags need to be declared in each package individually and must not be imported across package boundaries, + * because some build tools have trouble tree-shaking imported guards. + * + * As a convention, we define debug flags in a `flags.ts` file in the root of a package's `src` folder. + * + * Debug flag files will contain "magic strings" like `__SENTRY_DEBUG__` that may get replaced with actual values during + * our, or the user's build process. Take care when introducing new flags - they must not throw if they are not + * replaced. + */ + +declare const __SENTRY_DEBUG__: boolean; + +/** Flag that is true for debug builds, false otherwise. */ +export const IS_DEBUG_BUILD = typeof __SENTRY_DEBUG__ === 'undefined' ? true : __SENTRY_DEBUG__; diff --git a/packages/remix/src/index.server.ts b/packages/remix/src/index.server.ts new file mode 100644 index 000000000000..ed4b28038381 --- /dev/null +++ b/packages/remix/src/index.server.ts @@ -0,0 +1,34 @@ +/* eslint-disable import/export */ +import { configureScope, getCurrentHub, init as nodeInit } from '@sentry/node'; + +import { instrumentServer } from './utils/instrumentServer'; +import { buildMetadata } from './utils/metadata'; +import { RemixOptions } from './utils/remixOptions'; + +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()) { + // TODO: Log something + return; + } + + instrumentServer(); + + nodeInit(options); + + configureScope(scope => { + scope.setTag('runtime', 'node'); + }); +} + +export { ErrorBoundary, withErrorBoundary } from '@sentry/react'; +export { remixRouterInstrumentation, withSentryRouteTracing } from './performance/client'; +export { BrowserTracing, Integrations } from '@sentry/tracing'; +export * from '@sentry/node'; diff --git a/packages/remix/src/index.ts b/packages/remix/src/index.ts deleted file mode 100644 index f56038ae9853..000000000000 --- a/packages/remix/src/index.ts +++ /dev/null @@ -1,2 +0,0 @@ -export { remixRouterInstrumentation, withSentryRouteTracing } from './performance/client'; -export { BrowserTracing, Integrations } from '@sentry/tracing'; diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts new file mode 100644 index 000000000000..cbfc5892b185 --- /dev/null +++ b/packages/remix/src/utils/instrumentServer.ts @@ -0,0 +1,195 @@ +import { captureException, configureScope, getCurrentHub, startTransaction } from '@sentry/node'; +import { getActiveTransaction, hasTracingEnabled } from '@sentry/tracing'; +import { addExceptionMechanism, fill, loadModule, logger } from '@sentry/utils'; + +import { IS_DEBUG_BUILD } from '../flags'; + +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 ServerRouteModule { + action?: DataFunction; + headers?: unknown; + loader?: DataFunction; +} + +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; + handleDataRequest?: HandleDataRequestFunction; +} + +interface DataFunctionArgs { + request: Request; + context: AppLoadContext; + params: Params; +} + +interface DataFunction { + (args: DataFunctionArgs): Promise | Response | Promise | AppData; +} + +function makeWrappedDataFunction(origFn: DataFunction, name: 'action' | 'loader'): DataFunction { + return async function (this: unknown, args: DataFunctionArgs): Promise { + let res: Response | AppData; + const activeTransaction = getActiveTransaction(); + const currentScope = getCurrentHub().getScope(); + + 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); + 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 { + const currentScope = getCurrentHub().getScope(); + + // debugger; + const transaction = hasTracingEnabled() + ? startTransaction({ + name: request.url, + op: 'remix.server.requesthandler', + tags: { + method: request.method, + }, + }) + : undefined; + + 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) { + IS_DEBUG_BUILD && logger.warn('Remix SDK was unable to require `@remix-run/server-runtime` package.'); + + return; + } + + fill(pkg, 'createRequestHandler', makeWrappedCreateRequestHandler); +} diff --git a/packages/remix/src/utils/remixOptions.ts b/packages/remix/src/utils/remixOptions.ts index 74d67074b0a3..9534ed57de3b 100644 --- a/packages/remix/src/utils/remixOptions.ts +++ b/packages/remix/src/utils/remixOptions.ts @@ -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; diff --git a/packages/remix/test/index.server.test.ts b/packages/remix/test/index.server.test.ts new file mode 100644 index 000000000000..2bfe118fcc5f --- /dev/null +++ b/packages/remix/test/index.server.test.ts @@ -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' }); + }); +}); From 02149704881455206bc6d513b7fa86ace0956028 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Tue, 21 Jun 2022 15:44:42 +0100 Subject: [PATCH 2/4] Address review comments. --- packages/remix/src/flags.ts | 18 ---------- packages/remix/src/index.server.ts | 14 ++++---- packages/remix/src/utils/instrumentServer.ts | 36 +++++++++++--------- 3 files changed, 27 insertions(+), 41 deletions(-) delete mode 100644 packages/remix/src/flags.ts diff --git a/packages/remix/src/flags.ts b/packages/remix/src/flags.ts deleted file mode 100644 index fb99adbc2aa7..000000000000 --- a/packages/remix/src/flags.ts +++ /dev/null @@ -1,18 +0,0 @@ -/* - * This file defines flags and constants that can be modified during compile time in order to facilitate tree shaking - * for users. - * - * Debug flags need to be declared in each package individually and must not be imported across package boundaries, - * because some build tools have trouble tree-shaking imported guards. - * - * As a convention, we define debug flags in a `flags.ts` file in the root of a package's `src` folder. - * - * Debug flag files will contain "magic strings" like `__SENTRY_DEBUG__` that may get replaced with actual values during - * our, or the user's build process. Take care when introducing new flags - they must not throw if they are not - * replaced. - */ - -declare const __SENTRY_DEBUG__: boolean; - -/** Flag that is true for debug builds, false otherwise. */ -export const IS_DEBUG_BUILD = typeof __SENTRY_DEBUG__ === 'undefined' ? true : __SENTRY_DEBUG__; diff --git a/packages/remix/src/index.server.ts b/packages/remix/src/index.server.ts index ed4b28038381..d57b5942fbf8 100644 --- a/packages/remix/src/index.server.ts +++ b/packages/remix/src/index.server.ts @@ -1,10 +1,16 @@ /* 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(); @@ -15,7 +21,8 @@ export function init(options: RemixOptions): void { buildMetadata(options, ['remix', 'node']); if (sdkAlreadyInitialized()) { - // TODO: Log something + __DEBUG_BUILD__ && logger.log('SDK already initialized'); + return; } @@ -27,8 +34,3 @@ export function init(options: RemixOptions): void { scope.setTag('runtime', 'node'); }); } - -export { ErrorBoundary, withErrorBoundary } from '@sentry/react'; -export { remixRouterInstrumentation, withSentryRouteTracing } from './performance/client'; -export { BrowserTracing, Integrations } from '@sentry/tracing'; -export * from '@sentry/node'; diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index cbfc5892b185..2e318cf550df 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -1,9 +1,9 @@ import { captureException, configureScope, getCurrentHub, startTransaction } from '@sentry/node'; -import { getActiveTransaction, hasTracingEnabled } from '@sentry/tracing'; +import { getActiveTransaction } from '@sentry/tracing'; import { addExceptionMechanism, fill, loadModule, logger } from '@sentry/utils'; -import { IS_DEBUG_BUILD } from '../flags'; - +// 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; @@ -75,8 +75,12 @@ function makeWrappedDataFunction(origFn: DataFunction, name: 'action' | 'loader' const activeTransaction = getActiveTransaction(); const currentScope = getCurrentHub().getScope(); + if (!activeTransaction || !currentScope) { + return origFn.call(this, args); + } + try { - const span = activeTransaction?.startChild({ + const span = activeTransaction.startChild({ op: `remix.server.${name}`, description: activeTransaction.name, tags: { @@ -86,10 +90,12 @@ function makeWrappedDataFunction(origFn: DataFunction, name: 'action' | 'loader' if (span) { // Assign data function to hub to be able to see `db` transactions (if any) as children. - currentScope?.setSpan(span); + currentScope.setSpan(span); } res = await origFn.call(this, args); + + currentScope.setSpan(activeTransaction); span?.finish(); } catch (err) { configureScope(scope => { @@ -127,17 +133,13 @@ function makeWrappedLoader(origAction: DataFunction): DataFunction { function wrapRequestHandler(origRequestHandler: RequestHandler): RequestHandler { return async function (this: unknown, request: Request, loadContext?: unknown): Promise { const currentScope = getCurrentHub().getScope(); - - // debugger; - const transaction = hasTracingEnabled() - ? startTransaction({ - name: request.url, - op: 'remix.server.requesthandler', - tags: { - method: request.method, - }, - }) - : undefined; + const transaction = startTransaction({ + name: request.url, + op: 'remix.server.requesthandler', + tags: { + method: request.method, + }, + }); if (transaction) { currentScope?.setSpan(transaction); @@ -186,7 +188,7 @@ export function instrumentServer(): void { const pkg = loadModule<{ createRequestHandler: CreateRequestHandlerFunction }>('@remix-run/server-runtime'); if (!pkg) { - IS_DEBUG_BUILD && logger.warn('Remix SDK was unable to require `@remix-run/server-runtime` package.'); + __DEBUG_BUILD__ && logger.warn('Remix SDK was unable to require `@remix-run/server-runtime` package.'); return; } From 355b3909c9bcb559a78013df18ac90f5688a4f6c Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Tue, 21 Jun 2022 16:38:32 +0100 Subject: [PATCH 3/4] Remove unneeded optional chain. --- packages/remix/src/utils/instrumentServer.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index 2e318cf550df..395852eb6343 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -96,7 +96,7 @@ function makeWrappedDataFunction(origFn: DataFunction, name: 'action' | 'loader' res = await origFn.call(this, args); currentScope.setSpan(activeTransaction); - span?.finish(); + span.finish(); } catch (err) { configureScope(scope => { scope.addEventProcessor(event => { From 5b8bfee02f86ce1fedbbe94a575d00fc29587485 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Tue, 21 Jun 2022 16:38:48 +0100 Subject: [PATCH 4/4] Update packages/remix/src/utils/instrumentServer.ts Co-authored-by: Abhijeet Prasad --- packages/remix/src/utils/instrumentServer.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index 395852eb6343..486ab06b149d 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -135,7 +135,7 @@ function wrapRequestHandler(origRequestHandler: RequestHandler): RequestHandler const currentScope = getCurrentHub().getScope(); const transaction = startTransaction({ name: request.url, - op: 'remix.server.requesthandler', + op: 'http.server', tags: { method: request.method, },