From a173582e159585ea81196e424759f6006ac747eb Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Fri, 17 May 2024 15:43:05 +0100 Subject: [PATCH 01/27] feat(remix): Migrate to `opentelemetry-instrumentation-remix`. --- .../README.md | 36 -- .../app/entry.server.tsx | 7 - .../instrument.server.mjs | 10 + .../package.json | 2 +- .../server.mjs | 5 +- .../app/entry.server.tsx | 10 +- .../app/routes/_index.tsx | 4 +- .../create-remix-app-express/instrument.cjs | 14 + .../create-remix-app-express/package.json | 2 +- .../create-remix-app-express/server.mjs | 5 +- .../tests/behaviour-server.test.ts | 40 ++- .../create-remix-app-v2/instrument.server.cjs | 8 + .../create-remix-app-v2/playwright.config.ts | 66 ++++ .../tests/behaviour-server.test.ts | 9 +- .../create-remix-app/app/entry.server.tsx | 10 +- .../create-remix-app/instrument.server.cjs | 8 + .../create-remix-app/playwright.config.ts | 66 ++++ .../tests/behaviour-server.test.ts | 9 +- packages/remix/jest.config.js | 8 - packages/remix/package.json | 7 +- packages/remix/playwright.config.ts | 3 + packages/remix/src/index.server.ts | 29 +- packages/remix/src/utils/instrumentServer.ts | 318 +++++------------- .../src/utils/integrations/opentelemetry.ts | 24 ++ packages/remix/src/utils/remixOptions.ts | 4 +- .../remix/src/utils/serverAdapters/express.ts | 226 ------------- .../test/integration/app_v1/entry.server.tsx | 11 - .../test/integration/app_v2/entry.server.tsx | 9 - .../test/integration/instrument.server.cjs | 9 + .../remix/test/integration/jest.config.js | 8 - packages/remix/test/integration/package.json | 4 +- .../integration/test/client/meta-tags.test.ts | 8 +- .../integration/test/server/action.test.ts | 119 ++++--- .../integration/test/server/loader.test.ts | 106 +++--- .../test/integration/test/server/ssr.test.ts | 4 +- .../integration/test/server/utils/helpers.ts | 8 +- .../remix/test/integration/tsconfig.test.json | 2 +- packages/remix/vitest.config.ts | 8 + yarn.lock | 21 +- 39 files changed, 518 insertions(+), 729 deletions(-) delete mode 100644 dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/README.md create mode 100644 dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/instrument.server.mjs create mode 100644 dev-packages/e2e-tests/test-applications/create-remix-app-express/instrument.cjs create mode 100644 dev-packages/e2e-tests/test-applications/create-remix-app-v2/instrument.server.cjs create mode 100644 dev-packages/e2e-tests/test-applications/create-remix-app-v2/playwright.config.ts create mode 100644 dev-packages/e2e-tests/test-applications/create-remix-app/instrument.server.cjs create mode 100644 dev-packages/e2e-tests/test-applications/create-remix-app/playwright.config.ts delete mode 100644 packages/remix/jest.config.js create mode 100644 packages/remix/src/utils/integrations/opentelemetry.ts delete mode 100644 packages/remix/src/utils/serverAdapters/express.ts create mode 100644 packages/remix/test/integration/instrument.server.cjs delete mode 100644 packages/remix/test/integration/jest.config.js create mode 100644 packages/remix/vitest.config.ts diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/README.md b/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/README.md deleted file mode 100644 index 31400e85106f..000000000000 --- a/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/README.md +++ /dev/null @@ -1,36 +0,0 @@ -# Welcome to Remix + Vite! - -📖 See the [Remix docs](https://remix.run/docs) and the [Remix Vite docs](https://remix.run/docs/en/main/future/vite) -for details on supported features. - -## Development - -Run the Express server with Vite dev middleware: - -```shellscript -npm run dev -``` - -## Deployment - -First, build your app for production: - -```sh -npm run build -``` - -Then run the app in production mode: - -```sh -npm start -``` - -Now you'll need to pick a host to deploy it to. - -### DIY - -If you're familiar with deploying Express applications you should be right at home. Just make sure to deploy the output -of `npm run build` - -- `build/server` -- `build/client` diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/app/entry.server.tsx b/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/app/entry.server.tsx index 5e0608ff5749..a38636a8745d 100644 --- a/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/app/entry.server.tsx +++ b/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/app/entry.server.tsx @@ -1,12 +1,5 @@ import * as Sentry from '@sentry/remix'; -Sentry.init({ - tracesSampleRate: 1.0, // Capture 100% of the transactions, reduce in production! - environment: 'qa', // dynamic sampling bias to keep transactions - dsn: process.env.E2E_TEST_DSN, - tunnel: 'http://localhost:3031/', // proxy server -}); - import { PassThrough } from 'node:stream'; import type { AppLoadContext, EntryContext } from '@remix-run/node'; diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/instrument.server.mjs b/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/instrument.server.mjs new file mode 100644 index 000000000000..2c3d63a8b2e6 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/instrument.server.mjs @@ -0,0 +1,10 @@ +import * as Sentry from '@sentry/remix'; +import * as process from 'process'; + +Sentry.init({ + tracesSampleRate: 1.0, // Capture 100% of the transactions, reduce in production! + environment: 'qa', // dynamic sampling bias to keep transactions + dsn: process.env.E2E_TEST_DSN, + tunnel: 'http://localhost:3031/', // proxy server + debug: true, +}); diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/package.json b/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/package.json index 5ba4b54d8e8f..f939dd15ca8c 100644 --- a/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/package.json +++ b/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/package.json @@ -3,7 +3,7 @@ "sideEffects": false, "scripts": { "build": "remix vite:build", - "dev": "node ./server.mjs", + "dev": "NODE_OPTIONS='--import=./instrument.server.mjs' node ./server.mjs", "lint": "eslint --ignore-path .gitignore --cache --cache-location ./node_modules/.cache/eslint .", "start": "cross-env NODE_ENV=production node ./server.mjs", "typecheck": "tsc", diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/server.mjs b/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/server.mjs index f432b2d49184..dcd0ef1c5cbc 100644 --- a/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/server.mjs +++ b/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/server.mjs @@ -1,14 +1,11 @@ import { createRequestHandler } from '@remix-run/express'; import { installGlobals } from '@remix-run/node'; -import { wrapExpressCreateRequestHandler } from '@sentry/remix'; import compression from 'compression'; import express from 'express'; import morgan from 'morgan'; installGlobals(); -const sentryCreateRequestHandler = wrapExpressCreateRequestHandler(createRequestHandler); - const viteDevServer = process.env.NODE_ENV === 'production' ? undefined @@ -42,7 +39,7 @@ app.use(morgan('tiny')); // handle SSR requests app.all( '*', - sentryCreateRequestHandler({ + createRequestHandler({ build: viteDevServer ? () => viteDevServer.ssrLoadModule('virtual:remix/server-build') : await import('./build/server/index.js'), diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-express/app/entry.server.tsx b/dev-packages/e2e-tests/test-applications/create-remix-app-express/app/entry.server.tsx index 9171a6e46cb7..4e1e9e0ba537 100644 --- a/dev-packages/e2e-tests/test-applications/create-remix-app-express/app/entry.server.tsx +++ b/dev-packages/e2e-tests/test-applications/create-remix-app-express/app/entry.server.tsx @@ -1,15 +1,7 @@ import * as Sentry from '@sentry/remix'; -import * as isbotModule from 'isbot'; - -Sentry.init({ - tracesSampleRate: 1.0, // Capture 100% of the transactions, reduce in production! - environment: 'qa', // dynamic sampling bias to keep transactions - dsn: process.env.E2E_TEST_DSN, - tunnel: 'http://localhost:3031/', // proxy server - sendDefaultPii: true, // Testing the FormData -}); import { PassThrough } from 'node:stream'; +import * as isbotModule from 'isbot'; import type { AppLoadContext, EntryContext } from '@remix-run/node'; import { createReadableStreamFromReadable } from '@remix-run/node'; diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-express/app/routes/_index.tsx b/dev-packages/e2e-tests/test-applications/create-remix-app-express/app/routes/_index.tsx index b646c62ee4da..8c787ebd7c2f 100644 --- a/dev-packages/e2e-tests/test-applications/create-remix-app-express/app/routes/_index.tsx +++ b/dev-packages/e2e-tests/test-applications/create-remix-app-express/app/routes/_index.tsx @@ -5,7 +5,9 @@ export default function Index() { const [searchParams] = useSearchParams(); if (searchParams.get('tag')) { - Sentry.setTag('sentry_test', searchParams.get('tag')); + Sentry.setTags({ + sentry_test: searchParams.get('tag'), + }); } return ( diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-express/instrument.cjs b/dev-packages/e2e-tests/test-applications/create-remix-app-express/instrument.cjs new file mode 100644 index 000000000000..feea6fb5388e --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/create-remix-app-express/instrument.cjs @@ -0,0 +1,14 @@ +const Sentry = require('@sentry/remix'); +const process = require('process'); + +Sentry.init({ + tracesSampleRate: 1.0, // Capture 100% of the transactions, reduce in production! + environment: 'qa', // dynamic sampling bias to keep transactions + dsn: process.env.E2E_TEST_DSN, + tunnel: 'http://localhost:3031/', // proxy server + sendDefaultPii: true, // Testing the FormData + captureActionFormDataKeys: { + file: true, + text: true, + }, +}); diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-express/package.json b/dev-packages/e2e-tests/test-applications/create-remix-app-express/package.json index 53ccbdff9e7f..cce75b6d592e 100644 --- a/dev-packages/e2e-tests/test-applications/create-remix-app-express/package.json +++ b/dev-packages/e2e-tests/test-applications/create-remix-app-express/package.json @@ -6,7 +6,7 @@ "build": "remix vite:build", "dev": "node ./server.mjs", "lint": "eslint --ignore-path .gitignore --cache --cache-location ./node_modules/.cache/eslint .", - "start": "cross-env NODE_ENV=production node ./server.mjs", + "start": "cross-env NODE_ENV=production node --require='./instrument.cjs' ./server.mjs", "typecheck": "tsc", "clean": "npx rimraf node_modules pnpm-lock.yaml", "test:build": "pnpm install && npx playwright install && pnpm build", diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-express/server.mjs b/dev-packages/e2e-tests/test-applications/create-remix-app-express/server.mjs index f432b2d49184..dcd0ef1c5cbc 100644 --- a/dev-packages/e2e-tests/test-applications/create-remix-app-express/server.mjs +++ b/dev-packages/e2e-tests/test-applications/create-remix-app-express/server.mjs @@ -1,14 +1,11 @@ import { createRequestHandler } from '@remix-run/express'; import { installGlobals } from '@remix-run/node'; -import { wrapExpressCreateRequestHandler } from '@sentry/remix'; import compression from 'compression'; import express from 'express'; import morgan from 'morgan'; installGlobals(); -const sentryCreateRequestHandler = wrapExpressCreateRequestHandler(createRequestHandler); - const viteDevServer = process.env.NODE_ENV === 'production' ? undefined @@ -42,7 +39,7 @@ app.use(morgan('tiny')); // handle SSR requests app.all( '*', - sentryCreateRequestHandler({ + createRequestHandler({ build: viteDevServer ? () => viteDevServer.ssrLoadModule('virtual:remix/server-build') : await import('./build/server/index.js'), diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-express/tests/behaviour-server.test.ts b/dev-packages/e2e-tests/test-applications/create-remix-app-express/tests/behaviour-server.test.ts index 18825fb95e7b..7f41f6ea8880 100644 --- a/dev-packages/e2e-tests/test-applications/create-remix-app-express/tests/behaviour-server.test.ts +++ b/dev-packages/e2e-tests/test-applications/create-remix-app-express/tests/behaviour-server.test.ts @@ -27,19 +27,36 @@ test('Sends form data with action error to Sentry', async ({ page }) => { await page.locator('button[type=submit]').click(); const formdataActionTransaction = waitForTransaction('create-remix-app-express', transactionEvent => { - return transactionEvent?.spans?.some(span => span.op === 'function.remix.action'); + return transactionEvent?.spans?.some(span => span.data && span.data['code.function'] === 'action'); }); - const actionTransaction = await formdataActionTransaction; + const actionSpan = (await formdataActionTransaction).spans.find( + span => span.data && span.data['code.function'] === 'action', + ); - expect(actionTransaction).toBeDefined(); - expect(actionTransaction.contexts.trace.op).toBe('http.server'); - expect(actionTransaction.spans[0].data).toMatchObject({ - 'remix.action_form_data.text': 'test', - 'remix.action_form_data.file': 'file.txt', + expect(actionSpan).toBeDefined(); + expect(actionSpan.op).toBe('http'); + expect(actionSpan.data).toMatchObject({ + 'formData.text': 'test', + 'formData.file': 'file.txt', }); }); +test('Sends a loader span to Sentry', async ({ page }) => { + const loaderTransactionPromise = waitForTransaction('create-remix-app-express', transactionEvent => { + return transactionEvent?.spans?.some(span => span.data && span.data['code.function'] === 'loader'); + }); + + await page.goto('/'); + + const loaderSpan = (await loaderTransactionPromise).spans.find( + span => span.data && span.data['code.function'] === 'loader', + ); + + expect(loaderSpan).toBeDefined(); + expect(loaderSpan.op).toBe('http'); +}); + test('Sends two linked transactions (server & client) to Sentry', async ({ page }) => { // We use this to identify the transactions const testTag = uuid4(); @@ -47,7 +64,7 @@ test('Sends two linked transactions (server & client) to Sentry', async ({ page const httpServerTransactionPromise = waitForTransaction('create-remix-app-express', transactionEvent => { return ( transactionEvent.type === 'transaction' && - transactionEvent.contexts?.trace?.op === 'http.server' && + transactionEvent.contexts?.trace?.op === 'http' && transactionEvent.tags?.['sentry_test'] === testTag ); }); @@ -70,18 +87,21 @@ test('Sends two linked transactions (server & client) to Sentry', async ({ page const httpServerTraceId = httpServerTransaction.contexts?.trace?.trace_id; const httpServerSpanId = httpServerTransaction.contexts?.trace?.span_id; + const loaderSpanId = httpServerTransaction.spans.find( + span => span.data && span.data['code.function'] === 'loader', + )?.span_id; const pageLoadTraceId = pageloadTransaction.contexts?.trace?.trace_id; const pageLoadSpanId = pageloadTransaction.contexts?.trace?.span_id; const pageLoadParentSpanId = pageloadTransaction.contexts?.trace?.parent_span_id; - expect(httpServerTransaction.transaction).toBe('routes/_index'); + expect(httpServerTransaction.transaction).toBe('GET http://localhost:3030/'); expect(pageloadTransaction.transaction).toBe('routes/_index'); expect(httpServerTraceId).toBeDefined(); expect(httpServerSpanId).toBeDefined(); expect(pageLoadTraceId).toEqual(httpServerTraceId); - expect(pageLoadParentSpanId).toEqual(httpServerSpanId); + expect(pageLoadParentSpanId).toEqual(loaderSpanId); expect(pageLoadSpanId).not.toEqual(httpServerSpanId); }); diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-v2/instrument.server.cjs b/dev-packages/e2e-tests/test-applications/create-remix-app-v2/instrument.server.cjs new file mode 100644 index 000000000000..6d211cac4592 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/create-remix-app-v2/instrument.server.cjs @@ -0,0 +1,8 @@ +const Sentry = require('@sentry/remix'); + +Sentry.init({ + tracesSampleRate: 1.0, // Capture 100% of the transactions, reduce in production! + environment: 'qa', // dynamic sampling bias to keep transactions + dsn: process.env.E2E_TEST_DSN, + tunnel: 'http://localhost:3031/', // proxy server +}); diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-v2/playwright.config.ts b/dev-packages/e2e-tests/test-applications/create-remix-app-v2/playwright.config.ts new file mode 100644 index 000000000000..e9324bddb481 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/create-remix-app-v2/playwright.config.ts @@ -0,0 +1,66 @@ +import type { PlaywrightTestConfig } from '@playwright/test'; +import { devices } from '@playwright/test'; + +const port = 3030; +const eventProxyPort = 3031; + +/** + * See https://playwright.dev/docs/test-configuration. + */ +const config: PlaywrightTestConfig = { + testDir: './tests', + /* Maximum time one test can run for. */ + timeout: 150_000, + expect: { + /** + * Maximum time expect() should wait for the condition to be met. + * For example in `await expect(locator).toHaveText();` + */ + timeout: 5000, + }, + /* Run tests in files in parallel */ + fullyParallel: true, + /* Fail the build on CI if you accidentally left test.only in the source code. */ + forbidOnly: !!process.env.CI, + /* Retry on CI only */ + retries: 0, + /* Opt out of parallel tests on CI. */ + workers: 1, + /* Reporter to use. See https://playwright.dev/docs/test-reporters */ + reporter: 'list', + /* Shared settings for all the projects below. See https://playwright.dev/docs/api/class-testoptions. */ + use: { + /* Maximum time each action such as `click()` can take. Defaults to 0 (no limit). */ + actionTimeout: 0, + + /* Collect trace when retrying the failed test. See https://playwright.dev/docs/trace-viewer */ + trace: 'on-first-retry', + + /* Base URL to use in actions like `await page.goto('/')`. */ + baseURL: `http://localhost:${port}`, + }, + + /* Configure projects for major browsers */ + projects: [ + { + name: 'chromium', + use: { + ...devices['Desktop Chrome'], + }, + }, + ], + + /* Run your local dev server before starting the tests */ + webServer: [ + { + command: 'node start-event-proxy.mjs', + port: eventProxyPort, + }, + { + command: `PORT=${port} NODE_OPTIONS='--require ./instrument.server.cjs' pnpm start`, + port: port, + }, + ], +}; + +export default config; diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-v2/tests/behaviour-server.test.ts b/dev-packages/e2e-tests/test-applications/create-remix-app-v2/tests/behaviour-server.test.ts index 9387cec33752..42219f9dd8d2 100644 --- a/dev-packages/e2e-tests/test-applications/create-remix-app-v2/tests/behaviour-server.test.ts +++ b/dev-packages/e2e-tests/test-applications/create-remix-app-v2/tests/behaviour-server.test.ts @@ -10,7 +10,7 @@ test('Sends two linked transactions (server & client) to Sentry', async ({ page const httpServerTransactionPromise = waitForTransaction('create-remix-app-v2', transactionEvent => { return ( transactionEvent.type === 'transaction' && - transactionEvent.contexts?.trace?.op === 'http.server' && + transactionEvent.contexts?.trace?.op === 'http' && transactionEvent.tags?.['sentry_test'] === testTag ); }); @@ -33,18 +33,21 @@ test('Sends two linked transactions (server & client) to Sentry', async ({ page const httpServerTraceId = httpServerTransaction.contexts?.trace?.trace_id; const httpServerSpanId = httpServerTransaction.contexts?.trace?.span_id; + const loaderSpanId = httpServerTransaction?.spans?.find( + span => span.data && span.data['code.function'] === 'loader', + )?.span_id; const pageLoadTraceId = pageloadTransaction.contexts?.trace?.trace_id; const pageLoadSpanId = pageloadTransaction.contexts?.trace?.span_id; const pageLoadParentSpanId = pageloadTransaction.contexts?.trace?.parent_span_id; - expect(httpServerTransaction.transaction).toBe('routes/_index'); + expect(httpServerTransaction.transaction).toBe('GET http://localhost:3030/'); expect(pageloadTransaction.transaction).toBe('routes/_index'); expect(httpServerTraceId).toBeDefined(); expect(httpServerSpanId).toBeDefined(); expect(pageLoadTraceId).toEqual(httpServerTraceId); - expect(pageLoadParentSpanId).toEqual(httpServerSpanId); + expect(pageLoadParentSpanId).toEqual(loaderSpanId); expect(pageLoadSpanId).not.toEqual(httpServerSpanId); }); diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app/app/entry.server.tsx b/dev-packages/e2e-tests/test-applications/create-remix-app/app/entry.server.tsx index b0f1c5d19f09..ee0aaa79b814 100644 --- a/dev-packages/e2e-tests/test-applications/create-remix-app/app/entry.server.tsx +++ b/dev-packages/e2e-tests/test-applications/create-remix-app/app/entry.server.tsx @@ -1,12 +1,3 @@ -import * as Sentry from '@sentry/remix'; - -Sentry.init({ - tracesSampleRate: 1.0, // Capture 100% of the transactions, reduce in production! - environment: 'qa', // dynamic sampling bias to keep transactions - dsn: process.env.E2E_TEST_DSN, - tunnel: 'http://localhost:3031/', // proxy server -}); - /** * By default, Remix will handle generating the HTTP Response for you. * You are free to delete this file if you'd like to, but if you ever want it revealed again, you can run `npx remix reveal` ✨ @@ -14,6 +5,7 @@ Sentry.init({ */ import { PassThrough } from 'node:stream'; +import * as Sentry from '@sentry/remix'; import type { AppLoadContext, EntryContext } from '@remix-run/node'; import { Response } from '@remix-run/node'; diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app/instrument.server.cjs b/dev-packages/e2e-tests/test-applications/create-remix-app/instrument.server.cjs new file mode 100644 index 000000000000..6d211cac4592 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/create-remix-app/instrument.server.cjs @@ -0,0 +1,8 @@ +const Sentry = require('@sentry/remix'); + +Sentry.init({ + tracesSampleRate: 1.0, // Capture 100% of the transactions, reduce in production! + environment: 'qa', // dynamic sampling bias to keep transactions + dsn: process.env.E2E_TEST_DSN, + tunnel: 'http://localhost:3031/', // proxy server +}); diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app/playwright.config.ts b/dev-packages/e2e-tests/test-applications/create-remix-app/playwright.config.ts new file mode 100644 index 000000000000..e9324bddb481 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/create-remix-app/playwright.config.ts @@ -0,0 +1,66 @@ +import type { PlaywrightTestConfig } from '@playwright/test'; +import { devices } from '@playwright/test'; + +const port = 3030; +const eventProxyPort = 3031; + +/** + * See https://playwright.dev/docs/test-configuration. + */ +const config: PlaywrightTestConfig = { + testDir: './tests', + /* Maximum time one test can run for. */ + timeout: 150_000, + expect: { + /** + * Maximum time expect() should wait for the condition to be met. + * For example in `await expect(locator).toHaveText();` + */ + timeout: 5000, + }, + /* Run tests in files in parallel */ + fullyParallel: true, + /* Fail the build on CI if you accidentally left test.only in the source code. */ + forbidOnly: !!process.env.CI, + /* Retry on CI only */ + retries: 0, + /* Opt out of parallel tests on CI. */ + workers: 1, + /* Reporter to use. See https://playwright.dev/docs/test-reporters */ + reporter: 'list', + /* Shared settings for all the projects below. See https://playwright.dev/docs/api/class-testoptions. */ + use: { + /* Maximum time each action such as `click()` can take. Defaults to 0 (no limit). */ + actionTimeout: 0, + + /* Collect trace when retrying the failed test. See https://playwright.dev/docs/trace-viewer */ + trace: 'on-first-retry', + + /* Base URL to use in actions like `await page.goto('/')`. */ + baseURL: `http://localhost:${port}`, + }, + + /* Configure projects for major browsers */ + projects: [ + { + name: 'chromium', + use: { + ...devices['Desktop Chrome'], + }, + }, + ], + + /* Run your local dev server before starting the tests */ + webServer: [ + { + command: 'node start-event-proxy.mjs', + port: eventProxyPort, + }, + { + command: `PORT=${port} NODE_OPTIONS='--require ./instrument.server.cjs' pnpm start`, + port: port, + }, + ], +}; + +export default config; diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app/tests/behaviour-server.test.ts b/dev-packages/e2e-tests/test-applications/create-remix-app/tests/behaviour-server.test.ts index 5107990507e4..837564c9d91d 100644 --- a/dev-packages/e2e-tests/test-applications/create-remix-app/tests/behaviour-server.test.ts +++ b/dev-packages/e2e-tests/test-applications/create-remix-app/tests/behaviour-server.test.ts @@ -10,7 +10,7 @@ test('Sends two linked transactions (server & client) to Sentry', async ({ page const httpServerTransactionPromise = waitForTransaction('create-remix-app', transactionEvent => { return ( transactionEvent.type === 'transaction' && - transactionEvent.contexts?.trace?.op === 'http.server' && + transactionEvent.contexts?.trace?.op === 'http' && transactionEvent.tags?.['sentry_test'] === testTag ); }); @@ -33,18 +33,21 @@ test('Sends two linked transactions (server & client) to Sentry', async ({ page const httpServerTraceId = httpServerTransaction.contexts?.trace?.trace_id; const httpServerSpanId = httpServerTransaction.contexts?.trace?.span_id; + const loaderSpanId = httpServerTransaction?.spans?.find( + span => span.data && span.data['code.function'] === 'loader', + )?.span_id; const pageLoadTraceId = pageloadTransaction.contexts?.trace?.trace_id; const pageLoadSpanId = pageloadTransaction.contexts?.trace?.span_id; const pageLoadParentSpanId = pageloadTransaction.contexts?.trace?.parent_span_id; - expect(httpServerTransaction.transaction).toBe('routes/_index'); + expect(httpServerTransaction.transaction).toBe('GET http://localhost:3030/'); expect(pageloadTransaction.transaction).toBe('routes/_index'); expect(httpServerTraceId).toBeDefined(); expect(httpServerSpanId).toBeDefined(); expect(pageLoadTraceId).toEqual(httpServerTraceId); - expect(pageLoadParentSpanId).toEqual(httpServerSpanId); + expect(pageLoadParentSpanId).toEqual(loaderSpanId); expect(pageLoadSpanId).not.toEqual(httpServerSpanId); }); diff --git a/packages/remix/jest.config.js b/packages/remix/jest.config.js deleted file mode 100644 index 7d6c8cb0990e..000000000000 --- a/packages/remix/jest.config.js +++ /dev/null @@ -1,8 +0,0 @@ -const baseConfig = require('../../jest/jest.config.js'); - -module.exports = { - ...baseConfig, - testPathIgnorePatterns: ['/build/', '/node_modules/', '/test/integration/'], - // Some tests take longer to finish, as flushing spans with OpenTelemetry takes some more time - testTimeout: 15000, -}; diff --git a/packages/remix/package.json b/packages/remix/package.json index e4aa316b7e85..c5d6646e3fbf 100644 --- a/packages/remix/package.json +++ b/packages/remix/package.json @@ -66,12 +66,15 @@ "@sentry/types": "8.9.2", "@sentry/utils": "8.9.2", "glob": "^10.3.4", + "opentelemetry-instrumentation-remix": "0.7.0", "yargs": "^17.6.0" }, "devDependencies": { "@remix-run/node": "^1.4.3", "@remix-run/react": "^1.4.3", - "@types/express": "^4.17.14" + "@types/express": "^4.17.14", + "vite": "^5.2.11", + "vitest": "^1.6.0" }, "peerDependencies": { "@remix-run/node": "1.x || 2.x", @@ -103,7 +106,7 @@ "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/ --project='chromium'", "test:integration:client:ci": "yarn test:integration:client --reporter='line'", - "test:integration:server": "export NODE_OPTIONS='--stack-trace-limit=25' && jest --config=test/integration/jest.config.js test/integration/test/server/", + "test:integration:server": "export NODE_OPTIONS='--stack-trace-limit=25' && vitest run test/integration/test/server/", "test:unit": "jest", "test:watch": "jest --watch", "yalc:publish": "ts-node ../../scripts/prepack.ts && yalc publish build --push --sig" diff --git a/packages/remix/playwright.config.ts b/packages/remix/playwright.config.ts index b75f865d9518..596f82334b71 100644 --- a/packages/remix/playwright.config.ts +++ b/packages/remix/playwright.config.ts @@ -12,6 +12,9 @@ const config: PlaywrightTestConfig = { // Note that 3 is a random number selected to work well with our CI setup workers: process.env.CI ? 3 : undefined, webServer: { + env: { + NODE_OPTIONS: '--require ./instrument.server.cjs', + }, command: '(cd test/integration/ && yarn build && yarn start)', port: 3000, }, diff --git a/packages/remix/src/index.server.ts b/packages/remix/src/index.server.ts index a6476b692fbf..d32a20c4281d 100644 --- a/packages/remix/src/index.server.ts +++ b/packages/remix/src/index.server.ts @@ -1,10 +1,17 @@ -import { applySdkMetadata, isInitialized } from '@sentry/core'; +import { applySdkMetadata } from '@sentry/core'; import type { NodeOptions } from '@sentry/node'; -import { init as nodeInit, setTag } from '@sentry/node'; +import { + getDefaultIntegrations as getDefaultNodeIntegrations, + init as nodeInit, + isInitialized, + setTag, +} from '@sentry/node'; +import type { Integration } from '@sentry/types'; import { logger } from '@sentry/utils'; import { DEBUG_BUILD } from './utils/debug-build'; import { instrumentServer } from './utils/instrumentServer'; +import { remixIntegration } from './utils/integrations/opentelemetry'; import type { RemixOptions } from './utils/remixOptions'; // We need to explicitly export @sentry/node as they end up under `default` in ESM builds @@ -41,7 +48,6 @@ export { withScope, withIsolationScope, makeNodeTransport, - getDefaultIntegrations, defaultStackParser, lastEventId, flush, @@ -119,10 +125,21 @@ export { ErrorBoundary, withErrorBoundary } from '@sentry/react'; export { withSentry } from './client/performance'; export { captureRemixErrorBoundaryError } from './client/errors'; export { browserTracingIntegration } from './client/browserTracingIntegration'; -export { wrapExpressCreateRequestHandler } from './utils/serverAdapters/express'; export type { SentryMetaArgs } from './utils/types'; +/** + * Returns the default Remix integrations. + * + * @param options The options for the SDK. + */ +export function getDefaultIntegrations(options: RemixOptions): Integration[] { + return [ + ...getDefaultNodeIntegrations(options).filter(integration => integration.name !== 'Http'), + remixIntegration(options), + ]; +} + /** Initializes Sentry Remix SDK on Node. */ export function init(options: RemixOptions): void { applySdkMetadata(options, 'remix', ['remix', 'node']); @@ -133,9 +150,11 @@ export function init(options: RemixOptions): void { return; } - instrumentServer(); + options.defaultIntegrations = getDefaultIntegrations(options as NodeOptions); nodeInit(options as NodeOptions); + instrumentServer(); + setTag('runtime', 'node'); } diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index 86f596b61eb7..812f872838b7 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -1,29 +1,23 @@ /* eslint-disable max-lines */ import { - SEMANTIC_ATTRIBUTE_SENTRY_OP, - SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, - SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, captureException, - continueTrace, getActiveSpan, - getClient, getRootSpan, handleCallbackErrors, hasTracingEnabled, - setHttpStatus, spanToJSON, spanToTraceHeader, - startSpan, withIsolationScope, } from '@sentry/core'; -import { getDynamicSamplingContextFromSpan } from '@sentry/opentelemetry'; -import type { Span, TransactionSource, WrappedFunction } from '@sentry/types'; +import { continueTrace, getDynamicSamplingContextFromSpan } from '@sentry/opentelemetry'; +import type { WrappedFunction } from '@sentry/types'; import { addExceptionMechanism, dynamicSamplingContextToSentryBaggageHeader, fill, isNodeEnv, isPrimitive, + isThenable, loadModule, logger, objectify, @@ -31,15 +25,7 @@ import { import { DEBUG_BUILD } from './debug-build'; import { getFutureFlagsServer, getRemixVersionFromBuild } from './futureFlags'; -import { - extractData, - getRequestMatch, - isDeferredData, - isResponse, - isRouteErrorResponse, - json, - matchServerRoutes, -} from './vendor/response'; +import { extractData, isDeferredData, isResponse, isRouteErrorResponse, json } from './vendor/response'; import type { AppData, AppLoadContext, @@ -52,7 +38,6 @@ import type { RemixRequest, RequestHandler, ServerBuild, - ServerRoute, ServerRouteManifest, } from './vendor/types'; import { normalizeRemixRequest } from './web-fetch'; @@ -210,51 +195,30 @@ function makeWrappedDocumentRequestFunction(remixVersion?: number) { context: EntryContext, loadContext?: Record, ): Promise { - const activeSpan = getActiveSpan(); - const rootSpan = activeSpan && getRootSpan(activeSpan); - - const name = rootSpan ? spanToJSON(rootSpan).description : undefined; - - return startSpan( - { - // If we don't have a root span, `onlyIfParent` will lead to the span not being created anyhow - // So we don't need to care too much about the fallback name, it's just for typing purposes.... - name: name || '', - onlyIfParent: true, - attributes: { - method: request.method, - url: request.url, - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.function.remix', - [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'function.remix.document_request', - }, - }, + return handleCallbackErrors( () => { - return handleCallbackErrors( - () => { - return origDocumentRequestFunction.call( - this, - request, - responseStatusCode, - responseHeaders, - context, - loadContext, - ); - }, - err => { - const isRemixV1 = !FUTURE_FLAGS?.v2_errorBoundary && remixVersion !== 2; - - // This exists to capture the server-side rendering errors on Remix v1 - // On Remix v2, we capture SSR errors at `handleError` - // We also skip primitives here, as we can't dedupe them, and also we don't expect any primitive SSR errors. - if (isRemixV1 && !isPrimitive(err)) { - // eslint-disable-next-line @typescript-eslint/no-floating-promises - captureRemixServerException(err, 'documentRequest', request); - } - - throw err; - }, + return origDocumentRequestFunction.call( + this, + request, + responseStatusCode, + responseHeaders, + context, + loadContext, ); }, + err => { + const isRemixV1 = !FUTURE_FLAGS?.v2_errorBoundary && remixVersion !== 2; + + // This exists to capture the server-side rendering errors on Remix v1 + // On Remix v2, we capture SSR errors at `handleError` + // We also skip primitives here, as we can't dedupe them, and also we don't expect any primitive SSR errors. + if (isRemixV1 && !isPrimitive(err)) { + // eslint-disable-next-line @typescript-eslint/no-floating-promises + captureRemixServerException(err, 'documentRequest', request); + } + + throw err; + }, ); }; }; @@ -265,82 +229,39 @@ function makeWrappedDataFunction( id: string, name: 'action' | 'loader', remixVersion: number, - manuallyInstrumented: boolean, ): DataFunction { return async function (this: unknown, args: DataFunctionArgs): Promise { - if (args.context.__sentry_express_wrapped__ && !manuallyInstrumented) { - return origFn.call(this, args); - } - - return startSpan( - { - op: `function.remix.${name}`, - name: id, - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.ui.remix', - name, - }, + return handleCallbackErrors( + async () => { + return origFn.call(this, args); }, - span => { - return handleCallbackErrors( - async () => { - if (span) { - const options = getClient()?.getOptions(); - - // We only capture form data for `action` functions, when `sendDefaultPii` is enabled. - if (name === 'action' && options?.sendDefaultPii) { - try { - // We clone the request for Remix be able to read the FormData later. - const clonedRequest = args.request.clone(); - - // This only will return the last name of multiple file uploads in a single FormData entry. - // We can switch to `unstable_parseMultipartFormData` when it's stable. - // https://remix.run/docs/en/main/utils/parse-multipart-form-data#unstable_parsemultipartformdata - const formData = await clonedRequest.formData(); - - formData.forEach((value, key) => { - span.setAttribute( - `remix.action_form_data.${key}`, - typeof value === 'string' ? value : '[non-string value]', - ); - }); - } catch (e) { - DEBUG_BUILD && logger.warn('Failed to read FormData from request', e); - } - } - } - - return origFn.call(this, args); - }, - err => { - const isRemixV2 = FUTURE_FLAGS?.v2_errorBoundary || remixVersion === 2; - - // On Remix v2, we capture all unexpected errors (except the `Route Error Response`s / Thrown Responses) in `handleError` function. - // This is both for consistency and also avoid duplicates such as primitives like `string` or `number` being captured twice. - // Remix v1 does not have a `handleError` function, so we capture all errors here. - if (isRemixV2 ? isResponse(err) : true) { - // eslint-disable-next-line @typescript-eslint/no-floating-promises - captureRemixServerException(err, name, args.request); - } + err => { + const isRemixV2 = FUTURE_FLAGS?.v2_errorBoundary || remixVersion === 2; + + // On Remix v2, we capture all unexpected errors (except the `Route Error Response`s / Thrown Responses) in `handleError` function. + // This is both for consistency and also avoid duplicates such as primitives like `string` or `number` being captured twice. + // Remix v1 does not have a `handleError` function, so we capture all errors here. + if (isRemixV2 ? isResponse(err) : true) { + // eslint-disable-next-line @typescript-eslint/no-floating-promises + captureRemixServerException(err, name, args.request); + } - throw err; - }, - ); + throw err; }, ); }; } const makeWrappedAction = - (id: string, remixVersion: number, manuallyInstrumented: boolean) => + (id: string, remixVersion: number) => (origAction: DataFunction): DataFunction => { - return makeWrappedDataFunction(origAction, id, 'action', remixVersion, manuallyInstrumented); + return makeWrappedDataFunction(origAction, id, 'action', remixVersion); }; const makeWrappedLoader = - (id: string, remixVersion: number, manuallyInstrumented: boolean) => + (id: string, remixVersion: number) => (origLoader: DataFunction): DataFunction => { - return makeWrappedDataFunction(origLoader, id, 'loader', remixVersion, manuallyInstrumented); + return makeWrappedDataFunction(origLoader, id, 'loader', remixVersion); }; function getTraceAndBaggage(): { @@ -409,91 +330,15 @@ function makeWrappedRootLoader(remixVersion: number) { }; } -/** - * 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]) => ({ - ...route, - children: createRoutes(manifest, id), - })); -} - -/** - * Starts a new active span for the given request to be used by different `RequestHandler` wrappers. - */ -export function startRequestHandlerSpan( - { - name, - source, - sentryTrace, - baggage, - method, - }: { - name: string; - source: TransactionSource; - sentryTrace: string; - baggage: string; - method: string; - }, - callback: (span: Span) => T, -): T { - return continueTrace( - { - sentryTrace, - baggage, - }, - () => { - return startSpan( - { - name, - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.remix', - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: source, - [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'http.server', - method, - }, - }, - callback, - ); - }, - ); -} - -/** - * Get transaction name from routes and url - */ -export function getTransactionName(routes: ServerRoute[], url: URL): [string, TransactionSource] { - const matches = matchServerRoutes(routes, url.pathname); - const match = matches && getRequestMatch(url, matches); - return match === null ? [url.pathname, 'url'] : [match.route.id || 'no-route-id', 'route']; -} - -function wrapRequestHandler(origRequestHandler: RequestHandler, build: ServerBuild): RequestHandler { - const routes = createRoutes(build.routes); - +function wrapRequestHandler(origRequestHandler: RequestHandler): RequestHandler { return async function (this: unknown, request: RemixRequest, loadContext?: AppLoadContext): Promise { - // This means that the request handler of the adapter (ex: express) is already wrapped. - // So we don't want to double wrap it. - if (loadContext?.__sentry_express_wrapped__) { - return origRequestHandler.call(this, request, loadContext); - } - const upperCaseMethod = request.method.toUpperCase(); - // We don't want to wrap OPTIONS and HEAD requests if (upperCaseMethod === 'OPTIONS' || upperCaseMethod === 'HEAD') { return origRequestHandler.call(this, request, loadContext); } return withIsolationScope(async isolationScope => { - const options = getClient()?.getOptions(); - let normalizedRequest: Record = request; try { @@ -501,50 +346,25 @@ function wrapRequestHandler(origRequestHandler: RequestHandler, build: ServerBui } catch (e) { DEBUG_BUILD && logger.warn('Failed to normalize Remix request'); } - - const url = new URL(request.url); - const [name, source] = getTransactionName(routes, url); - - isolationScope.setTransactionName(name); isolationScope.setSDKProcessingMetadata({ request: { ...normalizedRequest, - route: { - path: name, - }, }, }); - - if (!options || !hasTracingEnabled(options)) { - return origRequestHandler.call(this, request, loadContext); - } - - return startRequestHandlerSpan( + return continueTrace( { - name, - source, sentryTrace: request.headers.get('sentry-trace') || '', baggage: request.headers.get('baggage') || '', - method: request.method, }, - async span => { - const res = (await origRequestHandler.call(this, request, loadContext)) as Response; - - if (isResponse(res)) { - setHttpStatus(span, res.status); - } - - return res; + async () => { + return (await origRequestHandler.call(this, request, loadContext)) as Response; }, ); }); }; } -/** - * Instruments `remix` ServerBuild for performance tracing and error tracking. - */ -export function instrumentBuild(build: ServerBuild, manuallyInstrumented: boolean = false): ServerBuild { +function instrumentBuildCallback(build: ServerBuild): ServerBuild { const routes: ServerRouteManifest = {}; const remixVersion = getRemixVersionFromBuild(build); @@ -566,12 +386,12 @@ export function instrumentBuild(build: ServerBuild, manuallyInstrumented: boolea const routeAction = wrappedRoute.module.action as undefined | WrappedFunction; if (routeAction && !routeAction.__sentry_original__) { - fill(wrappedRoute.module, 'action', makeWrappedAction(id, remixVersion, manuallyInstrumented)); + fill(wrappedRoute.module, 'action', makeWrappedAction(id, remixVersion)); } const routeLoader = wrappedRoute.module.loader as undefined | WrappedFunction; if (routeLoader && !routeLoader.__sentry_original__) { - fill(wrappedRoute.module, 'loader', makeWrappedLoader(id, remixVersion, manuallyInstrumented)); + fill(wrappedRoute.module, 'loader', makeWrappedLoader(id, remixVersion)); } // Entry module should have a loader function to provide `sentry-trace` and `baggage` @@ -591,15 +411,45 @@ export function instrumentBuild(build: ServerBuild, manuallyInstrumented: boolea return { ...build, routes, entry: wrappedEntry }; } +/** + * Instruments `remix` ServerBuild for performance tracing and error tracking. + */ +export function instrumentBuild( + build: ServerBuild | (() => ServerBuild | Promise), +): ServerBuild | Promise { + if (typeof build === 'function') { + const resolvedBuild = build(); + + if (isThenable(resolvedBuild)) { + return resolvedBuild.then(build => { + FUTURE_FLAGS = getFutureFlagsServer(build); + + return instrumentBuildCallback(build); + }); + } else { + FUTURE_FLAGS = getFutureFlagsServer(resolvedBuild); + + return instrumentBuildCallback(resolvedBuild); + } + } else { + FUTURE_FLAGS = getFutureFlagsServer(build as ServerBuild); + + return instrumentBuildCallback(build as ServerBuild); + } +} + function makeWrappedCreateRequestHandler( origCreateRequestHandler: CreateRequestHandlerFunction, ): CreateRequestHandlerFunction { - return function (this: unknown, build: ServerBuild, ...args: unknown[]): RequestHandler { - FUTURE_FLAGS = getFutureFlagsServer(build); - const newBuild = instrumentBuild(build, false); + return function ( + this: unknown, + build: ServerBuild | (() => Promise), + ...args: unknown[] + ): RequestHandler { + const newBuild = instrumentBuild(build); const requestHandler = origCreateRequestHandler.call(this, newBuild, ...args); - return wrapRequestHandler(requestHandler, newBuild); + return wrapRequestHandler(requestHandler); }; } diff --git a/packages/remix/src/utils/integrations/opentelemetry.ts b/packages/remix/src/utils/integrations/opentelemetry.ts new file mode 100644 index 000000000000..92b000416608 --- /dev/null +++ b/packages/remix/src/utils/integrations/opentelemetry.ts @@ -0,0 +1,24 @@ +import { RemixInstrumentation } from 'opentelemetry-instrumentation-remix'; + +import { defineIntegration } from '@sentry/core'; +import { addOpenTelemetryInstrumentation } from '@sentry/node'; +import type { IntegrationFn } from '@sentry/types'; +import type { RemixOptions } from '../remixOptions'; + +const _remixIntegration = ((options?: RemixOptions) => { + return { + name: 'Remix', + setupOnce() { + addOpenTelemetryInstrumentation( + new RemixInstrumentation({ + actionFormDataAttributes: options?.sendDefaultPii ? options?.captureActionFormDataKeys : undefined, + }), + ); + }, + }; +}) satisfies IntegrationFn; + +/** + * Instrumentation for aws-sdk package + */ +export const remixIntegration = defineIntegration(_remixIntegration); diff --git a/packages/remix/src/utils/remixOptions.ts b/packages/remix/src/utils/remixOptions.ts index 4a1fe13e18e1..26eae9c024c3 100644 --- a/packages/remix/src/utils/remixOptions.ts +++ b/packages/remix/src/utils/remixOptions.ts @@ -2,4 +2,6 @@ import type { NodeOptions } from '@sentry/node'; import type { BrowserOptions } from '@sentry/react'; import type { Options } from '@sentry/types'; -export type RemixOptions = Options | BrowserOptions | NodeOptions; +export type RemixOptions = (Options | BrowserOptions | NodeOptions) & { + captureActionFormDataKeys?: Record; +}; diff --git a/packages/remix/src/utils/serverAdapters/express.ts b/packages/remix/src/utils/serverAdapters/express.ts deleted file mode 100644 index d4caed091015..000000000000 --- a/packages/remix/src/utils/serverAdapters/express.ts +++ /dev/null @@ -1,226 +0,0 @@ -import { getClient, hasTracingEnabled, setHttpStatus, withIsolationScope } from '@sentry/core'; -import { flush } from '@sentry/node'; -import type { Span, TransactionSource } from '@sentry/types'; -import { extractRequestData, fill, isString, logger } from '@sentry/utils'; - -import { DEBUG_BUILD } from '../debug-build'; -import { createRoutes, getTransactionName, instrumentBuild, startRequestHandlerSpan } from '../instrumentServer'; -import type { - AppLoadContext, - ExpressCreateRequestHandler, - ExpressCreateRequestHandlerOptions, - ExpressNextFunction, - ExpressRequest, - ExpressRequestHandler, - ExpressResponse, - GetLoadContextFunction, - ServerBuild, - ServerRoute, -} from '../vendor/types'; - -function wrapExpressRequestHandler( - origRequestHandler: ExpressRequestHandler, - build: ServerBuild | (() => Promise | ServerBuild), -): ExpressRequestHandler { - let routes: ServerRoute[]; - - return async function ( - this: unknown, - req: ExpressRequest, - res: ExpressResponse, - next: ExpressNextFunction, - ): Promise { - await withIsolationScope(async isolationScope => { - // eslint-disable-next-line @typescript-eslint/unbound-method - res.end = wrapEndMethod(res.end); - - const request = extractRequestData(req); - const options = getClient()?.getOptions(); - - isolationScope.setSDKProcessingMetadata({ request }); - - if (!options || !hasTracingEnabled(options) || !request.url || !request.method) { - return origRequestHandler.call(this, req, res, next); - } - - const url = new URL(request.url); - - // This is only meant to be used on development servers, so we don't need to worry about performance here - if (build && typeof build === 'function') { - const resolvedBuild = build(); - - if (resolvedBuild instanceof Promise) { - return resolvedBuild.then(resolved => { - routes = createRoutes(resolved.routes); - - const [name, source] = getTransactionName(routes, url); - isolationScope.setTransactionName(name); - - startRequestHandlerTransaction.call(this, origRequestHandler, req, res, next, name, source); - }); - } else { - routes = createRoutes(resolvedBuild.routes); - } - } else { - routes = createRoutes(build.routes); - } - - const [name, source] = getTransactionName(routes, url); - isolationScope.setTransactionName(name); - - return startRequestHandlerTransaction.call(this, origRequestHandler, req, res, next, name, source); - }); - }; -} - -function startRequestHandlerTransaction( - this: unknown, - origRequestHandler: ExpressRequestHandler, - req: ExpressRequest, - res: ExpressResponse, - next: ExpressNextFunction, - name: string, - source: TransactionSource, -): unknown { - return startRequestHandlerSpan( - { - name, - source, - sentryTrace: (req.headers && isString(req.headers['sentry-trace']) && req.headers['sentry-trace']) || '', - baggage: (req.headers && isString(req.headers.baggage) && req.headers.baggage) || '', - method: req.method, - }, - span => { - // save a link to the transaction on the response, so that even if there's an error (landing us outside of - // the domain), we can still finish it (albeit possibly missing some scope data) - (res as AugmentedExpressResponse).__sentrySpan = span; - return origRequestHandler.call(this, req, res, next); - }, - ); -} - -function wrapGetLoadContext(origGetLoadContext: () => AppLoadContext): GetLoadContextFunction { - return function (this: unknown, req: ExpressRequest, res: ExpressResponse): AppLoadContext { - const loadContext = (origGetLoadContext.call(this, req, res) || {}) as AppLoadContext; - - loadContext['__sentry_express_wrapped__'] = true; - - return loadContext; - }; -} - -// wrap build function which returns either a Promise or the build itself -// This is currently only required for Vite development mode with HMR -function wrapBuildFn(origBuildFn: () => Promise | ServerBuild): () => Promise | ServerBuild { - return async function (this: unknown, ...args: unknown[]) { - const resolvedBuild = origBuildFn.call(this, ...args); - - if (resolvedBuild instanceof Promise) { - return resolvedBuild.then(resolved => { - return instrumentBuild(resolved, true); - }); - } - - return instrumentBuild(resolvedBuild, true); - }; -} - -// A wrapper around build if it's a Promise or a function that returns a Promise that calls instrumentServer on the resolved value -// This is currently only required for Vite development mode with HMR -function wrapBuild( - build: ServerBuild | (() => Promise | ServerBuild), -): ServerBuild | (() => Promise | ServerBuild) { - if (typeof build === 'function') { - return wrapBuildFn(build); - } - - return instrumentBuild(build, true); -} - -/** - * Instruments `createRequestHandler` from `@remix-run/express` - */ -export function wrapExpressCreateRequestHandler( - origCreateRequestHandler: ExpressCreateRequestHandler, - // eslint-disable-next-line @typescript-eslint/no-explicit-any -): (options: any) => ExpressRequestHandler { - return function (this: unknown, options: ExpressCreateRequestHandlerOptions): ExpressRequestHandler { - if (!('getLoadContext' in options)) { - options['getLoadContext'] = () => ({}); - } - - fill(options, 'getLoadContext', wrapGetLoadContext); - - const newBuild = wrapBuild(options.build); - const requestHandler = origCreateRequestHandler.call(this, { - ...options, - build: newBuild, - }); - - return wrapExpressRequestHandler(requestHandler, newBuild); - }; -} - -export type AugmentedExpressResponse = ExpressResponse & { - __sentrySpan?: Span; -}; - -type ResponseEndMethod = AugmentedExpressResponse['end']; -type WrappedResponseEndMethod = AugmentedExpressResponse['end']; - -/** - * Wrap `res.end()` so that it closes the transaction and flushes events before letting the request finish. - * - * Note: This wraps a sync method with an async method. While in general that's not a great idea in terms of keeping - * things in the right order, in this case it's safe, because the native `.end()` actually *is* async, and its run - * actually *is* awaited, just manually so (which reflects the fact that the core of the request/response code in Node - * by far predates the introduction of `async`/`await`). When `.end()` is done, it emits the `prefinish` event, and - * only once that fires does request processing continue. See - * https://github.com/nodejs/node/commit/7c9b607048f13741173d397795bac37707405ba7. - * - * @param origEnd The original `res.end()` method - * @returns The wrapped version - */ -function wrapEndMethod(origEnd: ResponseEndMethod): WrappedResponseEndMethod { - return async function newEnd(this: AugmentedExpressResponse, ...args: unknown[]) { - await finishSentryProcessing(this); - - return origEnd.call(this, ...args); - } as unknown as WrappedResponseEndMethod; -} - -/** - * Close the open transaction (if any) and flush events to Sentry. - * - * @param res The outgoing response for this request, on which the transaction is stored - */ -async function finishSentryProcessing(res: AugmentedExpressResponse): Promise { - const { __sentrySpan: span } = res; - - if (span) { - setHttpStatus(span, res.statusCode); - - // Push `transaction.finish` to the next event loop so open spans have a better chance of finishing before the - // transaction closes, and make sure to wait until that's done before flushing events - await new Promise(resolve => { - setImmediate(() => { - // Double checking whether the span is not already finished, - // OpenTelemetry gives error if we try to end a finished span - if (span.isRecording()) { - span.end(); - } - resolve(); - }); - }); - } - - // Flush the event queue to ensure that events get sent to Sentry before the response is finished and the lambda - // ends. If there was an error, rethrow it so that the normal exception-handling mechanisms can apply. - try { - DEBUG_BUILD && logger.log('Flushing events...'); - await flush(2000); - DEBUG_BUILD && logger.log('Done flushing events'); - } catch (e) { - DEBUG_BUILD && logger.log('Error while flushing events:\n', e); - } -} diff --git a/packages/remix/test/integration/app_v1/entry.server.tsx b/packages/remix/test/integration/app_v1/entry.server.tsx index d4ad53d80aec..6db43d935cdc 100644 --- a/packages/remix/test/integration/app_v1/entry.server.tsx +++ b/packages/remix/test/integration/app_v1/entry.server.tsx @@ -1,14 +1,3 @@ -// it is important this is first! -import * as Sentry from '@sentry/remix'; - -Sentry.init({ - dsn: 'https://public@dsn.ingest.sentry.io/1337', - tracesSampleRate: 1, - tracePropagationTargets: ['example.org'], - // Disabling to test series of envelopes deterministically. - autoSessionTracking: false, -}); - import type { EntryContext } from '@remix-run/node'; import { RemixServer } from '@remix-run/react'; import { renderToString } from 'react-dom/server'; diff --git a/packages/remix/test/integration/app_v2/entry.server.tsx b/packages/remix/test/integration/app_v2/entry.server.tsx index 04d5ef52f6c2..86b0312eb92f 100644 --- a/packages/remix/test/integration/app_v2/entry.server.tsx +++ b/packages/remix/test/integration/app_v2/entry.server.tsx @@ -1,14 +1,5 @@ -// it is important this is first! import * as Sentry from '@sentry/remix'; -Sentry.init({ - dsn: 'https://public@dsn.ingest.sentry.io/1337', - tracesSampleRate: 1, - tracePropagationTargets: ['example.org'], - // Disabling to test series of envelopes deterministically. - autoSessionTracking: false, -}); - import type { EntryContext } from '@remix-run/node'; import { RemixServer } from '@remix-run/react'; import { renderToString } from 'react-dom/server'; diff --git a/packages/remix/test/integration/instrument.server.cjs b/packages/remix/test/integration/instrument.server.cjs new file mode 100644 index 000000000000..199d4e309c41 --- /dev/null +++ b/packages/remix/test/integration/instrument.server.cjs @@ -0,0 +1,9 @@ +const Sentry = require('@sentry/remix'); + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + tracesSampleRate: 1, + tracePropagationTargets: ['example.org'], + // Disabling to test series of envelopes deterministically. + autoSessionTracking: false, +}); diff --git a/packages/remix/test/integration/jest.config.js b/packages/remix/test/integration/jest.config.js deleted file mode 100644 index 82c2059da915..000000000000 --- a/packages/remix/test/integration/jest.config.js +++ /dev/null @@ -1,8 +0,0 @@ -const baseConfig = require('../../jest.config.js'); - -module.exports = { - ...baseConfig, - testMatch: [`${__dirname}/test/server/**/*.test.ts`], - testPathIgnorePatterns: [`${__dirname}/test/client`], - forceExit: true, -}; diff --git a/packages/remix/test/integration/package.json b/packages/remix/test/integration/package.json index 63560ec64e8b..68ab036c875d 100644 --- a/packages/remix/test/integration/package.json +++ b/packages/remix/test/integration/package.json @@ -3,8 +3,8 @@ "sideEffects": false, "scripts": { "build": "remix build", - "dev": "remix dev", - "start": "remix-serve build" + "dev": "NODE_OPTIONS='--require ./instrument.server.cjs' remix dev", + "start": "NODE_OPTIONS='--require ./instrument.server.cjs' && remix-serve ./build/index.js" }, "dependencies": { "@remix-run/express": "1.17.0", diff --git a/packages/remix/test/integration/test/client/meta-tags.test.ts b/packages/remix/test/integration/test/client/meta-tags.test.ts index 34bf6fec1e01..db919d950a2c 100644 --- a/packages/remix/test/integration/test/client/meta-tags.test.ts +++ b/packages/remix/test/integration/test/client/meta-tags.test.ts @@ -42,10 +42,10 @@ test('should send transactions with corresponding `sentry-trace` and `baggage` i const sentryBaggageContent = await sentryBaggageTag?.getAttribute('content'); expect(sentryTraceContent).toContain( - `${envelope.contexts?.trace.trace_id}-${envelope.contexts?.trace.parent_span_id}-`, + `${envelope.contexts?.trace?.trace_id}-${envelope.contexts?.trace?.parent_span_id}-`, ); - expect(sentryBaggageContent).toContain(envelope.contexts?.trace.trace_id); + expect(sentryBaggageContent).toContain(envelope.contexts?.trace?.trace_id); }); test('should send transactions with corresponding `sentry-trace` and `baggage` inside a parameterized route', async ({ @@ -59,8 +59,8 @@ test('should send transactions with corresponding `sentry-trace` and `baggage` i const sentryBaggageContent = await sentryBaggageTag?.getAttribute('content'); expect(sentryTraceContent).toContain( - `${envelope.contexts?.trace.trace_id}-${envelope.contexts?.trace.parent_span_id}-`, + `${envelope.contexts?.trace?.trace_id}-${envelope.contexts?.trace?.parent_span_id}-`, ); - expect(sentryBaggageContent).toContain(envelope.contexts?.trace.trace_id); + expect(sentryBaggageContent).toContain(envelope.contexts?.trace?.trace_id); }); diff --git a/packages/remix/test/integration/test/server/action.test.ts b/packages/remix/test/integration/test/server/action.test.ts index 4755523262dc..65f04cd3ebf3 100644 --- a/packages/remix/test/integration/test/server/action.test.ts +++ b/packages/remix/test/integration/test/server/action.test.ts @@ -1,35 +1,49 @@ +import { describe, it } from 'vitest'; import { RemixTestEnv, assertSentryEvent, assertSentryTransaction } from './utils/helpers'; const useV2 = process.env.REMIX_VERSION === '2'; -jest.spyOn(console, 'error').mockImplementation(); - -// Repeat tests for each adapter -describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', adapter => { +describe('Remix API Actions', () => { it('correctly instruments a parameterized Remix API action', async () => { - const env = await RemixTestEnv.init(adapter); + const env = await RemixTestEnv.init(); const url = `${env.url}/action-json-response/123123`; - const envelope = await env.getEnvelopeRequest({ url, method: 'post', envelopeType: 'transaction' }); - const transaction = envelope[2]; + const envelopes = await env.getMultipleEnvelopeRequest({ + url, + method: 'post', + envelopeType: 'transaction', + count: 1, + }); + const transaction = envelopes[0][2]; assertSentryTransaction(transaction, { - transaction: `routes/action-json-response${useV2 ? '.' : '/'}$id`, + transaction: `POST action-json-response/:id`, spans: [ { - description: `routes/action-json-response${useV2 ? '.' : '/'}$id`, - op: 'function.remix.action', - }, - { - description: 'root', - op: 'function.remix.loader', + data: { + 'code.function': 'action', + 'sentry.op': 'http', + 'otel.kind': 'INTERNAL', + 'match.route.id': `routes/action-json-response${useV2 ? '.' : '/'}$id`, + 'match.params.id': '123123', + }, }, { - description: `routes/action-json-response${useV2 ? '.' : '/'}$id`, - op: 'function.remix.loader', + data: { + 'code.function': 'loader', + 'sentry.op': 'http', + 'otel.kind': 'INTERNAL', + 'match.route.id': `routes/action-json-response${useV2 ? '.' : '/'}$id`, + 'match.params.id': '123123', + }, }, { - description: `routes/action-json-response${useV2 ? '.' : '/'}$id`, - op: 'function.remix.document_request', + data: { + 'code.function': 'loader', + 'sentry.op': 'http', + 'otel.kind': 'INTERNAL', + 'match.route.id': 'root', + 'match.params.id': '123123', + }, }, ], request: { @@ -45,7 +59,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada }); it('reports an error thrown from the action', async () => { - const env = await RemixTestEnv.init(adapter); + const env = await RemixTestEnv.init(); const url = `${env.url}/action-json-response/-1`; const envelopes = await env.getMultipleEnvelopeRequest({ @@ -70,7 +84,6 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada }); assertSentryEvent(event[2], { - transaction: expect.stringMatching(/routes\/action-json-response(\/|\.)\$id/), exception: { values: [ { @@ -91,7 +104,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada }); it('includes request data in transaction and error events', async () => { - const env = await RemixTestEnv.init(adapter); + const env = await RemixTestEnv.init(); const url = `${env.url}/action-json-response/-1`; const envelopes = await env.getMultipleEnvelopeRequest({ @@ -105,7 +118,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada const [event] = envelopes.filter(envelope => envelope[1].type === 'event'); assertSentryTransaction(transaction[2], { - transaction: `routes/action-json-response${useV2 ? '.' : '/'}$id`, + transaction: `POST action-json-response/:id`, request: { method: 'POST', url, @@ -118,7 +131,6 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada }); assertSentryEvent(event[2], { - transaction: expect.stringMatching(/routes\/action-json-response(\/|\.)\$id/), exception: { values: [ { @@ -140,7 +152,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada }); it('handles an error-throwing redirection target', async () => { - const env = await RemixTestEnv.init(adapter); + const env = await RemixTestEnv.init(); const url = `${env.url}/action-json-response/-2`; const envelopes = await env.getMultipleEnvelopeRequest({ @@ -156,33 +168,30 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada assertSentryTransaction(transaction_1[2], { contexts: { trace: { - op: 'http.server', + op: 'http', status: 'ok', data: { - method: 'POST', 'http.response.status_code': 302, }, }, }, - transaction: `routes/action-json-response${useV2 ? '.' : '/'}$id`, + transaction: `POST action-json-response/:id`, }); assertSentryTransaction(transaction_2[2], { contexts: { trace: { - op: 'http.server', + op: 'http', status: 'internal_error', data: { - method: 'GET', 'http.response.status_code': 500, }, }, }, - transaction: `routes/action-json-response${useV2 ? '.' : '/'}$id`, + transaction: `GET action-json-response/:id`, }); assertSentryEvent(event[2], { - transaction: expect.stringMatching(/routes\/action-json-response(\/|\.)\$id/), exception: { values: [ { @@ -203,7 +212,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada }); it('handles a thrown `json()` error response with `statusText`', async () => { - const env = await RemixTestEnv.init(adapter); + const env = await RemixTestEnv.init(); const url = `${env.url}/action-json-response/-3`; const envelopes = await env.getMultipleEnvelopeRequest({ @@ -219,19 +228,17 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada assertSentryTransaction(transaction[2], { contexts: { trace: { - op: 'http.server', + op: 'http', status: 'internal_error', data: { - method: 'POST', 'http.response.status_code': 500, }, }, }, - transaction: `routes/action-json-response${useV2 ? '.' : '/'}$id`, + transaction: `POST action-json-response/:id`, }); assertSentryEvent(event[2], { - transaction: expect.stringMatching(/routes\/action-json-response(\/|\.)\$id/), exception: { values: [ { @@ -252,7 +259,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada }); it('handles a thrown `json()` error response without `statusText`', async () => { - const env = await RemixTestEnv.init(adapter); + const env = await RemixTestEnv.init(); const url = `${env.url}/action-json-response/-4`; const envelopes = await env.getMultipleEnvelopeRequest({ @@ -268,19 +275,17 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada assertSentryTransaction(transaction[2], { contexts: { trace: { - op: 'http.server', + op: 'http', status: 'internal_error', data: { - method: 'POST', 'http.response.status_code': 500, }, }, }, - transaction: `routes/action-json-response${useV2 ? '.' : '/'}$id`, + transaction: `POST action-json-response/:id`, }); assertSentryEvent(event[2], { - transaction: expect.stringMatching(/routes\/action-json-response(\/|\.)\$id/), exception: { values: [ { @@ -301,7 +306,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada }); it('handles a thrown `json()` error response with string body', async () => { - const env = await RemixTestEnv.init(adapter); + const env = await RemixTestEnv.init(); const url = `${env.url}/action-json-response/-5`; const envelopes = await env.getMultipleEnvelopeRequest({ @@ -317,19 +322,17 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada assertSentryTransaction(transaction[2], { contexts: { trace: { - op: 'http.server', + op: 'http', status: 'internal_error', data: { - method: 'POST', 'http.response.status_code': 500, }, }, }, - transaction: `routes/action-json-response${useV2 ? '.' : '/'}$id`, + transaction: `POST action-json-response/:id`, }); assertSentryEvent(event[2], { - transaction: expect.stringMatching(/routes\/action-json-response(\/|\.)\$id/), exception: { values: [ { @@ -350,7 +353,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada }); it('handles a thrown `json()` error response with an empty object', async () => { - const env = await RemixTestEnv.init(adapter); + const env = await RemixTestEnv.init(); const url = `${env.url}/action-json-response/-6`; const envelopes = await env.getMultipleEnvelopeRequest({ @@ -366,19 +369,17 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada assertSentryTransaction(transaction[2], { contexts: { trace: { - op: 'http.server', + op: 'http', status: 'internal_error', data: { - method: 'POST', 'http.response.status_code': 500, }, }, }, - transaction: `routes/action-json-response${useV2 ? '.' : '/'}$id`, + transaction: `POST action-json-response/:id`, }); assertSentryEvent(event[2], { - transaction: expect.stringMatching(/routes\/action-json-response(\/|\.)\$id/), exception: { values: [ { @@ -399,7 +400,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada }); it('handles thrown string (primitive) from an action', async () => { - const env = await RemixTestEnv.init(adapter); + const env = await RemixTestEnv.init(); const url = `${env.url}/server-side-unexpected-errors/-1`; const envelopes = await env.getMultipleEnvelopeRequest({ @@ -415,19 +416,17 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada assertSentryTransaction(transaction[2], { contexts: { trace: { - op: 'http.server', + op: 'http', status: 'internal_error', data: { - method: 'POST', 'http.response.status_code': 500, }, }, }, - transaction: `routes/server-side-unexpected-errors${useV2 ? '.' : '/'}$id`, + transaction: `POST server-side-unexpected-errors/:id`, }); assertSentryEvent(event[2], { - transaction: expect.stringMatching(/routes\/server-side-unexpected-errors(\/|\.)\$id/), exception: { values: [ { @@ -448,7 +447,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada }); it('handles thrown object from an action', async () => { - const env = await RemixTestEnv.init(adapter); + const env = await RemixTestEnv.init(); const url = `${env.url}/server-side-unexpected-errors/-2`; const envelopes = await env.getMultipleEnvelopeRequest({ @@ -464,19 +463,17 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada assertSentryTransaction(transaction[2], { contexts: { trace: { - op: 'http.server', + op: 'http', status: 'internal_error', data: { - method: 'POST', 'http.response.status_code': 500, }, }, }, - transaction: `routes/server-side-unexpected-errors${useV2 ? '.' : '/'}$id`, + transaction: `POST server-side-unexpected-errors/:id`, }); assertSentryEvent(event[2], { - transaction: expect.stringMatching(/routes\/server-side-unexpected-errors(\/|\.)\$id/), exception: { values: [ { diff --git a/packages/remix/test/integration/test/server/loader.test.ts b/packages/remix/test/integration/test/server/loader.test.ts index da046d02f7e5..162b656bfe4f 100644 --- a/packages/remix/test/integration/test/server/loader.test.ts +++ b/packages/remix/test/integration/test/server/loader.test.ts @@ -1,14 +1,12 @@ import { Event } from '@sentry/types'; +import { describe, expect, it } from 'vitest'; import { RemixTestEnv, assertSentryEvent, assertSentryTransaction } from './utils/helpers'; const useV2 = process.env.REMIX_VERSION === '2'; -jest.spyOn(console, 'error').mockImplementation(); - -// Repeat tests for each adapter -describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', adapter => { +describe('Remix API Loaders', () => { it('reports an error thrown from the loader', async () => { - const env = await RemixTestEnv.init(adapter); + const env = await RemixTestEnv.init(); const url = `${env.url}/loader-json-response/-2`; const envelopes = await env.getMultipleEnvelopeRequest({ url, count: 2, envelopeType: ['transaction', 'event'] }); @@ -28,7 +26,6 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada }); assertSentryEvent(event, { - transaction: expect.stringMatching(/routes\/loader-json-response(\/|\.)\$id/), exception: { values: [ { @@ -49,7 +46,7 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada }); it('reports a thrown error response the loader', async () => { - const env = await RemixTestEnv.init(adapter); + const env = await RemixTestEnv.init(); const url = `${env.url}/loader-throw-response/-1`; // We also wait for the transaction, even though we don't care about it for this test @@ -71,7 +68,6 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada }); assertSentryEvent(event, { - transaction: expect.stringMatching(/routes\/loader-throw-response(\/|\.)\$id/), exception: { values: [ { @@ -92,35 +88,39 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada }); it('correctly instruments a parameterized Remix API loader', async () => { - const env = await RemixTestEnv.init(adapter); + const env = await RemixTestEnv.init(); const url = `${env.url}/loader-json-response/123123`; const envelope = await env.getEnvelopeRequest({ url, envelopeType: 'transaction' }); const transaction = envelope[2]; assertSentryTransaction(transaction, { - transaction: `routes/loader-json-response${useV2 ? '.' : '/'}$id`, + transaction: `GET loader-json-response/:id`, transaction_info: { source: 'route', }, spans: [ { - description: 'root', - op: 'function.remix.loader', - }, - { - description: `routes/loader-json-response${useV2 ? '.' : '/'}$id`, - op: 'function.remix.loader', + data: { + 'code.function': 'loader', + 'otel.kind': 'INTERNAL', + 'sentry.op': 'http', + }, + origin: 'manual', }, { - description: `routes/loader-json-response${useV2 ? '.' : '/'}$id`, - op: 'function.remix.document_request', + data: { + 'code.function': 'loader', + 'otel.kind': 'INTERNAL', + 'sentry.op': 'http', + }, + origin: 'manual', }, ], }); }); it('handles an error-throwing redirection target', async () => { - const env = await RemixTestEnv.init(adapter); + const env = await RemixTestEnv.init(); const url = `${env.url}/loader-json-response/-1`; const envelopes = await env.getMultipleEnvelopeRequest({ @@ -135,33 +135,30 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada assertSentryTransaction(transaction_1[2], { contexts: { trace: { - op: 'http.server', + op: 'http', status: 'ok', data: { - method: 'GET', 'http.response.status_code': 302, }, }, }, - transaction: `routes/loader-json-response${useV2 ? '.' : '/'}$id`, + transaction: `GET loader-json-response/:id`, }); assertSentryTransaction(transaction_2[2], { contexts: { trace: { - op: 'http.server', + op: 'http', status: 'internal_error', data: { - method: 'GET', 'http.response.status_code': 500, }, }, }, - transaction: `routes/loader-json-response${useV2 ? '.' : '/'}$id`, + transaction: `GET loader-json-response/:id`, }); assertSentryEvent(event[2], { - transaction: expect.stringMatching(/routes\/loader-json-response(\/|\.)\$id/), exception: { values: [ { @@ -182,7 +179,7 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada }); it('makes sure scope does not bleed between requests', async () => { - const env = await RemixTestEnv.init(adapter); + const env = await RemixTestEnv.init(); const envelopes = await Promise.all([ env.getEnvelopeRequest({ url: `${env.url}/scope-bleed/1`, endServer: false, envelopeType: 'transaction' }), @@ -205,7 +202,7 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada }); it('continues transaction from sentry-trace header and baggage', async () => { - const env = await RemixTestEnv.init(adapter); + const env = await RemixTestEnv.init(); const url = `${env.url}/loader-json-response/3`; // send sentry-trace and baggage headers to loader @@ -232,50 +229,39 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada }); it('correctly instruments a deferred loader', async () => { - const env = await RemixTestEnv.init(adapter); + const env = await RemixTestEnv.init(); const url = `${env.url}/loader-defer-response/123123`; const envelope = await env.getEnvelopeRequest({ url, envelopeType: 'transaction' }); const transaction = envelope[2]; assertSentryTransaction(transaction, { - transaction: useV2 ? 'routes/loader-defer-response.$id' : 'routes/loader-defer-response/$id', + transaction: 'GET loader-defer-response/:id', transaction_info: { source: 'route', }, - spans: useV2 - ? [ - { - description: 'root', - op: 'function.remix.loader', - }, - { - description: 'routes/loader-defer-response.$id', - op: 'function.remix.loader', - }, - { - description: 'routes/loader-defer-response.$id', - op: 'function.remix.document_request', - }, - ] - : [ - { - description: 'root', - op: 'function.remix.loader', - }, - { - description: 'routes/loader-defer-response/$id', - op: 'function.remix.loader', - }, - { - description: 'routes/loader-defer-response/$id', - op: 'function.remix.document_request', - }, - ], + spans: [ + { + data: { + 'code.function': 'loader', + 'sentry.op': 'http', + 'otel.kind': 'INTERNAL', + 'match.route.id': `routes/loader-defer-response${useV2 ? '.' : '/'}$id`, + }, + }, + { + data: { + 'code.function': 'loader', + 'sentry.op': 'http', + 'otel.kind': 'INTERNAL', + 'match.route.id': 'root', + }, + }, + ], }); }); it('does not capture thrown redirect responses', async () => { - const env = await RemixTestEnv.init(adapter); + const env = await RemixTestEnv.init(); const url = `${env.url}/throw-redirect`; const envelopesCount = await env.countEnvelopes({ diff --git a/packages/remix/test/integration/test/server/ssr.test.ts b/packages/remix/test/integration/test/server/ssr.test.ts index 39c49a0d2957..bab2e1857644 100644 --- a/packages/remix/test/integration/test/server/ssr.test.ts +++ b/packages/remix/test/integration/test/server/ssr.test.ts @@ -1,10 +1,11 @@ +import { describe, expect, it } from 'vitest'; import { RemixTestEnv, assertSentryEvent, assertSentryTransaction } from './utils/helpers'; const useV2 = process.env.REMIX_VERSION === '2'; describe('Server Side Rendering', () => { it('correctly reports a server side rendering error', async () => { - const env = await RemixTestEnv.init('builtin'); + const env = await RemixTestEnv.init(); const url = `${env.url}/ssr-error`; const envelopes = await env.getMultipleEnvelopeRequest({ url, count: 2, envelopeType: ['transaction', 'event'] }); const [transaction] = envelopes.filter(envelope => envelope[1].type === 'transaction'); @@ -28,7 +29,6 @@ describe('Server Side Rendering', () => { }); assertSentryEvent(event[2], { - transaction: 'routes/ssr-error', exception: { values: [ { diff --git a/packages/remix/test/integration/test/server/utils/helpers.ts b/packages/remix/test/integration/test/server/utils/helpers.ts index caf9d5525fd7..eccda209fb48 100644 --- a/packages/remix/test/integration/test/server/utils/helpers.ts +++ b/packages/remix/test/integration/test/server/utils/helpers.ts @@ -1,7 +1,6 @@ import * as http from 'http'; import { AddressInfo } from 'net'; import { createRequestHandler } from '@remix-run/express'; -import { wrapExpressCreateRequestHandler } from '@sentry/remix'; import express from 'express'; import { TestEnv } from '../../../../../../../dev-packages/node-integration-tests/utils'; @@ -12,15 +11,12 @@ export class RemixTestEnv extends TestEnv { super(server, url); } - public static async init(adapter: string = 'builtin'): Promise { - const requestHandlerFactory = - adapter === 'express' ? wrapExpressCreateRequestHandler(createRequestHandler) : createRequestHandler; - + public static async init(): Promise { let serverPort; const server = await new Promise(resolve => { const app = express(); - app.all('*', requestHandlerFactory({ build: require('../../../build') })); + app.all('*', createRequestHandler({ build: require('../../../build') })); const server = app.listen(0, () => { serverPort = (server.address() as AddressInfo).port; diff --git a/packages/remix/test/integration/tsconfig.test.json b/packages/remix/test/integration/tsconfig.test.json index d3175b6a1b01..8ce7525d33fd 100644 --- a/packages/remix/test/integration/tsconfig.test.json +++ b/packages/remix/test/integration/tsconfig.test.json @@ -4,6 +4,6 @@ "include": ["test/**/*"], "compilerOptions": { - "types": ["node", "jest"] + "types": ["node", "vitest/globals"] } } diff --git a/packages/remix/vitest.config.ts b/packages/remix/vitest.config.ts new file mode 100644 index 000000000000..5240b036b083 --- /dev/null +++ b/packages/remix/vitest.config.ts @@ -0,0 +1,8 @@ +import { defineConfig } from 'vitest/config'; + +export default defineConfig({ + test: { + globals: true, + setupFiles: './test/integration/instrument.server.cjs', + }, +}); diff --git a/yarn.lock b/yarn.lock index e83c8fe9f310..f4d66c26b8dc 100644 --- a/yarn.lock +++ b/yarn.lock @@ -24190,6 +24190,14 @@ opentelemetry-instrumentation-fetch-node@1.2.0: "@opentelemetry/instrumentation" "^0.43.0" "@opentelemetry/semantic-conventions" "^1.17.0" +opentelemetry-instrumentation-remix@0.7.0: + version "0.7.0" + resolved "https://registry.yarnpkg.com/opentelemetry-instrumentation-remix/-/opentelemetry-instrumentation-remix-0.7.0.tgz#ea3ac4d6da69300de1417c938eade2d029c5cd21" + integrity sha512-dF7IdcLkN2xIUATaa2X4ahb/Plk/c2wPdOz90MCVgFHuQZvGtzP9DwBpxXEzs6dz4f57ZzJsHpwJvAXHCSJrbg== + dependencies: + "@opentelemetry/instrumentation" "^0.43.0" + "@opentelemetry/semantic-conventions" "^1.17.0" + optional-require@1.0.x: version "1.0.3" resolved "https://registry.yarnpkg.com/optional-require/-/optional-require-1.0.3.tgz#275b8e9df1dc6a17ad155369c2422a440f89cb07" @@ -25565,7 +25573,7 @@ postcss@^8.2.14, postcss@^8.4.7, postcss@^8.4.8: picocolors "^1.0.0" source-map-js "^1.1.0" -postcss@^8.4.23, postcss@^8.4.36: +postcss@^8.4.23, postcss@^8.4.36, postcss@^8.4.38: version "8.4.38" resolved "https://registry.yarnpkg.com/postcss/-/postcss-8.4.38.tgz#b387d533baf2054288e337066d81c6bee9db9e0e" integrity sha512-Wglpdk03BSfXkHoQa3b/oulrotAkwrlLDRSOb9D0bN86FdRyE9lppSp33aHNPgBa0JKCoB+drFLZkQoRRYae5A== @@ -30634,6 +30642,17 @@ vite@^5.0.10: optionalDependencies: fsevents "~2.3.3" +vite@^5.2.11: + version "5.2.11" + resolved "https://registry.yarnpkg.com/vite/-/vite-5.2.11.tgz#726ec05555431735853417c3c0bfb36003ca0cbd" + integrity sha512-HndV31LWW05i1BLPMUCE1B9E9GFbOu1MbenhS58FuK6owSO5qHm7GiCotrNY1YE5rMeQSFBGmT5ZaLEjFizgiQ== + dependencies: + esbuild "^0.20.1" + postcss "^8.4.38" + rollup "^4.13.0" + optionalDependencies: + fsevents "~2.3.3" + vitefu@^0.2.2, vitefu@^0.2.5: version "0.2.5" resolved "https://registry.yarnpkg.com/vitefu/-/vitefu-0.2.5.tgz#c1b93c377fbdd3e5ddd69840ea3aa70b40d90969" From f3b8ec806d2ed7be3a80901a996831549dd2d834 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Fri, 17 May 2024 16:02:07 +0100 Subject: [PATCH 02/27] Fix linter. --- packages/remix/.eslintrc.js | 2 +- packages/remix/tsconfig.test.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/remix/.eslintrc.js b/packages/remix/.eslintrc.js index 4195984e1745..992bcd3e9d2b 100644 --- a/packages/remix/.eslintrc.js +++ b/packages/remix/.eslintrc.js @@ -6,7 +6,7 @@ module.exports = { parserOptions: { jsx: true, }, - ignorePatterns: ['playwright.config.ts', 'test/integration/**'], + ignorePatterns: ['playwright.config.ts', 'vitest.config.ts', 'test/integration/**'], extends: ['../../.eslintrc.js'], rules: { '@sentry-internal/sdk/no-optional-chaining': 'off', diff --git a/packages/remix/tsconfig.test.json b/packages/remix/tsconfig.test.json index 7aa20c05d60c..ffcc2b26016c 100644 --- a/packages/remix/tsconfig.test.json +++ b/packages/remix/tsconfig.test.json @@ -1,7 +1,7 @@ { "extends": "./tsconfig.json", - "include": ["test/**/*"], + "include": ["test/**/*", "vitest.config.ts"], "compilerOptions": { "types": ["node", "jest"], From ca48da01247dd751865a7b255376bc79b5df02fd Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Fri, 17 May 2024 16:10:07 +0100 Subject: [PATCH 03/27] Return jest config for unit tests back. --- packages/remix/jest.config.js | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 packages/remix/jest.config.js diff --git a/packages/remix/jest.config.js b/packages/remix/jest.config.js new file mode 100644 index 000000000000..7d6c8cb0990e --- /dev/null +++ b/packages/remix/jest.config.js @@ -0,0 +1,8 @@ +const baseConfig = require('../../jest/jest.config.js'); + +module.exports = { + ...baseConfig, + testPathIgnorePatterns: ['/build/', '/node_modules/', '/test/integration/'], + // Some tests take longer to finish, as flushing spans with OpenTelemetry takes some more time + testTimeout: 15000, +}; From 07550da68def3576cea75a492309dfaadcd1ee41 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Fri, 17 May 2024 16:27:29 +0100 Subject: [PATCH 04/27] Don't run Remix integration tests on Node 14 --- .github/workflows/build.yml | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 53b8f9e32e52..6f1159480d2d 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -912,10 +912,8 @@ jobs: matrix: node: [18, 20, 22] remix: [1, 2] - # Remix v2 only supports Node 18+, so run Node 14, 16 tests separately + # Remix v2 only supports Node 18+, so run 16 tests separately include: - - node: 14 - remix: 1 - node: 16 remix: 1 steps: From f1e5c0ce4fa1ced16f9196a1a47f9b1d37b50200 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Fri, 17 May 2024 17:17:50 +0100 Subject: [PATCH 05/27] Instrument function builds properly. --- .../tests/behaviour-server.test.ts | 9 ++++-- packages/remix/src/utils/instrumentServer.ts | 29 ++++++++++--------- 2 files changed, 21 insertions(+), 17 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/tests/behaviour-server.test.ts b/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/tests/behaviour-server.test.ts index c2e2873f60c6..1906113ba768 100644 --- a/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/tests/behaviour-server.test.ts +++ b/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/tests/behaviour-server.test.ts @@ -10,7 +10,7 @@ test('Sends two linked transactions (server & client) to Sentry', async ({ page const httpServerTransactionPromise = waitForTransaction('create-remix-app-express-vite-dev', transactionEvent => { return ( transactionEvent.type === 'transaction' && - transactionEvent.contexts?.trace?.op === 'http.server' && + transactionEvent.contexts?.trace?.op === 'http' && transactionEvent.tags?.['sentry_test'] === testTag ); }); @@ -33,18 +33,21 @@ test('Sends two linked transactions (server & client) to Sentry', async ({ page const httpServerTraceId = httpServerTransaction.contexts?.trace?.trace_id; const httpServerSpanId = httpServerTransaction.contexts?.trace?.span_id; + const loaderSpanId = httpServerTransaction.spans.find( + span => span.data && span.data['code.function'] === 'loader', + )?.span_id; const pageLoadTraceId = pageloadTransaction.contexts?.trace?.trace_id; const pageLoadSpanId = pageloadTransaction.contexts?.trace?.span_id; const pageLoadParentSpanId = pageloadTransaction.contexts?.trace?.parent_span_id; - expect(httpServerTransaction.transaction).toBe('routes/_index'); + expect(httpServerTransaction.transaction).toBe('GET http://localhost:3030/'); expect(pageloadTransaction.transaction).toBe('routes/_index'); expect(httpServerTraceId).toBeDefined(); expect(httpServerSpanId).toBeDefined(); expect(pageLoadTraceId).toEqual(httpServerTraceId); - expect(pageLoadParentSpanId).toEqual(httpServerSpanId); + expect(pageLoadParentSpanId).toEqual(loaderSpanId); expect(pageLoadSpanId).not.toEqual(httpServerSpanId); }); diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index 812f872838b7..c8fc8993cc28 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -17,7 +17,6 @@ import { fill, isNodeEnv, isPrimitive, - isThenable, loadModule, logger, objectify, @@ -416,25 +415,27 @@ function instrumentBuildCallback(build: ServerBuild): ServerBuild { */ export function instrumentBuild( build: ServerBuild | (() => ServerBuild | Promise), -): ServerBuild | Promise { +): ServerBuild | (() => ServerBuild | Promise) { if (typeof build === 'function') { - const resolvedBuild = build(); + return function () { + const resolvedBuild = build(); - if (isThenable(resolvedBuild)) { - return resolvedBuild.then(build => { - FUTURE_FLAGS = getFutureFlagsServer(build); + if (resolvedBuild instanceof Promise) { + return resolvedBuild.then(build => { + FUTURE_FLAGS = getFutureFlagsServer(build); - return instrumentBuildCallback(build); - }); - } else { - FUTURE_FLAGS = getFutureFlagsServer(resolvedBuild); + return instrumentBuildCallback(build); + }); + } else { + FUTURE_FLAGS = getFutureFlagsServer(resolvedBuild); - return instrumentBuildCallback(resolvedBuild); - } + return instrumentBuildCallback(resolvedBuild); + } + }; } else { - FUTURE_FLAGS = getFutureFlagsServer(build as ServerBuild); + FUTURE_FLAGS = getFutureFlagsServer(build); - return instrumentBuildCallback(build as ServerBuild); + return instrumentBuildCallback(build); } } From ce315d3b4424e3f3039c8224840ee4c301920b2d Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Mon, 20 May 2024 11:58:34 +0100 Subject: [PATCH 06/27] Add ErrorBoundary propagation test. --- .../app/routes/client-error.tsx | 11 +++++ .../tests/behaviour-server.test.ts | 49 +++++++++++++++++++ 2 files changed, 60 insertions(+) diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-express/app/routes/client-error.tsx b/dev-packages/e2e-tests/test-applications/create-remix-app-express/app/routes/client-error.tsx index 4e5330621191..f4d6ec9c4f0a 100644 --- a/dev-packages/e2e-tests/test-applications/create-remix-app-express/app/routes/client-error.tsx +++ b/dev-packages/e2e-tests/test-applications/create-remix-app-express/app/routes/client-error.tsx @@ -1,6 +1,17 @@ +import { useSearchParams } from '@remix-run/react'; +import * as Sentry from '@sentry/remix'; + import { useState } from 'react'; export default function ErrorBoundaryCapture() { + const [searchParams] = useSearchParams(); + + if (searchParams.get('tag')) { + Sentry.setTags({ + sentry_test: searchParams.get('tag'), + }); + } + const [count, setCount] = useState(0); if (count > 0) { diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-express/tests/behaviour-server.test.ts b/dev-packages/e2e-tests/test-applications/create-remix-app-express/tests/behaviour-server.test.ts index 7f41f6ea8880..b928aa5bce2c 100644 --- a/dev-packages/e2e-tests/test-applications/create-remix-app-express/tests/behaviour-server.test.ts +++ b/dev-packages/e2e-tests/test-applications/create-remix-app-express/tests/behaviour-server.test.ts @@ -105,3 +105,52 @@ test('Sends two linked transactions (server & client) to Sentry', async ({ page expect(pageLoadParentSpanId).toEqual(loaderSpanId); expect(pageLoadSpanId).not.toEqual(httpServerSpanId); }); + +test('Propagates trace when ErrorBoundary is triggered', async ({ page }) => { + // We use this to identify the transactions + const testTag = uuid4(); + + const httpServerTransactionPromise = waitForTransaction('create-remix-app-express', transactionEvent => { + return ( + transactionEvent.type === 'transaction' && + transactionEvent.contexts?.trace?.op === 'http' && + transactionEvent.tags?.['sentry_test'] === testTag + ); + }); + + const pageLoadTransactionPromise = waitForTransaction('create-remix-app-express', transactionEvent => { + return ( + transactionEvent.type === 'transaction' && + transactionEvent.contexts?.trace?.op === 'pageload' && + transactionEvent.tags?.['sentry_test'] === testTag + ); + }); + + page.goto(`/client-error?tag=${testTag}`); + + const pageloadTransaction = await pageLoadTransactionPromise; + const httpServerTransaction = await httpServerTransactionPromise; + + expect(pageloadTransaction).toBeDefined(); + expect(httpServerTransaction).toBeDefined(); + + const httpServerTraceId = httpServerTransaction.contexts?.trace?.trace_id; + const httpServerSpanId = httpServerTransaction.contexts?.trace?.span_id; + const loaderSpanId = httpServerTransaction.spans.find( + span => span.data && span.data['code.function'] === 'loader', + )?.span_id; + + const pageLoadTraceId = pageloadTransaction.contexts?.trace?.trace_id; + const pageLoadSpanId = pageloadTransaction.contexts?.trace?.span_id; + const pageLoadParentSpanId = pageloadTransaction.contexts?.trace?.parent_span_id; + + expect(httpServerTransaction.transaction).toBe('GET client-error'); + expect(pageloadTransaction.transaction).toBe('routes/client-error'); + + expect(httpServerTraceId).toBeDefined(); + expect(httpServerSpanId).toBeDefined(); + + expect(pageLoadTraceId).toEqual(httpServerTraceId); + expect(pageLoadParentSpanId).toEqual(loaderSpanId); + expect(pageLoadSpanId).not.toEqual(httpServerSpanId); +}); From 4ec9e3aa84a6019f5bf289fe71ac788b5fe2fda2 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Tue, 21 May 2024 10:16:55 +0100 Subject: [PATCH 07/27] Export a no-op function in place of `wrapExpressCreateRequestHandler`. --- packages/remix/src/index.server.ts | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/packages/remix/src/index.server.ts b/packages/remix/src/index.server.ts index d32a20c4281d..dc2c2808ad0a 100644 --- a/packages/remix/src/index.server.ts +++ b/packages/remix/src/index.server.ts @@ -140,6 +140,20 @@ export function getDefaultIntegrations(options: RemixOptions): Integration[] { ]; } +/** + * Returns the given Express createRequestHandler function. + * This function is no-op and only returns the given function. + * + * @deprecated No need to wrap the Express request handler. + * @param createRequestHandlerFn The Remix Express `createRequestHandler`. + * @returns `createRequestHandler` function. + */ +export function wrapExpressCreateRequestHandler(createRequestHandlerFn: unknown): unknown { + DEBUG_BUILD && logger.warn('wrapExpressCreateRequestHandler is deprecated and no longer needed.'); + + return createRequestHandlerFn; +} + /** Initializes Sentry Remix SDK on Node. */ export function init(options: RemixOptions): void { applySdkMetadata(options, 'remix', ['remix', 'node']); From 8419b7ca7e953af88f1bde849951e4e287fc195c Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Fri, 24 May 2024 11:42:10 -0400 Subject: [PATCH 08/27] Add a custom `httpIntegration` for Remix. --- .../tests/behaviour-server.test.ts | 2 +- .../tests/behaviour-server.test.ts | 24 +++++------ .../tests/behaviour-server.test.ts | 2 +- .../tests/behaviour-server.test.ts | 2 +- packages/remix/package.json | 1 + packages/remix/src/index.server.ts | 2 + packages/remix/src/utils/integrations/http.ts | 42 +++++++++++++++++++ .../src/utils/integrations/opentelemetry.ts | 33 ++++++++++++++- .../integration/test/server/action.test.ts | 22 +++++----- .../integration/test/server/loader.test.ts | 12 +++--- 10 files changed, 108 insertions(+), 34 deletions(-) create mode 100644 packages/remix/src/utils/integrations/http.ts diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/tests/behaviour-server.test.ts b/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/tests/behaviour-server.test.ts index 1906113ba768..f638141dcf57 100644 --- a/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/tests/behaviour-server.test.ts +++ b/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/tests/behaviour-server.test.ts @@ -10,7 +10,7 @@ test('Sends two linked transactions (server & client) to Sentry', async ({ page const httpServerTransactionPromise = waitForTransaction('create-remix-app-express-vite-dev', transactionEvent => { return ( transactionEvent.type === 'transaction' && - transactionEvent.contexts?.trace?.op === 'http' && + transactionEvent.contexts?.trace?.op === 'http.server' && transactionEvent.tags?.['sentry_test'] === testTag ); }); diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-express/tests/behaviour-server.test.ts b/dev-packages/e2e-tests/test-applications/create-remix-app-express/tests/behaviour-server.test.ts index b928aa5bce2c..236e37d1757a 100644 --- a/dev-packages/e2e-tests/test-applications/create-remix-app-express/tests/behaviour-server.test.ts +++ b/dev-packages/e2e-tests/test-applications/create-remix-app-express/tests/behaviour-server.test.ts @@ -35,7 +35,7 @@ test('Sends form data with action error to Sentry', async ({ page }) => { ); expect(actionSpan).toBeDefined(); - expect(actionSpan.op).toBe('http'); + expect(actionSpan.op).toBe('action.remix'); expect(actionSpan.data).toMatchObject({ 'formData.text': 'test', 'formData.file': 'file.txt', @@ -54,17 +54,17 @@ test('Sends a loader span to Sentry', async ({ page }) => { ); expect(loaderSpan).toBeDefined(); - expect(loaderSpan.op).toBe('http'); + expect(loaderSpan.op).toBe('loader.remix'); }); -test('Sends two linked transactions (server & client) to Sentry', async ({ page }) => { +test('Propagates trace when ErrorBoundary is triggered', async ({ page }) => { // We use this to identify the transactions const testTag = uuid4(); const httpServerTransactionPromise = waitForTransaction('create-remix-app-express', transactionEvent => { return ( transactionEvent.type === 'transaction' && - transactionEvent.contexts?.trace?.op === 'http' && + transactionEvent.contexts?.trace?.op === 'http.server' && transactionEvent.tags?.['sentry_test'] === testTag ); }); @@ -77,7 +77,7 @@ test('Sends two linked transactions (server & client) to Sentry', async ({ page ); }); - page.goto(`/?tag=${testTag}`); + page.goto(`/client-error?tag=${testTag}`); const pageloadTransaction = await pageLoadTransactionPromise; const httpServerTransaction = await httpServerTransactionPromise; @@ -95,8 +95,8 @@ test('Sends two linked transactions (server & client) to Sentry', async ({ page const pageLoadSpanId = pageloadTransaction.contexts?.trace?.span_id; const pageLoadParentSpanId = pageloadTransaction.contexts?.trace?.parent_span_id; - expect(httpServerTransaction.transaction).toBe('GET http://localhost:3030/'); - expect(pageloadTransaction.transaction).toBe('routes/_index'); + expect(httpServerTransaction.transaction).toBe('GET client-error'); + expect(pageloadTransaction.transaction).toBe('routes/client-error'); expect(httpServerTraceId).toBeDefined(); expect(httpServerSpanId).toBeDefined(); @@ -106,14 +106,14 @@ test('Sends two linked transactions (server & client) to Sentry', async ({ page expect(pageLoadSpanId).not.toEqual(httpServerSpanId); }); -test('Propagates trace when ErrorBoundary is triggered', async ({ page }) => { +test('Sends two linked transactions (server & client) to Sentry', async ({ page }) => { // We use this to identify the transactions const testTag = uuid4(); const httpServerTransactionPromise = waitForTransaction('create-remix-app-express', transactionEvent => { return ( transactionEvent.type === 'transaction' && - transactionEvent.contexts?.trace?.op === 'http' && + transactionEvent.contexts?.trace?.op === 'http.server' && transactionEvent.tags?.['sentry_test'] === testTag ); }); @@ -126,7 +126,7 @@ test('Propagates trace when ErrorBoundary is triggered', async ({ page }) => { ); }); - page.goto(`/client-error?tag=${testTag}`); + page.goto(`/?tag=${testTag}`); const pageloadTransaction = await pageLoadTransactionPromise; const httpServerTransaction = await httpServerTransactionPromise; @@ -144,8 +144,8 @@ test('Propagates trace when ErrorBoundary is triggered', async ({ page }) => { const pageLoadSpanId = pageloadTransaction.contexts?.trace?.span_id; const pageLoadParentSpanId = pageloadTransaction.contexts?.trace?.parent_span_id; - expect(httpServerTransaction.transaction).toBe('GET client-error'); - expect(pageloadTransaction.transaction).toBe('routes/client-error'); + expect(httpServerTransaction.transaction).toBe('GET http://localhost:3030/'); + expect(pageloadTransaction.transaction).toBe('routes/_index'); expect(httpServerTraceId).toBeDefined(); expect(httpServerSpanId).toBeDefined(); diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-v2/tests/behaviour-server.test.ts b/dev-packages/e2e-tests/test-applications/create-remix-app-v2/tests/behaviour-server.test.ts index 42219f9dd8d2..ea95b97fa611 100644 --- a/dev-packages/e2e-tests/test-applications/create-remix-app-v2/tests/behaviour-server.test.ts +++ b/dev-packages/e2e-tests/test-applications/create-remix-app-v2/tests/behaviour-server.test.ts @@ -10,7 +10,7 @@ test('Sends two linked transactions (server & client) to Sentry', async ({ page const httpServerTransactionPromise = waitForTransaction('create-remix-app-v2', transactionEvent => { return ( transactionEvent.type === 'transaction' && - transactionEvent.contexts?.trace?.op === 'http' && + transactionEvent.contexts?.trace?.op === 'http.server' && transactionEvent.tags?.['sentry_test'] === testTag ); }); diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app/tests/behaviour-server.test.ts b/dev-packages/e2e-tests/test-applications/create-remix-app/tests/behaviour-server.test.ts index 837564c9d91d..45f24ad9d18b 100644 --- a/dev-packages/e2e-tests/test-applications/create-remix-app/tests/behaviour-server.test.ts +++ b/dev-packages/e2e-tests/test-applications/create-remix-app/tests/behaviour-server.test.ts @@ -10,7 +10,7 @@ test('Sends two linked transactions (server & client) to Sentry', async ({ page const httpServerTransactionPromise = waitForTransaction('create-remix-app', transactionEvent => { return ( transactionEvent.type === 'transaction' && - transactionEvent.contexts?.trace?.op === 'http' && + transactionEvent.contexts?.trace?.op === 'http.server' && transactionEvent.tags?.['sentry_test'] === testTag ); }); diff --git a/packages/remix/package.json b/packages/remix/package.json index c5d6646e3fbf..242ca87baeff 100644 --- a/packages/remix/package.json +++ b/packages/remix/package.json @@ -57,6 +57,7 @@ "access": "public" }, "dependencies": { + "@opentelemetry/instrumentation-http": "0.51.1", "@remix-run/router": "1.x", "@sentry/cli": "^2.32.1", "@sentry/core": "8.9.2", diff --git a/packages/remix/src/index.server.ts b/packages/remix/src/index.server.ts index dc2c2808ad0a..2d41b401db4c 100644 --- a/packages/remix/src/index.server.ts +++ b/packages/remix/src/index.server.ts @@ -11,6 +11,7 @@ import { logger } from '@sentry/utils'; import { DEBUG_BUILD } from './utils/debug-build'; import { instrumentServer } from './utils/instrumentServer'; +import { httpIntegration } from './utils/integrations/http'; import { remixIntegration } from './utils/integrations/opentelemetry'; import type { RemixOptions } from './utils/remixOptions'; @@ -136,6 +137,7 @@ export type { SentryMetaArgs } from './utils/types'; export function getDefaultIntegrations(options: RemixOptions): Integration[] { return [ ...getDefaultNodeIntegrations(options).filter(integration => integration.name !== 'Http'), + httpIntegration(), remixIntegration(options), ]; } diff --git a/packages/remix/src/utils/integrations/http.ts b/packages/remix/src/utils/integrations/http.ts new file mode 100644 index 000000000000..7c4b80f44fe7 --- /dev/null +++ b/packages/remix/src/utils/integrations/http.ts @@ -0,0 +1,42 @@ +// This integration is ported from the Next.JS SDK. +import { HttpInstrumentation } from '@opentelemetry/instrumentation-http'; +import { httpIntegration as originalHttpIntegration } from '@sentry/node'; +import type { IntegrationFn } from '@sentry/types'; + +class RemixHttpIntegration extends HttpInstrumentation { + // Instead of the default behavior, we just don't do any wrapping for incoming requests + protected _getPatchIncomingRequestFunction(_component: 'http' | 'https') { + return ( + original: (event: string, ...args: unknown[]) => boolean, + ): ((this: unknown, event: string, ...args: unknown[]) => boolean) => { + return function incomingRequest(this: unknown, event: string, ...args: unknown[]): boolean { + return original.apply(this, [event, ...args]); + }; + }; + } +} + +interface HttpOptions { + /** + * Whether breadcrumbs should be recorded for requests. + * Defaults to true + */ + breadcrumbs?: boolean; + + /** + * Do not capture spans or breadcrumbs for outgoing HTTP requests to URLs where the given callback returns `true`. + * This controls both span & breadcrumb creation - spans will be non recording if tracing is disabled. + */ + ignoreOutgoingRequests?: (url: string) => boolean; +} + +/** + * The http integration instruments Node's internal http and https modules. + * It creates breadcrumbs and spans for outgoing HTTP requests which will be attached to the currently active span. + */ +export const httpIntegration = ((options: HttpOptions = {}) => { + return originalHttpIntegration({ + ...options, + _instrumentation: RemixHttpIntegration, + }); +}) satisfies IntegrationFn; diff --git a/packages/remix/src/utils/integrations/opentelemetry.ts b/packages/remix/src/utils/integrations/opentelemetry.ts index 92b000416608..e06ca10f0ccd 100644 --- a/packages/remix/src/utils/integrations/opentelemetry.ts +++ b/packages/remix/src/utils/integrations/opentelemetry.ts @@ -1,8 +1,8 @@ import { RemixInstrumentation } from 'opentelemetry-instrumentation-remix'; import { defineIntegration } from '@sentry/core'; -import { addOpenTelemetryInstrumentation } from '@sentry/node'; -import type { IntegrationFn } from '@sentry/types'; +import { SEMANTIC_ATTRIBUTE_SENTRY_OP, addOpenTelemetryInstrumentation, spanToJSON } from '@sentry/node'; +import type { Client, IntegrationFn, Span } from '@sentry/types'; import type { RemixOptions } from '../remixOptions'; const _remixIntegration = ((options?: RemixOptions) => { @@ -15,9 +15,38 @@ const _remixIntegration = ((options?: RemixOptions) => { }), ); }, + + setup(client: Client) { + client.on('spanStart', span => { + addRemixSpanAttributes(span); + }); + }, }; }) satisfies IntegrationFn; +const addRemixSpanAttributes = (span: Span): void => { + const attributes = spanToJSON(span).data || {}; + + // this is one of: loader, action, requestHandler + const type = attributes['code.function']; + + // If this is already set, or we have no remix span, no need to process again... + if (attributes[SEMANTIC_ATTRIBUTE_SENTRY_OP] || !type) { + return; + } + + // `requestHandler` span from `opentelemetry-instrumentation-remix` is the main server span. + // It should be marked as the `http.server` operation. + // The incoming requests are skipped by the custom `RemixHttpIntegration` package. + if (type === 'requestHandler') { + span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_OP, 'http.server'); + return; + } + + // All other spans are marked as `remix` operations with their specific type [loader, action] + span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_OP, `${type}.remix`); +}; + /** * Instrumentation for aws-sdk package */ diff --git a/packages/remix/test/integration/test/server/action.test.ts b/packages/remix/test/integration/test/server/action.test.ts index 65f04cd3ebf3..940e54abd037 100644 --- a/packages/remix/test/integration/test/server/action.test.ts +++ b/packages/remix/test/integration/test/server/action.test.ts @@ -21,7 +21,7 @@ describe('Remix API Actions', () => { { data: { 'code.function': 'action', - 'sentry.op': 'http', + 'sentry.op': 'action.remix', 'otel.kind': 'INTERNAL', 'match.route.id': `routes/action-json-response${useV2 ? '.' : '/'}$id`, 'match.params.id': '123123', @@ -30,7 +30,7 @@ describe('Remix API Actions', () => { { data: { 'code.function': 'loader', - 'sentry.op': 'http', + 'sentry.op': 'loader.remix', 'otel.kind': 'INTERNAL', 'match.route.id': `routes/action-json-response${useV2 ? '.' : '/'}$id`, 'match.params.id': '123123', @@ -39,7 +39,7 @@ describe('Remix API Actions', () => { { data: { 'code.function': 'loader', - 'sentry.op': 'http', + 'sentry.op': 'loader.remix', 'otel.kind': 'INTERNAL', 'match.route.id': 'root', 'match.params.id': '123123', @@ -168,7 +168,7 @@ describe('Remix API Actions', () => { assertSentryTransaction(transaction_1[2], { contexts: { trace: { - op: 'http', + op: 'http.server', status: 'ok', data: { 'http.response.status_code': 302, @@ -181,7 +181,7 @@ describe('Remix API Actions', () => { assertSentryTransaction(transaction_2[2], { contexts: { trace: { - op: 'http', + op: 'http.server', status: 'internal_error', data: { 'http.response.status_code': 500, @@ -228,7 +228,7 @@ describe('Remix API Actions', () => { assertSentryTransaction(transaction[2], { contexts: { trace: { - op: 'http', + op: 'http.server', status: 'internal_error', data: { 'http.response.status_code': 500, @@ -275,7 +275,7 @@ describe('Remix API Actions', () => { assertSentryTransaction(transaction[2], { contexts: { trace: { - op: 'http', + op: 'http.server', status: 'internal_error', data: { 'http.response.status_code': 500, @@ -322,7 +322,7 @@ describe('Remix API Actions', () => { assertSentryTransaction(transaction[2], { contexts: { trace: { - op: 'http', + op: 'http.server', status: 'internal_error', data: { 'http.response.status_code': 500, @@ -369,7 +369,7 @@ describe('Remix API Actions', () => { assertSentryTransaction(transaction[2], { contexts: { trace: { - op: 'http', + op: 'http.server', status: 'internal_error', data: { 'http.response.status_code': 500, @@ -416,7 +416,7 @@ describe('Remix API Actions', () => { assertSentryTransaction(transaction[2], { contexts: { trace: { - op: 'http', + op: 'http.server', status: 'internal_error', data: { 'http.response.status_code': 500, @@ -463,7 +463,7 @@ describe('Remix API Actions', () => { assertSentryTransaction(transaction[2], { contexts: { trace: { - op: 'http', + op: 'http.server', status: 'internal_error', data: { 'http.response.status_code': 500, diff --git a/packages/remix/test/integration/test/server/loader.test.ts b/packages/remix/test/integration/test/server/loader.test.ts index 162b656bfe4f..df701da9c782 100644 --- a/packages/remix/test/integration/test/server/loader.test.ts +++ b/packages/remix/test/integration/test/server/loader.test.ts @@ -103,7 +103,7 @@ describe('Remix API Loaders', () => { data: { 'code.function': 'loader', 'otel.kind': 'INTERNAL', - 'sentry.op': 'http', + 'sentry.op': 'loader.remix', }, origin: 'manual', }, @@ -111,7 +111,7 @@ describe('Remix API Loaders', () => { data: { 'code.function': 'loader', 'otel.kind': 'INTERNAL', - 'sentry.op': 'http', + 'sentry.op': 'loader.remix', }, origin: 'manual', }, @@ -135,7 +135,7 @@ describe('Remix API Loaders', () => { assertSentryTransaction(transaction_1[2], { contexts: { trace: { - op: 'http', + op: 'http.server', status: 'ok', data: { 'http.response.status_code': 302, @@ -148,7 +148,7 @@ describe('Remix API Loaders', () => { assertSentryTransaction(transaction_2[2], { contexts: { trace: { - op: 'http', + op: 'http.server', status: 'internal_error', data: { 'http.response.status_code': 500, @@ -243,7 +243,7 @@ describe('Remix API Loaders', () => { { data: { 'code.function': 'loader', - 'sentry.op': 'http', + 'sentry.op': 'loader.remix', 'otel.kind': 'INTERNAL', 'match.route.id': `routes/loader-defer-response${useV2 ? '.' : '/'}$id`, }, @@ -251,7 +251,7 @@ describe('Remix API Loaders', () => { { data: { 'code.function': 'loader', - 'sentry.op': 'http', + 'sentry.op': 'loader.remix', 'otel.kind': 'INTERNAL', 'match.route.id': 'root', }, From 21d606dc364dbf765a295f5c662ad6bd2fa77d08 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Wed, 29 May 2024 13:35:22 -0400 Subject: [PATCH 09/27] Switch to `import` pattern to load instrumentation on Express tests --- .../{instrument.server.mjs => instrument.mjs} | 0 .../create-remix-app-express-vite-dev/package.json | 2 +- .../create-remix-app-express-vite-dev/server.mjs | 2 ++ .../test-applications/create-remix-app-express/package.json | 2 +- .../test-applications/create-remix-app-express/server.mjs | 2 ++ 5 files changed, 6 insertions(+), 2 deletions(-) rename dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/{instrument.server.mjs => instrument.mjs} (100%) diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/instrument.server.mjs b/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/instrument.mjs similarity index 100% rename from dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/instrument.server.mjs rename to dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/instrument.mjs diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/package.json b/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/package.json index f939dd15ca8c..5ba4b54d8e8f 100644 --- a/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/package.json +++ b/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/package.json @@ -3,7 +3,7 @@ "sideEffects": false, "scripts": { "build": "remix vite:build", - "dev": "NODE_OPTIONS='--import=./instrument.server.mjs' node ./server.mjs", + "dev": "node ./server.mjs", "lint": "eslint --ignore-path .gitignore --cache --cache-location ./node_modules/.cache/eslint .", "start": "cross-env NODE_ENV=production node ./server.mjs", "typecheck": "tsc", diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/server.mjs b/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/server.mjs index dcd0ef1c5cbc..a3ddf0a15424 100644 --- a/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/server.mjs +++ b/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/server.mjs @@ -1,3 +1,5 @@ +import './instrument.mjs'; + import { createRequestHandler } from '@remix-run/express'; import { installGlobals } from '@remix-run/node'; import compression from 'compression'; diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-express/package.json b/dev-packages/e2e-tests/test-applications/create-remix-app-express/package.json index cce75b6d592e..53ccbdff9e7f 100644 --- a/dev-packages/e2e-tests/test-applications/create-remix-app-express/package.json +++ b/dev-packages/e2e-tests/test-applications/create-remix-app-express/package.json @@ -6,7 +6,7 @@ "build": "remix vite:build", "dev": "node ./server.mjs", "lint": "eslint --ignore-path .gitignore --cache --cache-location ./node_modules/.cache/eslint .", - "start": "cross-env NODE_ENV=production node --require='./instrument.cjs' ./server.mjs", + "start": "cross-env NODE_ENV=production node ./server.mjs", "typecheck": "tsc", "clean": "npx rimraf node_modules pnpm-lock.yaml", "test:build": "pnpm install && npx playwright install && pnpm build", diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-express/server.mjs b/dev-packages/e2e-tests/test-applications/create-remix-app-express/server.mjs index dcd0ef1c5cbc..eb6078bf0321 100644 --- a/dev-packages/e2e-tests/test-applications/create-remix-app-express/server.mjs +++ b/dev-packages/e2e-tests/test-applications/create-remix-app-express/server.mjs @@ -1,3 +1,5 @@ +import './instrument.cjs'; + import { createRequestHandler } from '@remix-run/express'; import { installGlobals } from '@remix-run/node'; import compression from 'compression'; From 4da9be8959d8c68e4d95b07b279d5ffc1241718a Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Wed, 29 May 2024 19:28:21 -0400 Subject: [PATCH 10/27] Remove extra `Sentry.init`s in v2 e2e test app. --- .../create-remix-app-v2/app/entry.server.tsx | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-v2/app/entry.server.tsx b/dev-packages/e2e-tests/test-applications/create-remix-app-v2/app/entry.server.tsx index c49e814246a8..0529e2417e48 100644 --- a/dev-packages/e2e-tests/test-applications/create-remix-app-v2/app/entry.server.tsx +++ b/dev-packages/e2e-tests/test-applications/create-remix-app-v2/app/entry.server.tsx @@ -1,12 +1,5 @@ import * as Sentry from '@sentry/remix'; -Sentry.init({ - tracesSampleRate: 1.0, // Capture 100% of the transactions, reduce in production! - environment: 'qa', // dynamic sampling bias to keep transactions - dsn: process.env.E2E_TEST_DSN, - tunnel: 'http://localhost:3031/', // proxy server -}); - /** * By default, Remix will handle generating the HTTP Response for you. * You are free to delete this file if you'd like to, but if you ever want it revealed again, you can run `npx remix reveal` ✨ @@ -26,13 +19,6 @@ installGlobals(); const ABORT_DELAY = 5_000; -Sentry.init({ - environment: 'qa', // dynamic sampling bias to keep transactions - dsn: process.env.E2E_TEST_DSN, - // Performance Monitoring - tracesSampleRate: 1.0, // Capture 100% of the transactions, reduce in production! -}); - const handleErrorImpl = () => { Sentry.setTag('remix-test-tag', 'remix-test-value'); }; From 0799c502b351075b67f03fbf0cf96b5062a759cf Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Tue, 4 Jun 2024 16:09:31 +0100 Subject: [PATCH 11/27] Make auto-instrumentation opt-in. --- .../instrument.mjs | 2 +- .../create-remix-app-express/instrument.cjs | 1 + .../create-remix-app-v2/instrument.server.cjs | 1 + .../create-remix-app/instrument.server.cjs | 1 + packages/remix/src/index.server.ts | 14 +- packages/remix/src/utils/errors.ts | 177 ++++++++++ packages/remix/src/utils/instrumentServer.ts | 320 +++++++++--------- packages/remix/src/utils/remixOptions.ts | 1 + packages/remix/src/utils/utils.ts | 51 +++ .../test/integration/instrument.server.cjs | 1 + 10 files changed, 394 insertions(+), 175 deletions(-) create mode 100644 packages/remix/src/utils/errors.ts create mode 100644 packages/remix/src/utils/utils.ts diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/instrument.mjs b/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/instrument.mjs index 2c3d63a8b2e6..9d8e4e7fa408 100644 --- a/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/instrument.mjs +++ b/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/instrument.mjs @@ -6,5 +6,5 @@ Sentry.init({ environment: 'qa', // dynamic sampling bias to keep transactions dsn: process.env.E2E_TEST_DSN, tunnel: 'http://localhost:3031/', // proxy server - debug: true, + autoInstrumentRemix: true, // auto instrument Remix }); diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-express/instrument.cjs b/dev-packages/e2e-tests/test-applications/create-remix-app-express/instrument.cjs index feea6fb5388e..0b0ae77ad651 100644 --- a/dev-packages/e2e-tests/test-applications/create-remix-app-express/instrument.cjs +++ b/dev-packages/e2e-tests/test-applications/create-remix-app-express/instrument.cjs @@ -7,6 +7,7 @@ Sentry.init({ dsn: process.env.E2E_TEST_DSN, tunnel: 'http://localhost:3031/', // proxy server sendDefaultPii: true, // Testing the FormData + autoInstrumentRemix: true, // auto instrument Remix captureActionFormDataKeys: { file: true, text: true, diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-v2/instrument.server.cjs b/dev-packages/e2e-tests/test-applications/create-remix-app-v2/instrument.server.cjs index 6d211cac4592..5b80ca7b8695 100644 --- a/dev-packages/e2e-tests/test-applications/create-remix-app-v2/instrument.server.cjs +++ b/dev-packages/e2e-tests/test-applications/create-remix-app-v2/instrument.server.cjs @@ -5,4 +5,5 @@ Sentry.init({ environment: 'qa', // dynamic sampling bias to keep transactions dsn: process.env.E2E_TEST_DSN, tunnel: 'http://localhost:3031/', // proxy server + autoInstrumentRemix: true, // auto instrument Remix }); diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app/instrument.server.cjs b/dev-packages/e2e-tests/test-applications/create-remix-app/instrument.server.cjs index 6d211cac4592..5b80ca7b8695 100644 --- a/dev-packages/e2e-tests/test-applications/create-remix-app/instrument.server.cjs +++ b/dev-packages/e2e-tests/test-applications/create-remix-app/instrument.server.cjs @@ -5,4 +5,5 @@ Sentry.init({ environment: 'qa', // dynamic sampling bias to keep transactions dsn: process.env.E2E_TEST_DSN, tunnel: 'http://localhost:3031/', // proxy server + autoInstrumentRemix: true, // auto instrument Remix }); diff --git a/packages/remix/src/index.server.ts b/packages/remix/src/index.server.ts index 2d41b401db4c..cdc10e4e8a9d 100644 --- a/packages/remix/src/index.server.ts +++ b/packages/remix/src/index.server.ts @@ -116,12 +116,14 @@ export { export * from '@sentry/node'; export { - captureRemixServerException, // eslint-disable-next-line deprecation/deprecation wrapRemixHandleError, sentryHandleError, wrapHandleErrorWithSentry, } from './utils/instrumentServer'; + +export { captureRemixServerException } from './utils/errors'; + export { ErrorBoundary, withErrorBoundary } from '@sentry/react'; export { withSentry } from './client/performance'; export { captureRemixErrorBoundaryError } from './client/errors'; @@ -134,9 +136,9 @@ export type { SentryMetaArgs } from './utils/types'; * * @param options The options for the SDK. */ -export function getDefaultIntegrations(options: RemixOptions): Integration[] { +export function getRemixDefaultIntegrations(options: RemixOptions): Integration[] { return [ - ...getDefaultNodeIntegrations(options).filter(integration => integration.name !== 'Http'), + ...getDefaultNodeIntegrations(options as NodeOptions).filter(integration => integration.name !== 'Http'), httpIntegration(), remixIntegration(options), ]; @@ -166,11 +168,13 @@ export function init(options: RemixOptions): void { return; } - options.defaultIntegrations = getDefaultIntegrations(options as NodeOptions); + if (options.autoInstrumentRemix) { + options.defaultIntegrations = getRemixDefaultIntegrations(options as NodeOptions); + } nodeInit(options as NodeOptions); - instrumentServer(); + instrumentServer(options); setTag('runtime', 'node'); } diff --git a/packages/remix/src/utils/errors.ts b/packages/remix/src/utils/errors.ts new file mode 100644 index 000000000000..5c68d0ceece8 --- /dev/null +++ b/packages/remix/src/utils/errors.ts @@ -0,0 +1,177 @@ +import type { AppData, DataFunctionArgs, EntryContext, HandleDocumentRequestFunction } from '@remix-run/node'; +import { + captureException, + getActiveSpan, + getClient, + getCurrentScope, + getRootSpan, + handleCallbackErrors, + spanToJSON, +} from '@sentry/core'; +import { addExceptionMechanism, isPrimitive, logger, objectify } from '@sentry/utils'; +import { DEBUG_BUILD } from './debug-build'; +import type { RemixOptions } from './remixOptions'; +import { storeFormDataKeys } from './utils'; +import { extractData, isResponse, isRouteErrorResponse } from './vendor/response'; +import type { DataFunction, RemixRequest } from './vendor/types'; +import { normalizeRemixRequest } from './web-fetch'; + +/** + * Captures an exception happened in the Remix server. + * + * @param err The error to capture. + * @param name The name of the origin function. + * @param request The request object. + * + * @returns A promise that resolves when the exception is captured. + */ +export async function captureRemixServerException(err: unknown, name: string, request: Request): Promise { + const { IS_REMIX_V2 } = getCurrentScope().getScopeData().sdkProcessingMetadata; + + // Skip capturing if the thrown error is not a 5xx response + // https://remix.run/docs/en/v1/api/conventions#throwing-responses-in-loaders + if (IS_REMIX_V2 && isRouteErrorResponse(err) && err.status < 500) { + return; + } + + if (isResponse(err) && err.status < 500) { + return; + } + // Skip capturing if the request is aborted as Remix docs suggest + // Ref: https://remix.run/docs/en/main/file-conventions/entry.server#handleerror + if (request.signal.aborted) { + DEBUG_BUILD && logger.warn('Skipping capture of aborted request'); + return; + } + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + let normalizedRequest: Record = request as unknown as any; + + try { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + normalizedRequest = normalizeRemixRequest(request as unknown as any); + } catch (e) { + DEBUG_BUILD && logger.warn('Failed to normalize Remix request'); + } + + const objectifiedErr = objectify(err); + + captureException(isResponse(objectifiedErr) ? await extractResponseError(objectifiedErr) : objectifiedErr, scope => { + const activeSpan = getActiveSpan(); + const rootSpan = activeSpan && getRootSpan(activeSpan); + const activeRootSpanName = rootSpan ? spanToJSON(rootSpan).description : undefined; + + scope.setSDKProcessingMetadata({ + request: { + ...normalizedRequest, + // When `route` is not defined, `RequestData` integration uses the full URL + route: activeRootSpanName + ? { + path: activeRootSpanName, + } + : undefined, + }, + }); + + scope.addEventProcessor(event => { + addExceptionMechanism(event, { + type: 'instrument', + handled: false, + data: { + function: name, + }, + }); + + return event; + }); + + return scope; + }); +} + +/** + * + */ +export function errorHandleDocumentRequestFunction( + this: unknown, + origDocumentRequestFunction: HandleDocumentRequestFunction, + requestContext: { + request: RemixRequest; + responseStatusCode: number; + responseHeaders: Headers; + context: EntryContext; + loadContext?: Record; + }, + isRemixV2: boolean, +): HandleDocumentRequestFunction { + const { request, responseStatusCode, responseHeaders, context, loadContext } = requestContext; + + return handleCallbackErrors( + () => { + return origDocumentRequestFunction.call(this, request, responseStatusCode, responseHeaders, context, loadContext); + }, + err => { + // This exists to capture the server-side rendering errors on Remix v1 + // On Remix v2, we capture SSR errors at `handleError` + // We also skip primitives here, as we can't dedupe them, and also we don't expect any primitive SSR errors. + if (!isRemixV2 && !isPrimitive(err)) { + // eslint-disable-next-line @typescript-eslint/no-floating-promises + captureRemixServerException(err, 'documentRequest', request); + } + + throw err; + }, + ); +} + +/** + * + */ +export async function errorHandleDataFunction( + this: unknown, + origFn: DataFunction, + name: string, + args: DataFunctionArgs, + isRemixV2: boolean, +): Promise { + return handleCallbackErrors( + async () => { + const span = getActiveSpan(); + + if (name === 'action' && span) { + const options = getClient()?.getOptions() as RemixOptions; + + if (options.sendDefaultPii && options.captureActionFormDataKeys) { + await storeFormDataKeys(args, span); + } + } + + return origFn.call(this, args); + }, + err => { + // On Remix v2, we capture all unexpected errors (except the `Route Error Response`s / Thrown Responses) in `handleError` function. + // This is both for consistency and also avoid duplicates such as primitives like `string` or `number` being captured twice. + // Remix v1 does not have a `handleError` function, so we capture all errors here. + if (isRemixV2 ? isResponse(err) : true) { + // eslint-disable-next-line @typescript-eslint/no-floating-promises + captureRemixServerException(err, name, args.request); + } + + throw err; + }, + ); +} + +async function extractResponseError(response: Response): Promise { + const responseData = await extractData(response); + + if (typeof responseData === 'string' && responseData.length > 0) { + return new Error(responseData); + } + + if (response.statusText) { + return new Error(response.statusText); + } + + return responseData; +} diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index c8fc8993cc28..297aad5d27bf 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -1,29 +1,26 @@ /* eslint-disable max-lines */ import { - captureException, + SEMANTIC_ATTRIBUTE_SENTRY_OP, + SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, + SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, getActiveSpan, + getGlobalScope, getRootSpan, - handleCallbackErrors, hasTracingEnabled, spanToJSON, spanToTraceHeader, + startSpan, withIsolationScope, } from '@sentry/core'; import { continueTrace, getDynamicSamplingContextFromSpan } from '@sentry/opentelemetry'; -import type { WrappedFunction } from '@sentry/types'; -import { - addExceptionMechanism, - dynamicSamplingContextToSentryBaggageHeader, - fill, - isNodeEnv, - isPrimitive, - loadModule, - logger, - objectify, -} from '@sentry/utils'; +import type { TransactionSource, WrappedFunction } from '@sentry/types'; +import { dynamicSamplingContextToSentryBaggageHeader, fill, isNodeEnv, loadModule, logger } from '@sentry/utils'; import { DEBUG_BUILD } from './debug-build'; +import { captureRemixServerException, errorHandleDataFunction, errorHandleDocumentRequestFunction } from './errors'; import { getFutureFlagsServer, getRemixVersionFromBuild } from './futureFlags'; +import type { RemixOptions } from './remixOptions'; +import { createRoutes, getTransactionName } from './utils'; import { extractData, isDeferredData, isResponse, isRouteErrorResponse, json } from './vendor/response'; import type { AppData, @@ -42,7 +39,6 @@ import type { import { normalizeRemixRequest } from './web-fetch'; let FUTURE_FLAGS: FutureConfig | undefined; -let IS_REMIX_V2: boolean | undefined; const redirectStatusCodes = new Set([301, 302, 303, 307, 308]); function isRedirectResponse(response: Response): boolean { @@ -53,20 +49,6 @@ function isCatchResponse(response: Response): boolean { return response.headers.get('X-Remix-Catch') != null; } -async function extractResponseError(response: Response): Promise { - const responseData = await extractData(response); - - if (typeof responseData === 'string' && responseData.length > 0) { - return new Error(responseData); - } - - if (response.statusText) { - return new Error(response.statusText); - } - - return responseData; -} - /** * Sentry utility to be used in place of `handleError` function of Remix v2 * Remix Docs: https://remix.run/docs/en/main/file-conventions/entry.server#handleerror @@ -113,78 +95,7 @@ export function wrapHandleErrorWithSentry( }; } -/** - * Captures an exception happened in the Remix server. - * - * @param err The error to capture. - * @param name The name of the origin function. - * @param request The request object. - * - * @returns A promise that resolves when the exception is captured. - */ -export async function captureRemixServerException(err: unknown, name: string, request: Request): Promise { - // Skip capturing if the thrown error is not a 5xx response - // https://remix.run/docs/en/v1/api/conventions#throwing-responses-in-loaders - if (IS_REMIX_V2 && isRouteErrorResponse(err) && err.status < 500) { - return; - } - - if (isResponse(err) && err.status < 500) { - return; - } - // Skip capturing if the request is aborted as Remix docs suggest - // Ref: https://remix.run/docs/en/main/file-conventions/entry.server#handleerror - if (request.signal.aborted) { - DEBUG_BUILD && logger.warn('Skipping capture of aborted request'); - return; - } - - // eslint-disable-next-line @typescript-eslint/no-explicit-any - let normalizedRequest: Record = request as unknown as any; - - try { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - normalizedRequest = normalizeRemixRequest(request as unknown as any); - } catch (e) { - DEBUG_BUILD && logger.warn('Failed to normalize Remix request'); - } - - const objectifiedErr = objectify(err); - - captureException(isResponse(objectifiedErr) ? await extractResponseError(objectifiedErr) : objectifiedErr, scope => { - const activeSpan = getActiveSpan(); - const rootSpan = activeSpan && getRootSpan(activeSpan); - const activeRootSpanName = rootSpan ? spanToJSON(rootSpan).description : undefined; - - scope.setSDKProcessingMetadata({ - request: { - ...normalizedRequest, - // When `route` is not defined, `RequestData` integration uses the full URL - route: activeRootSpanName - ? { - path: activeRootSpanName, - } - : undefined, - }, - }); - - scope.addEventProcessor(event => { - addExceptionMechanism(event, { - type: 'instrument', - handled: false, - data: { - function: name, - }, - }); - - return event; - }); - - return scope; - }); -} - -function makeWrappedDocumentRequestFunction(remixVersion?: number) { +function makeWrappedDocumentRequestFunction(autoInstrumentRemix?: boolean, remixVersion?: number) { return function (origDocumentRequestFunction: HandleDocumentRequestFunction): HandleDocumentRequestFunction { return async function ( this: unknown, @@ -194,31 +105,52 @@ function makeWrappedDocumentRequestFunction(remixVersion?: number) { context: EntryContext, loadContext?: Record, ): Promise { - return handleCallbackErrors( - () => { - return origDocumentRequestFunction.call( - this, - request, - responseStatusCode, - responseHeaders, - context, - loadContext, - ); - }, - err => { - const isRemixV1 = !FUTURE_FLAGS?.v2_errorBoundary && remixVersion !== 2; - - // This exists to capture the server-side rendering errors on Remix v1 - // On Remix v2, we capture SSR errors at `handleError` - // We also skip primitives here, as we can't dedupe them, and also we don't expect any primitive SSR errors. - if (isRemixV1 && !isPrimitive(err)) { - // eslint-disable-next-line @typescript-eslint/no-floating-promises - captureRemixServerException(err, 'documentRequest', request); - } + const documentRequestContext = { + request, + responseStatusCode, + responseHeaders, + context, + loadContext, + }; - throw err; - }, - ); + const isRemixV2 = FUTURE_FLAGS?.v2_errorBoundary || remixVersion === 2; + + if (!autoInstrumentRemix) { + const activeSpan = getActiveSpan(); + const rootSpan = activeSpan && getRootSpan(activeSpan); + + const name = rootSpan ? spanToJSON(rootSpan).description : undefined; + + return startSpan( + { + // If we don't have a root span, `onlyIfParent` will lead to the span not being created anyhow + // So we don't need to care too much about the fallback name, it's just for typing purposes.... + name: name || '', + onlyIfParent: true, + attributes: { + method: request.method, + url: request.url, + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.function.remix', + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'function.remix.document_request', + }, + }, + () => { + return errorHandleDocumentRequestFunction.call( + this, + origDocumentRequestFunction, + documentRequestContext, + isRemixV2, + ); + }, + ); + } else { + return errorHandleDocumentRequestFunction.call( + this, + origDocumentRequestFunction, + documentRequestContext, + isRemixV2, + ); + } }; }; } @@ -228,39 +160,41 @@ function makeWrappedDataFunction( id: string, name: 'action' | 'loader', remixVersion: number, + autoInstrumentRemix?: boolean, ): DataFunction { return async function (this: unknown, args: DataFunctionArgs): Promise { - return handleCallbackErrors( - async () => { - return origFn.call(this, args); - }, - err => { - const isRemixV2 = FUTURE_FLAGS?.v2_errorBoundary || remixVersion === 2; - - // On Remix v2, we capture all unexpected errors (except the `Route Error Response`s / Thrown Responses) in `handleError` function. - // This is both for consistency and also avoid duplicates such as primitives like `string` or `number` being captured twice. - // Remix v1 does not have a `handleError` function, so we capture all errors here. - if (isRemixV2 ? isResponse(err) : true) { - // eslint-disable-next-line @typescript-eslint/no-floating-promises - captureRemixServerException(err, name, args.request); - } + const isRemixV2 = FUTURE_FLAGS?.v2_errorBoundary || remixVersion === 2; - throw err; - }, - ); + if (!autoInstrumentRemix) { + return startSpan( + { + op: `function.remix.${name}`, + name: id, + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.function.remix', + name, + }, + }, + () => { + return errorHandleDataFunction.call(this, origFn, name, args, isRemixV2); + }, + ); + } else { + return errorHandleDataFunction.call(this, origFn, name, args, isRemixV2); + } }; } const makeWrappedAction = - (id: string, remixVersion: number) => + (id: string, remixVersion: number, autoInstrumentRemix?: boolean) => (origAction: DataFunction): DataFunction => { - return makeWrappedDataFunction(origAction, id, 'action', remixVersion); + return makeWrappedDataFunction(origAction, id, 'action', remixVersion, autoInstrumentRemix); }; const makeWrappedLoader = - (id: string, remixVersion: number) => + (id: string, remixVersion: number, autoInstrumentRemix?: boolean) => (origLoader: DataFunction): DataFunction => { - return makeWrappedDataFunction(origLoader, id, 'loader', remixVersion); + return makeWrappedDataFunction(origLoader, id, 'loader', remixVersion, autoInstrumentRemix); }; function getTraceAndBaggage(): { @@ -329,7 +263,11 @@ function makeWrappedRootLoader(remixVersion: number) { }; } -function wrapRequestHandler(origRequestHandler: RequestHandler): RequestHandler { +function wrapRequestHandler( + origRequestHandler: RequestHandler, + build: ServerBuild | (() => ServerBuild | Promise), + autoInstrumentRemix: boolean, +): RequestHandler { return async function (this: unknown, request: RemixRequest, loadContext?: AppLoadContext): Promise { const upperCaseMethod = request.method.toUpperCase(); // We don't want to wrap OPTIONS and HEAD requests @@ -337,6 +275,25 @@ function wrapRequestHandler(origRequestHandler: RequestHandler): RequestHandler return origRequestHandler.call(this, request, loadContext); } + let name: string; + let source: TransactionSource; + + if (!autoInstrumentRemix) { + let resolvedBuild: ServerBuild; + + if (typeof build === 'function') { + resolvedBuild = await build(); + } else { + resolvedBuild = build; + } + + const routes = createRoutes(resolvedBuild.routes); + + // const routes = createRoutes(build.routes); + const url = new URL(request.url); + [name, source] = getTransactionName(routes, url); + } + return withIsolationScope(async isolationScope => { let normalizedRequest: Record = request; @@ -350,12 +307,30 @@ function wrapRequestHandler(origRequestHandler: RequestHandler): RequestHandler ...normalizedRequest, }, }); + return continueTrace( { sentryTrace: request.headers.get('sentry-trace') || '', baggage: request.headers.get('baggage') || '', }, async () => { + if (!autoInstrumentRemix) { + return startSpan( + { + name, + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.remix', + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: source, + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'http.server', + method: request.method, + }, + }, + () => { + return origRequestHandler.call(this, request, loadContext); + }, + ); + } + return (await origRequestHandler.call(this, request, loadContext)) as Response; }, ); @@ -363,11 +338,16 @@ function wrapRequestHandler(origRequestHandler: RequestHandler): RequestHandler }; } -function instrumentBuildCallback(build: ServerBuild): ServerBuild { +function instrumentBuildCallback(build: ServerBuild, autoInstrumentRemix: boolean): ServerBuild { const routes: ServerRouteManifest = {}; const remixVersion = getRemixVersionFromBuild(build); - IS_REMIX_V2 = remixVersion === 2; + + const globalScope = getGlobalScope(); + + globalScope.setSDKProcessingMetadata({ + IS_REMIX_V2: remixVersion === 2, + }); const wrappedEntry = { ...build.entry, module: { ...build.entry.module } }; @@ -377,7 +357,7 @@ function instrumentBuildCallback(build: ServerBuild): ServerBuild { // We should be able to wrap them, as they may not be wrapped before. const defaultExport = wrappedEntry.module.default as undefined | WrappedFunction; if (defaultExport && !defaultExport.__sentry_original__) { - fill(wrappedEntry.module, 'default', makeWrappedDocumentRequestFunction(remixVersion)); + fill(wrappedEntry.module, 'default', makeWrappedDocumentRequestFunction(autoInstrumentRemix, remixVersion)); } for (const [id, route] of Object.entries(build.routes)) { @@ -385,12 +365,12 @@ function instrumentBuildCallback(build: ServerBuild): ServerBuild { const routeAction = wrappedRoute.module.action as undefined | WrappedFunction; if (routeAction && !routeAction.__sentry_original__) { - fill(wrappedRoute.module, 'action', makeWrappedAction(id, remixVersion)); + fill(wrappedRoute.module, 'action', makeWrappedAction(id, remixVersion, autoInstrumentRemix)); } const routeLoader = wrappedRoute.module.loader as undefined | WrappedFunction; if (routeLoader && !routeLoader.__sentry_original__) { - fill(wrappedRoute.module, 'loader', makeWrappedLoader(id, remixVersion)); + fill(wrappedRoute.module, 'loader', makeWrappedLoader(id, remixVersion, autoInstrumentRemix)); } // Entry module should have a loader function to provide `sentry-trace` and `baggage` @@ -415,7 +395,10 @@ function instrumentBuildCallback(build: ServerBuild): ServerBuild { */ export function instrumentBuild( build: ServerBuild | (() => ServerBuild | Promise), + options: RemixOptions, ): ServerBuild | (() => ServerBuild | Promise) { + const autoInstrumentRemix = options?.autoInstrumentRemix || false; + if (typeof build === 'function') { return function () { const resolvedBuild = build(); @@ -424,41 +407,40 @@ export function instrumentBuild( return resolvedBuild.then(build => { FUTURE_FLAGS = getFutureFlagsServer(build); - return instrumentBuildCallback(build); + return instrumentBuildCallback(build, autoInstrumentRemix); }); } else { FUTURE_FLAGS = getFutureFlagsServer(resolvedBuild); - return instrumentBuildCallback(resolvedBuild); + return instrumentBuildCallback(resolvedBuild, autoInstrumentRemix); } }; } else { FUTURE_FLAGS = getFutureFlagsServer(build); - return instrumentBuildCallback(build); + return instrumentBuildCallback(build, autoInstrumentRemix); } } -function makeWrappedCreateRequestHandler( - origCreateRequestHandler: CreateRequestHandlerFunction, -): CreateRequestHandlerFunction { - return function ( - this: unknown, - build: ServerBuild | (() => Promise), - ...args: unknown[] - ): RequestHandler { - const newBuild = instrumentBuild(build); - const requestHandler = origCreateRequestHandler.call(this, newBuild, ...args); - - return wrapRequestHandler(requestHandler); +const makeWrappedCreateRequestHandler = (options: RemixOptions) => + function (origCreateRequestHandler: CreateRequestHandlerFunction): CreateRequestHandlerFunction { + return function ( + this: unknown, + build: ServerBuild | (() => Promise), + ...args: unknown[] + ): RequestHandler { + const newBuild = instrumentBuild(build, options); + const requestHandler = origCreateRequestHandler.call(this, newBuild, ...args); + + return wrapRequestHandler(requestHandler, newBuild, options.autoInstrumentRemix || false); + }; }; -} /** * 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 { +export function instrumentServer(options: RemixOptions): void { const pkg = loadModule<{ createRequestHandler: CreateRequestHandlerFunction; }>('@remix-run/server-runtime'); @@ -469,5 +451,5 @@ export function instrumentServer(): void { return; } - fill(pkg, 'createRequestHandler', makeWrappedCreateRequestHandler); + fill(pkg, 'createRequestHandler', makeWrappedCreateRequestHandler(options)); } diff --git a/packages/remix/src/utils/remixOptions.ts b/packages/remix/src/utils/remixOptions.ts index 26eae9c024c3..4f73eca92ff3 100644 --- a/packages/remix/src/utils/remixOptions.ts +++ b/packages/remix/src/utils/remixOptions.ts @@ -4,4 +4,5 @@ import type { Options } from '@sentry/types'; export type RemixOptions = (Options | BrowserOptions | NodeOptions) & { captureActionFormDataKeys?: Record; + autoInstrumentRemix?: boolean; }; diff --git a/packages/remix/src/utils/utils.ts b/packages/remix/src/utils/utils.ts new file mode 100644 index 000000000000..fed9e721e013 --- /dev/null +++ b/packages/remix/src/utils/utils.ts @@ -0,0 +1,51 @@ +import type { DataFunctionArgs } from '@remix-run/node'; +import type { Span, TransactionSource } from '@sentry/types'; +import { logger } from '@sentry/utils'; +import { DEBUG_BUILD } from './debug-build'; +import { getRequestMatch, matchServerRoutes } from './vendor/response'; +import type { ServerRoute, ServerRouteManifest } from './vendor/types'; + +/** + * + */ +export async function storeFormDataKeys(args: DataFunctionArgs, span: Span): Promise { + try { + // We clone the request for Remix be able to read the FormData later. + const clonedRequest = args.request.clone(); + + // This only will return the last name of multiple file uploads in a single FormData entry. + // We can switch to `unstable_parseMultipartFormData` when it's stable. + // https://remix.run/docs/en/main/utils/parse-multipart-form-data#unstable_parsemultipartformdata + const formData = await clonedRequest.formData(); + + formData.forEach((value, key) => { + span.setAttribute(`remix.action_form_data.${key}`, typeof value === 'string' ? value : '[non-string value]'); + }); + } catch (e) { + DEBUG_BUILD && logger.warn('Failed to read FormData from request', e); + } +} + +/** + * Get transaction name from routes and url + */ +export function getTransactionName(routes: ServerRoute[], url: URL): [string, TransactionSource] { + const matches = matchServerRoutes(routes, url.pathname); + const match = matches && getRequestMatch(url, matches); + return match === null ? [url.pathname, 'url'] : [match.route.id || 'no-route-id', 'route']; +} + +/** + * 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]) => ({ + ...route, + children: createRoutes(manifest, id), + })); +} diff --git a/packages/remix/test/integration/instrument.server.cjs b/packages/remix/test/integration/instrument.server.cjs index 199d4e309c41..1dd801c7352f 100644 --- a/packages/remix/test/integration/instrument.server.cjs +++ b/packages/remix/test/integration/instrument.server.cjs @@ -6,4 +6,5 @@ Sentry.init({ tracePropagationTargets: ['example.org'], // Disabling to test series of envelopes deterministically. autoSessionTracking: false, + autoInstrumentRemix: true, }); From bd3feeb897aa4506079aff2dee5d8862d53d7b3f Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Thu, 6 Jun 2024 00:52:16 +0100 Subject: [PATCH 12/27] Add legacy integration tests back. --- packages/remix/package.json | 6 +- .../test/integration/instrument.server.cjs | 2 +- .../instrumentation-legacy/action.test.ts | 500 ++++++++++++++++++ .../instrumentation-legacy/loader.test.ts | 287 ++++++++++ .../server/instrumentation-legacy/ssr.test.ts | 50 ++ .../{ => instrumentation-otel}/action.test.ts | 2 +- .../{ => instrumentation-otel}/loader.test.ts | 2 +- .../{ => instrumentation-otel}/ssr.test.ts | 2 +- packages/remix/vitest.config.ts | 2 + 9 files changed, 847 insertions(+), 6 deletions(-) create mode 100644 packages/remix/test/integration/test/server/instrumentation-legacy/action.test.ts create mode 100644 packages/remix/test/integration/test/server/instrumentation-legacy/loader.test.ts create mode 100644 packages/remix/test/integration/test/server/instrumentation-legacy/ssr.test.ts rename packages/remix/test/integration/test/server/{ => instrumentation-otel}/action.test.ts (99%) rename packages/remix/test/integration/test/server/{ => instrumentation-otel}/loader.test.ts (99%) rename packages/remix/test/integration/test/server/{ => instrumentation-otel}/ssr.test.ts (98%) diff --git a/packages/remix/package.json b/packages/remix/package.json index 242ca87baeff..18b7d2584af9 100644 --- a/packages/remix/package.json +++ b/packages/remix/package.json @@ -99,7 +99,9 @@ "fix": "eslint . --format stylish --fix", "lint": "eslint . --format stylish", "test": "yarn test:unit", - "test:integration": "run-s test:integration:v1 test:integration:v2", + "test:integration": "run-s test:integration:otel test:integration:legacy", + "test:integration:otel": "export USE_OTEL=1 && run-s test:integration:v1 test:integration:v2", + "test:integration:legacy": "export USE_OTEL=0 && run-s test:integration:v1 test:integration:v2", "test:integration:v1": "run-s test:integration:clean test:integration:prepare test:integration:client test:integration:server", "test:integration:v2": "export REMIX_VERSION=2 && run-s test:integration:v1", "test:integration:ci": "run-s test:integration:clean test:integration:prepare test:integration:client:ci test:integration:server", @@ -107,7 +109,7 @@ "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/ --project='chromium'", "test:integration:client:ci": "yarn test:integration:client --reporter='line'", - "test:integration:server": "export NODE_OPTIONS='--stack-trace-limit=25' && vitest run test/integration/test/server/", + "test:integration:server": "export NODE_OPTIONS='--stack-trace-limit=25' && vitest run", "test:unit": "jest", "test:watch": "jest --watch", "yalc:publish": "ts-node ../../scripts/prepack.ts && yalc publish build --push --sig" diff --git a/packages/remix/test/integration/instrument.server.cjs b/packages/remix/test/integration/instrument.server.cjs index 1dd801c7352f..5e1d9e31ab46 100644 --- a/packages/remix/test/integration/instrument.server.cjs +++ b/packages/remix/test/integration/instrument.server.cjs @@ -6,5 +6,5 @@ Sentry.init({ tracePropagationTargets: ['example.org'], // Disabling to test series of envelopes deterministically. autoSessionTracking: false, - autoInstrumentRemix: true, + autoInstrumentRemix: process.env.USE_OTEL === '1', }); diff --git a/packages/remix/test/integration/test/server/instrumentation-legacy/action.test.ts b/packages/remix/test/integration/test/server/instrumentation-legacy/action.test.ts new file mode 100644 index 000000000000..a53d3953417b --- /dev/null +++ b/packages/remix/test/integration/test/server/instrumentation-legacy/action.test.ts @@ -0,0 +1,500 @@ +import { describe, it } from 'vitest'; +import { RemixTestEnv, assertSentryEvent, assertSentryTransaction } from '../utils/helpers'; + +const useV2 = process.env.REMIX_VERSION === '2'; + +describe('Remix API Actions', () => { + it('correctly instruments a parameterized Remix API action', async () => { + const env = await RemixTestEnv.init(); + const url = `${env.url}/action-json-response/123123`; + const envelopes = await env.getMultipleEnvelopeRequest({ + url, + method: 'post', + envelopeType: 'transaction', + count: 1, + }); + const transaction = envelopes[0][2]; + + assertSentryTransaction(transaction, { + transaction: `routes/action-json-response${useV2 ? '.' : '/'}$id`, + spans: [ + { + description: `routes/action-json-response${useV2 ? '.' : '/'}$id`, + op: 'function.remix.action', + }, + { + description: 'root', + op: 'function.remix.loader', + }, + { + description: `routes/action-json-response${useV2 ? '.' : '/'}$id`, + op: 'function.remix.loader', + }, + { + description: `routes/action-json-response${useV2 ? '.' : '/'}$id`, + op: 'function.remix.document_request', + }, + ], + request: { + method: 'POST', + url, + cookies: expect.any(Object), + headers: { + 'user-agent': expect.any(String), + host: expect.stringContaining('localhost:'), + }, + }, + }); + }); + + it('reports an error thrown from the action', async () => { + const env = await RemixTestEnv.init(); + const url = `${env.url}/action-json-response/-1`; + + const envelopes = await env.getMultipleEnvelopeRequest({ + url, + count: 2, + method: 'post', + envelopeType: ['transaction', 'event'], + }); + + const [transaction] = envelopes.filter(envelope => envelope[1].type === 'transaction'); + const [event] = envelopes.filter(envelope => envelope[1].type === 'event'); + + assertSentryTransaction(transaction[2], { + contexts: { + trace: { + status: 'internal_error', + data: { + 'http.response.status_code': 500, + }, + }, + }, + }); + + assertSentryEvent(event[2], { + transaction: expect.stringMatching(/routes\/action-json-response(\/|\.)\$id/), + exception: { + values: [ + { + type: 'Error', + value: 'Unexpected Server Error', + stacktrace: expect.any(Object), + mechanism: { + data: { + function: useV2 ? 'remix.server.handleError' : 'action', + }, + handled: false, + type: 'instrument', + }, + }, + ], + }, + }); + }); + + it('includes request data in transaction and error events', async () => { + const env = await RemixTestEnv.init(); + const url = `${env.url}/action-json-response/-1`; + + const envelopes = await env.getMultipleEnvelopeRequest({ + url, + count: 2, + method: 'post', + envelopeType: ['transaction', 'event'], + }); + + const [transaction] = envelopes.filter(envelope => envelope[1].type === 'transaction'); + const [event] = envelopes.filter(envelope => envelope[1].type === 'event'); + + assertSentryTransaction(transaction[2], { + transaction: `routes/action-json-response${useV2 ? '.' : '/'}$id`, + request: { + method: 'POST', + url, + cookies: expect.any(Object), + headers: { + 'user-agent': expect.any(String), + host: expect.stringContaining('localhost:'), + }, + }, + }); + + assertSentryEvent(event[2], { + transaction: expect.stringMatching(/routes\/action-json-response(\/|\.)\$id/), + exception: { + values: [ + { + type: 'Error', + value: 'Unexpected Server Error', + }, + ], + }, + request: { + method: 'POST', + url, + cookies: expect.any(Object), + headers: { + 'user-agent': expect.any(String), + host: expect.stringContaining('localhost:'), + }, + }, + }); + }); + + it('handles an error-throwing redirection target', async () => { + const env = await RemixTestEnv.init(); + const url = `${env.url}/action-json-response/-2`; + + const envelopes = await env.getMultipleEnvelopeRequest({ + url, + count: 3, + method: 'post', + envelopeType: ['transaction', 'event'], + }); + + const [transaction_1, transaction_2] = envelopes.filter(envelope => envelope[1].type === 'transaction'); + const [event] = envelopes.filter(envelope => envelope[1].type === 'event'); + assertSentryTransaction(transaction_1[2], { + contexts: { + trace: { + op: 'http.server', + status: 'ok', + data: { + method: 'POST', + 'http.response.status_code': 302, + }, + }, + }, + transaction: `routes/action-json-response${useV2 ? '.' : '/'}$id`, + }); + + assertSentryTransaction(transaction_2[2], { + contexts: { + trace: { + op: 'http.server', + status: 'internal_error', + data: { + method: 'GET', + 'http.response.status_code': 500, + }, + }, + }, + transaction: `routes/action-json-response${useV2 ? '.' : '/'}$id`, + }); + + assertSentryEvent(event[2], { + transaction: expect.stringMatching(/routes\/action-json-response(\/|\.)\$id/), + exception: { + values: [ + { + type: 'Error', + value: 'Unexpected Server Error', + stacktrace: expect.any(Object), + mechanism: { + data: { + function: useV2 ? 'remix.server.handleError' : 'loader', + }, + handled: false, + type: 'instrument', + }, + }, + ], + }, + }); + }); + + it('handles a thrown `json()` error response with `statusText`', async () => { + const env = await RemixTestEnv.init(); + const url = `${env.url}/action-json-response/-3`; + + const envelopes = await env.getMultipleEnvelopeRequest({ + url, + count: 2, + method: 'post', + envelopeType: ['transaction', 'event'], + }); + + const [transaction] = envelopes.filter(envelope => envelope[1].type === 'transaction'); + const [event] = envelopes.filter(envelope => envelope[1].type === 'event'); + + assertSentryTransaction(transaction[2], { + contexts: { + trace: { + op: 'http.server', + status: 'internal_error', + data: { + method: 'POST', + 'http.response.status_code': 500, + }, + }, + }, + transaction: `routes/action-json-response${useV2 ? '.' : '/'}$id`, + }); + + assertSentryEvent(event[2], { + transaction: expect.stringMatching(/routes\/action-json-response(\/|\.)\$id/), + exception: { + values: [ + { + type: 'Error', + value: 'Sentry Test Error', + stacktrace: expect.any(Object), + mechanism: { + data: { + function: 'action', + }, + handled: false, + type: 'instrument', + }, + }, + ], + }, + }); + }); + + it('handles a thrown `json()` error response without `statusText`', async () => { + const env = await RemixTestEnv.init(); + const url = `${env.url}/action-json-response/-4`; + + const envelopes = await env.getMultipleEnvelopeRequest({ + url, + count: 2, + method: 'post', + envelopeType: ['transaction', 'event'], + }); + + const [transaction] = envelopes.filter(envelope => envelope[1].type === 'transaction'); + const [event] = envelopes.filter(envelope => envelope[1].type === 'event'); + + assertSentryTransaction(transaction[2], { + contexts: { + trace: { + op: 'http.server', + status: 'internal_error', + data: { + method: 'POST', + 'http.response.status_code': 500, + }, + }, + }, + transaction: `routes/action-json-response${useV2 ? '.' : '/'}$id`, + }); + + assertSentryEvent(event[2], { + transaction: expect.stringMatching(/routes\/action-json-response(\/|\.)\$id/), + exception: { + values: [ + { + type: 'Error', + value: 'Object captured as exception with keys: data', + stacktrace: expect.any(Object), + mechanism: { + data: { + function: 'action', + }, + handled: false, + type: 'instrument', + }, + }, + ], + }, + }); + }); + + it('handles a thrown `json()` error response with string body', async () => { + const env = await RemixTestEnv.init(); + const url = `${env.url}/action-json-response/-5`; + + const envelopes = await env.getMultipleEnvelopeRequest({ + url, + count: 2, + method: 'post', + envelopeType: ['transaction', 'event'], + }); + + const [transaction] = envelopes.filter(envelope => envelope[1].type === 'transaction'); + const [event] = envelopes.filter(envelope => envelope[1].type === 'event'); + + assertSentryTransaction(transaction[2], { + contexts: { + trace: { + op: 'http.server', + status: 'internal_error', + data: { + method: 'POST', + 'http.response.status_code': 500, + }, + }, + }, + transaction: `routes/action-json-response${useV2 ? '.' : '/'}$id`, + }); + + assertSentryEvent(event[2], { + transaction: expect.stringMatching(/routes\/action-json-response(\/|\.)\$id/), + exception: { + values: [ + { + type: 'Error', + value: 'Sentry Test Error [string body]', + stacktrace: expect.any(Object), + mechanism: { + data: { + function: 'action', + }, + handled: false, + type: 'instrument', + }, + }, + ], + }, + }); + }); + + it('handles a thrown `json()` error response with an empty object', async () => { + const env = await RemixTestEnv.init(); + const url = `${env.url}/action-json-response/-6`; + + const envelopes = await env.getMultipleEnvelopeRequest({ + url, + count: 2, + method: 'post', + envelopeType: ['transaction', 'event'], + }); + + const [transaction] = envelopes.filter(envelope => envelope[1].type === 'transaction'); + const [event] = envelopes.filter(envelope => envelope[1].type === 'event'); + + assertSentryTransaction(transaction[2], { + contexts: { + trace: { + op: 'http.server', + status: 'internal_error', + data: { + method: 'POST', + 'http.response.status_code': 500, + }, + }, + }, + transaction: `routes/action-json-response${useV2 ? '.' : '/'}$id`, + }); + + assertSentryEvent(event[2], { + transaction: expect.stringMatching(/routes\/action-json-response(\/|\.)\$id/), + exception: { + values: [ + { + type: 'Error', + value: 'Object captured as exception with keys: [object has no keys]', + stacktrace: expect.any(Object), + mechanism: { + data: { + function: 'action', + }, + handled: false, + type: 'instrument', + }, + }, + ], + }, + }); + }); + + it('handles thrown string (primitive) from an action', async () => { + const env = await RemixTestEnv.init(); + const url = `${env.url}/server-side-unexpected-errors/-1`; + + const envelopes = await env.getMultipleEnvelopeRequest({ + url, + count: 2, + method: 'post', + envelopeType: ['event', 'transaction'], + }); + + const [transaction] = envelopes.filter(envelope => envelope[1].type === 'transaction'); + const [event] = envelopes.filter(envelope => envelope[1].type === 'event'); + + assertSentryTransaction(transaction[2], { + contexts: { + trace: { + op: 'http.server', + status: 'internal_error', + data: { + method: 'POST', + 'http.response.status_code': 500, + }, + }, + }, + transaction: `routes/server-side-unexpected-errors${useV2 ? '.' : '/'}$id`, + }); + + assertSentryEvent(event[2], { + transaction: expect.stringMatching(/routes\/server-side-unexpected-errors(\/|\.)\$id/), + exception: { + values: [ + { + type: 'Error', + value: 'Thrown String Error', + stacktrace: expect.any(Object), + mechanism: { + data: { + function: useV2 ? 'remix.server.handleError' : 'action', + }, + handled: false, + type: 'instrument', + }, + }, + ], + }, + }); + }); + + it('handles thrown object from an action', async () => { + const env = await RemixTestEnv.init(); + const url = `${env.url}/server-side-unexpected-errors/-2`; + + const envelopes = await env.getMultipleEnvelopeRequest({ + url, + count: 2, + method: 'post', + envelopeType: ['event', 'transaction'], + }); + + const [transaction] = envelopes.filter(envelope => envelope[1].type === 'transaction'); + const [event] = envelopes.filter(envelope => envelope[1].type === 'event'); + + assertSentryTransaction(transaction[2], { + contexts: { + trace: { + op: 'http.server', + status: 'internal_error', + data: { + method: 'POST', + 'http.response.status_code': 500, + }, + }, + }, + transaction: `routes/server-side-unexpected-errors${useV2 ? '.' : '/'}$id`, + }); + + assertSentryEvent(event[2], { + transaction: expect.stringMatching(/routes\/server-side-unexpected-errors(\/|\.)\$id/), + exception: { + values: [ + { + type: 'Error', + value: 'Thrown Object Error', + stacktrace: expect.any(Object), + mechanism: { + data: { + function: useV2 ? 'remix.server.handleError' : 'action', + }, + handled: false, + type: 'instrument', + }, + }, + ], + }, + }); + }); +}); diff --git a/packages/remix/test/integration/test/server/instrumentation-legacy/loader.test.ts b/packages/remix/test/integration/test/server/instrumentation-legacy/loader.test.ts new file mode 100644 index 000000000000..a90dda8b60bd --- /dev/null +++ b/packages/remix/test/integration/test/server/instrumentation-legacy/loader.test.ts @@ -0,0 +1,287 @@ +import { Event } from '@sentry/types'; +import { describe, expect, it } from 'vitest'; +import { RemixTestEnv, assertSentryEvent, assertSentryTransaction } from '../utils/helpers'; + +const useV2 = process.env.REMIX_VERSION === '2'; + +describe('Remix API Loaders', () => { + it('reports an error thrown from the loader', async () => { + const env = await RemixTestEnv.init(); + const url = `${env.url}/loader-json-response/-2`; + + const envelopes = await env.getMultipleEnvelopeRequest({ url, count: 2, envelopeType: ['transaction', 'event'] }); + + const event = envelopes[0][2].type === 'transaction' ? envelopes[1][2] : envelopes[0][2]; + const transaction = envelopes[0][2].type === 'transaction' ? envelopes[0][2] : envelopes[1][2]; + + assertSentryTransaction(transaction, { + contexts: { + trace: { + status: 'internal_error', + data: { + 'http.response.status_code': 500, + }, + }, + }, + }); + + assertSentryEvent(event, { + transaction: expect.stringMatching(/routes\/loader-json-response(\/|\.)\$id/), + exception: { + values: [ + { + type: 'Error', + value: 'Unexpected Server Error', + stacktrace: expect.any(Object), + mechanism: { + data: { + function: useV2 ? 'remix.server.handleError' : 'loader', + }, + handled: false, + type: 'instrument', + }, + }, + ], + }, + }); + }); + + it('reports a thrown error response the loader', async () => { + const env = await RemixTestEnv.init(); + const url = `${env.url}/loader-throw-response/-1`; + + // We also wait for the transaction, even though we don't care about it for this test + // but otherwise this may leak into another test + const envelopes = await env.getMultipleEnvelopeRequest({ url, count: 2, envelopeType: ['event', 'transaction'] }); + + const event = envelopes[0][2].type === 'transaction' ? envelopes[1][2] : envelopes[0][2]; + const transaction = envelopes[0][2].type === 'transaction' ? envelopes[0][2] : envelopes[1][2]; + + assertSentryTransaction(transaction, { + contexts: { + trace: { + status: 'internal_error', + data: { + 'http.response.status_code': 500, + }, + }, + }, + }); + + assertSentryEvent(event, { + transaction: expect.stringMatching(/routes\/loader-throw-response(\/|\.)\$id/), + exception: { + values: [ + { + type: 'Error', + value: 'Not found', + stacktrace: expect.any(Object), + mechanism: { + data: { + function: 'loader', + }, + handled: false, + type: 'instrument', + }, + }, + ], + }, + }); + }); + + it('correctly instruments a parameterized Remix API loader', async () => { + const env = await RemixTestEnv.init(); + const url = `${env.url}/loader-json-response/123123`; + const envelope = await env.getEnvelopeRequest({ url, envelopeType: 'transaction' }); + const transaction = envelope[2]; + + assertSentryTransaction(transaction, { + transaction: `routes/loader-json-response${useV2 ? '.' : '/'}$id`, + transaction_info: { + source: 'route', + }, + spans: [ + { + description: 'root', + op: 'function.remix.loader', + }, + { + description: `routes/loader-json-response${useV2 ? '.' : '/'}$id`, + op: 'function.remix.loader', + }, + { + description: `routes/loader-json-response${useV2 ? '.' : '/'}$id`, + op: 'function.remix.document_request', + }, + ], + }); + }); + + it('handles an error-throwing redirection target', async () => { + const env = await RemixTestEnv.init(); + const url = `${env.url}/loader-json-response/-1`; + + const envelopes = await env.getMultipleEnvelopeRequest({ + url, + count: 3, + envelopeType: ['transaction', 'event'], + }); + + const [transaction_1, transaction_2] = envelopes.filter(envelope => envelope[1].type === 'transaction'); + const [event] = envelopes.filter(envelope => envelope[1].type === 'event'); + + assertSentryTransaction(transaction_1[2], { + contexts: { + trace: { + op: 'http.server', + status: 'ok', + data: { + method: 'GET', + 'http.response.status_code': 302, + }, + }, + }, + transaction: `routes/loader-json-response${useV2 ? '.' : '/'}$id`, + }); + + assertSentryTransaction(transaction_2[2], { + contexts: { + trace: { + op: 'http.server', + status: 'internal_error', + data: { + method: 'GET', + 'http.response.status_code': 500, + }, + }, + }, + transaction: `routes/loader-json-response${useV2 ? '.' : '/'}$id`, + }); + + assertSentryEvent(event[2], { + transaction: expect.stringMatching(/routes\/loader-json-response(\/|\.)\$id/), + exception: { + values: [ + { + type: 'Error', + value: 'Unexpected Server Error', + stacktrace: expect.any(Object), + mechanism: { + data: { + function: useV2 ? 'remix.server.handleError' : 'loader', + }, + handled: false, + type: 'instrument', + }, + }, + ], + }, + }); + }); + + it('makes sure scope does not bleed between requests', async () => { + const env = await RemixTestEnv.init(); + + const envelopes = await Promise.all([ + env.getEnvelopeRequest({ url: `${env.url}/scope-bleed/1`, endServer: false, envelopeType: 'transaction' }), + env.getEnvelopeRequest({ url: `${env.url}/scope-bleed/2`, endServer: false, envelopeType: 'transaction' }), + env.getEnvelopeRequest({ url: `${env.url}/scope-bleed/3`, endServer: false, envelopeType: 'transaction' }), + env.getEnvelopeRequest({ url: `${env.url}/scope-bleed/4`, endServer: false, envelopeType: 'transaction' }), + ]); + + await new Promise(resolve => env.server.close(resolve)); + + envelopes.forEach(envelope => { + const tags = envelope[2].tags as NonNullable; + const customTagArr = Object.keys(tags).filter(t => t.startsWith('tag')); + expect(customTagArr).toHaveLength(1); + + const key = customTagArr[0]; + const val = key[key.length - 1]; + expect(tags[key]).toEqual(val); + }); + }); + + it('continues transaction from sentry-trace header and baggage', async () => { + const env = await RemixTestEnv.init(); + const url = `${env.url}/loader-json-response/3`; + + // send sentry-trace and baggage headers to loader + env.setAxiosConfig({ + headers: { + 'sentry-trace': '12312012123120121231201212312012-1121201211212012-1', + baggage: 'sentry-version=1.0,sentry-environment=production,sentry-trace_id=12312012123120121231201212312012', + }, + }); + const envelope = await env.getEnvelopeRequest({ url, envelopeType: 'transaction' }); + + expect(envelope[0].trace).toMatchObject({ + trace_id: '12312012123120121231201212312012', + }); + + assertSentryTransaction(envelope[2], { + contexts: { + trace: { + trace_id: '12312012123120121231201212312012', + parent_span_id: '1121201211212012', + }, + }, + }); + }); + + it('correctly instruments a deferred loader', async () => { + const env = await RemixTestEnv.init(); + const url = `${env.url}/loader-defer-response/123123`; + const envelope = await env.getEnvelopeRequest({ url, envelopeType: 'transaction' }); + const transaction = envelope[2]; + + assertSentryTransaction(transaction, { + transaction: useV2 ? 'routes/loader-defer-response.$id' : 'routes/loader-defer-response/$id', + transaction_info: { + source: 'route', + }, + spans: useV2 + ? [ + { + description: 'root', + op: 'function.remix.loader', + }, + { + description: 'routes/loader-defer-response.$id', + op: 'function.remix.loader', + }, + { + description: 'routes/loader-defer-response.$id', + op: 'function.remix.document_request', + }, + ] + : [ + { + description: 'root', + op: 'function.remix.loader', + }, + { + description: 'routes/loader-defer-response/$id', + op: 'function.remix.loader', + }, + { + description: 'routes/loader-defer-response/$id', + op: 'function.remix.document_request', + }, + ], + }); + }); + + it('does not capture thrown redirect responses', async () => { + const env = await RemixTestEnv.init(); + const url = `${env.url}/throw-redirect`; + + const envelopesCount = await env.countEnvelopes({ + url, + envelopeType: 'event', + timeout: 3000, + }); + + expect(envelopesCount).toBe(0); + }); +}); diff --git a/packages/remix/test/integration/test/server/instrumentation-legacy/ssr.test.ts b/packages/remix/test/integration/test/server/instrumentation-legacy/ssr.test.ts new file mode 100644 index 000000000000..8400b9f9cff2 --- /dev/null +++ b/packages/remix/test/integration/test/server/instrumentation-legacy/ssr.test.ts @@ -0,0 +1,50 @@ +import { describe, expect, it } from 'vitest'; +import { RemixTestEnv, assertSentryEvent, assertSentryTransaction } from '../utils/helpers'; + +const useV2 = process.env.REMIX_VERSION === '2'; + +describe('Server Side Rendering', () => { + it('correctly reports a server side rendering error', async () => { + const env = await RemixTestEnv.init(); + const url = `${env.url}/ssr-error`; + const envelopes = await env.getMultipleEnvelopeRequest({ url, count: 2, envelopeType: ['transaction', 'event'] }); + const [transaction] = envelopes.filter(envelope => envelope[1].type === 'transaction'); + const [event] = envelopes.filter(envelope => envelope[1].type === 'event'); + assertSentryTransaction(transaction[2], { + contexts: { + trace: { + status: 'internal_error', + data: { + 'http.response.status_code': 500, + }, + }, + }, + tags: useV2 + ? { + // Testing that the wrapped `handleError` correctly adds tags + 'remix-test-tag': 'remix-test-value', + } + : {}, + }); + + assertSentryEvent(event[2], { + transaction: 'routes/ssr-error', + exception: { + values: [ + { + type: 'Error', + value: 'Sentry SSR Test Error', + stacktrace: expect.any(Object), + mechanism: { + data: { + function: useV2 ? 'remix.server.handleError' : 'documentRequest', + }, + handled: false, + type: 'instrument', + }, + }, + ], + }, + }); + }); +}); diff --git a/packages/remix/test/integration/test/server/action.test.ts b/packages/remix/test/integration/test/server/instrumentation-otel/action.test.ts similarity index 99% rename from packages/remix/test/integration/test/server/action.test.ts rename to packages/remix/test/integration/test/server/instrumentation-otel/action.test.ts index 940e54abd037..a784cd3b8d9c 100644 --- a/packages/remix/test/integration/test/server/action.test.ts +++ b/packages/remix/test/integration/test/server/instrumentation-otel/action.test.ts @@ -1,5 +1,5 @@ import { describe, it } from 'vitest'; -import { RemixTestEnv, assertSentryEvent, assertSentryTransaction } from './utils/helpers'; +import { RemixTestEnv, assertSentryEvent, assertSentryTransaction } from '../utils/helpers'; const useV2 = process.env.REMIX_VERSION === '2'; diff --git a/packages/remix/test/integration/test/server/loader.test.ts b/packages/remix/test/integration/test/server/instrumentation-otel/loader.test.ts similarity index 99% rename from packages/remix/test/integration/test/server/loader.test.ts rename to packages/remix/test/integration/test/server/instrumentation-otel/loader.test.ts index df701da9c782..62e0bf78ac10 100644 --- a/packages/remix/test/integration/test/server/loader.test.ts +++ b/packages/remix/test/integration/test/server/instrumentation-otel/loader.test.ts @@ -1,6 +1,6 @@ import { Event } from '@sentry/types'; import { describe, expect, it } from 'vitest'; -import { RemixTestEnv, assertSentryEvent, assertSentryTransaction } from './utils/helpers'; +import { RemixTestEnv, assertSentryEvent, assertSentryTransaction } from '../utils/helpers'; const useV2 = process.env.REMIX_VERSION === '2'; diff --git a/packages/remix/test/integration/test/server/ssr.test.ts b/packages/remix/test/integration/test/server/instrumentation-otel/ssr.test.ts similarity index 98% rename from packages/remix/test/integration/test/server/ssr.test.ts rename to packages/remix/test/integration/test/server/instrumentation-otel/ssr.test.ts index bab2e1857644..587e57abb1c3 100644 --- a/packages/remix/test/integration/test/server/ssr.test.ts +++ b/packages/remix/test/integration/test/server/instrumentation-otel/ssr.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from 'vitest'; -import { RemixTestEnv, assertSentryEvent, assertSentryTransaction } from './utils/helpers'; +import { RemixTestEnv, assertSentryEvent, assertSentryTransaction } from '../utils/helpers'; const useV2 = process.env.REMIX_VERSION === '2'; diff --git a/packages/remix/vitest.config.ts b/packages/remix/vitest.config.ts index 5240b036b083..980bdefe7309 100644 --- a/packages/remix/vitest.config.ts +++ b/packages/remix/vitest.config.ts @@ -4,5 +4,7 @@ export default defineConfig({ test: { globals: true, setupFiles: './test/integration/instrument.server.cjs', + include: + process.env.USE_OTEL === '1' ? ['**/instrumentation-otel/*.test.ts'] : ['**/instrumentation-legacy/*.test.ts'], }, }); From d4678b1e24f6b65eaa45558b3c5f05d1db63745b Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Fri, 7 Jun 2024 11:34:24 +0100 Subject: [PATCH 13/27] Fix broken request transactions. --- packages/node/src/index.ts | 2 + .../remix/{test/integration => }/.gitignore | 3 +- packages/remix/package.json | 2 +- packages/remix/src/utils/errors.ts | 14 +++-- packages/remix/src/utils/instrumentServer.ts | 57 ++++++++++++------- .../src/utils/integrations/opentelemetry.ts | 18 ++++-- .../test/integration/app_v1/entry.server.tsx | 4 ++ .../test/integration/app_v2/entry.server.tsx | 4 ++ packages/remix/test/integration/package.json | 4 +- packages/remix/vitest.config.ts | 2 + 10 files changed, 72 insertions(+), 38 deletions(-) rename packages/remix/{test/integration => }/.gitignore (80%) diff --git a/packages/node/src/index.ts b/packages/node/src/index.ts index 0003850d8f71..85d001b465e5 100644 --- a/packages/node/src/index.ts +++ b/packages/node/src/index.ts @@ -27,6 +27,8 @@ export { connectIntegration, setupConnectErrorHandler } from './integrations/tra export { spotlightIntegration } from './integrations/spotlight'; export { SentryContextManager } from './otel/contextManager'; +export { generateInstrumentOnce } from './otel/instrument'; + export { init, getDefaultIntegrations, diff --git a/packages/remix/test/integration/.gitignore b/packages/remix/.gitignore similarity index 80% rename from packages/remix/test/integration/.gitignore rename to packages/remix/.gitignore index 94f2cbbd8a42..3062ec3dd163 100644 --- a/packages/remix/test/integration/.gitignore +++ b/packages/remix/.gitignore @@ -1,6 +1,6 @@ node_modules -/.cache +test/integration/.cache /build /public/build .env @@ -8,3 +8,4 @@ node_modules /playwright-report/ /playwright/.cache/ yarn.lock + diff --git a/packages/remix/package.json b/packages/remix/package.json index 18b7d2584af9..5f15d8fbbd96 100644 --- a/packages/remix/package.json +++ b/packages/remix/package.json @@ -57,7 +57,7 @@ "access": "public" }, "dependencies": { - "@opentelemetry/instrumentation-http": "0.51.1", + "@opentelemetry/instrumentation-http": "0.52.0", "@remix-run/router": "1.x", "@sentry/cli": "^2.32.1", "@sentry/core": "8.9.2", diff --git a/packages/remix/src/utils/errors.ts b/packages/remix/src/utils/errors.ts index 5c68d0ceece8..32625ce54c72 100644 --- a/packages/remix/src/utils/errors.ts +++ b/packages/remix/src/utils/errors.ts @@ -3,7 +3,6 @@ import { captureException, getActiveSpan, getClient, - getCurrentScope, getRootSpan, handleCallbackErrors, spanToJSON, @@ -25,12 +24,15 @@ import { normalizeRemixRequest } from './web-fetch'; * * @returns A promise that resolves when the exception is captured. */ -export async function captureRemixServerException(err: unknown, name: string, request: Request): Promise { - const { IS_REMIX_V2 } = getCurrentScope().getScopeData().sdkProcessingMetadata; - +export async function captureRemixServerException( + err: unknown, + name: string, + request: Request, + isRemixV2: boolean = true, +): Promise { // Skip capturing if the thrown error is not a 5xx response // https://remix.run/docs/en/v1/api/conventions#throwing-responses-in-loaders - if (IS_REMIX_V2 && isRouteErrorResponse(err) && err.status < 500) { + if (isRemixV2 && isRouteErrorResponse(err) && err.status < 500) { return; } @@ -116,7 +118,7 @@ export function errorHandleDocumentRequestFunction( // We also skip primitives here, as we can't dedupe them, and also we don't expect any primitive SSR errors. if (!isRemixV2 && !isPrimitive(err)) { // eslint-disable-next-line @typescript-eslint/no-floating-promises - captureRemixServerException(err, 'documentRequest', request); + captureRemixServerException(err, 'documentRequest', request, isRemixV2); } throw err; diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index 297aad5d27bf..be9ed627d9e9 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -4,9 +4,10 @@ import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, getActiveSpan, - getGlobalScope, + getClient, getRootSpan, hasTracingEnabled, + setHttpStatus, spanToJSON, spanToTraceHeader, startSpan, @@ -34,6 +35,7 @@ import type { RemixRequest, RequestHandler, ServerBuild, + ServerRoute, ServerRouteManifest, } from './vendor/types'; import { normalizeRemixRequest } from './web-fetch'; @@ -171,7 +173,7 @@ function makeWrappedDataFunction( op: `function.remix.${name}`, name: id, attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.function.remix', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.ui.remix', name, }, }, @@ -268,6 +270,11 @@ function wrapRequestHandler( build: ServerBuild | (() => ServerBuild | Promise), autoInstrumentRemix: boolean, ): RequestHandler { + let resolvedBuild: ServerBuild; + let routes: ServerRoute[]; + let name: string; + let source: TransactionSource; + return async function (this: unknown, request: RemixRequest, loadContext?: AppLoadContext): Promise { const upperCaseMethod = request.method.toUpperCase(); // We don't want to wrap OPTIONS and HEAD requests @@ -275,26 +282,19 @@ function wrapRequestHandler( return origRequestHandler.call(this, request, loadContext); } - let name: string; - let source: TransactionSource; - if (!autoInstrumentRemix) { - let resolvedBuild: ServerBuild; - if (typeof build === 'function') { resolvedBuild = await build(); } else { resolvedBuild = build; } - const routes = createRoutes(resolvedBuild.routes); - - // const routes = createRoutes(build.routes); - const url = new URL(request.url); - [name, source] = getTransactionName(routes, url); + routes = createRoutes(resolvedBuild.routes); } return withIsolationScope(async isolationScope => { + const options = getClient()?.getOptions(); + let normalizedRequest: Record = request; try { @@ -302,12 +302,27 @@ function wrapRequestHandler( } catch (e) { DEBUG_BUILD && logger.warn('Failed to normalize Remix request'); } + + if (!autoInstrumentRemix) { + const url = new URL(request.url); + [name, source] = getTransactionName(routes, url); + + isolationScope.setTransactionName(name); + } + isolationScope.setSDKProcessingMetadata({ request: { ...normalizedRequest, + route: { + path: name, + }, }, }); + if (!options || !hasTracingEnabled(options)) { + return origRequestHandler.call(this, request, loadContext); + } + return continueTrace( { sentryTrace: request.headers.get('sentry-trace') || '', @@ -325,8 +340,14 @@ function wrapRequestHandler( method: request.method, }, }, - () => { - return origRequestHandler.call(this, request, loadContext); + async span => { + const res = (await origRequestHandler.call(this, request, loadContext)) as Response; + + if (isResponse(res)) { + setHttpStatus(span, res.status); + } + + return res; }, ); } @@ -340,15 +361,7 @@ function wrapRequestHandler( function instrumentBuildCallback(build: ServerBuild, autoInstrumentRemix: boolean): ServerBuild { const routes: ServerRouteManifest = {}; - const remixVersion = getRemixVersionFromBuild(build); - - const globalScope = getGlobalScope(); - - globalScope.setSDKProcessingMetadata({ - IS_REMIX_V2: remixVersion === 2, - }); - const wrappedEntry = { ...build.entry, module: { ...build.entry.module } }; // Not keeping boolean flags like it's done for `requestHandler` functions, diff --git a/packages/remix/src/utils/integrations/opentelemetry.ts b/packages/remix/src/utils/integrations/opentelemetry.ts index e06ca10f0ccd..5168cf5adcbd 100644 --- a/packages/remix/src/utils/integrations/opentelemetry.ts +++ b/packages/remix/src/utils/integrations/opentelemetry.ts @@ -1,19 +1,25 @@ import { RemixInstrumentation } from 'opentelemetry-instrumentation-remix'; import { defineIntegration } from '@sentry/core'; -import { SEMANTIC_ATTRIBUTE_SENTRY_OP, addOpenTelemetryInstrumentation, spanToJSON } from '@sentry/node'; +import { SEMANTIC_ATTRIBUTE_SENTRY_OP, generateInstrumentOnce, spanToJSON } from '@sentry/node'; import type { Client, IntegrationFn, Span } from '@sentry/types'; import type { RemixOptions } from '../remixOptions'; +const INTEGRATION_NAME = 'Remix'; + +const instrumentRemix = generateInstrumentOnce( + INTEGRATION_NAME, + (_options?: RemixOptions) => + new RemixInstrumentation({ + actionFormDataAttributes: _options?.sendDefaultPii ? _options?.captureActionFormDataKeys : undefined, + }), +); + const _remixIntegration = ((options?: RemixOptions) => { return { name: 'Remix', setupOnce() { - addOpenTelemetryInstrumentation( - new RemixInstrumentation({ - actionFormDataAttributes: options?.sendDefaultPii ? options?.captureActionFormDataKeys : undefined, - }), - ); + instrumentRemix(options); }, setup(client: Client) { diff --git a/packages/remix/test/integration/app_v1/entry.server.tsx b/packages/remix/test/integration/app_v1/entry.server.tsx index 6db43d935cdc..9ecf5f467588 100644 --- a/packages/remix/test/integration/app_v1/entry.server.tsx +++ b/packages/remix/test/integration/app_v1/entry.server.tsx @@ -1,3 +1,7 @@ +if (process.env.USE_OTEL !== '1') { + require('../instrument.server.cjs'); +} + import type { EntryContext } from '@remix-run/node'; import { RemixServer } from '@remix-run/react'; import { renderToString } from 'react-dom/server'; diff --git a/packages/remix/test/integration/app_v2/entry.server.tsx b/packages/remix/test/integration/app_v2/entry.server.tsx index 86b0312eb92f..968ec19a5f59 100644 --- a/packages/remix/test/integration/app_v2/entry.server.tsx +++ b/packages/remix/test/integration/app_v2/entry.server.tsx @@ -1,3 +1,7 @@ +if (process.env.USE_OTEL !== '1') { + require('../instrument.server.cjs'); +} + import * as Sentry from '@sentry/remix'; import type { EntryContext } from '@remix-run/node'; diff --git a/packages/remix/test/integration/package.json b/packages/remix/test/integration/package.json index 68ab036c875d..1bf410447ed6 100644 --- a/packages/remix/test/integration/package.json +++ b/packages/remix/test/integration/package.json @@ -3,8 +3,8 @@ "sideEffects": false, "scripts": { "build": "remix build", - "dev": "NODE_OPTIONS='--require ./instrument.server.cjs' remix dev", - "start": "NODE_OPTIONS='--require ./instrument.server.cjs' && remix-serve ./build/index.js" + "dev": "remix dev", + "start":"remix-serve ./build/index.js" }, "dependencies": { "@remix-run/express": "1.17.0", diff --git a/packages/remix/vitest.config.ts b/packages/remix/vitest.config.ts index 980bdefe7309..1c4092a5c31e 100644 --- a/packages/remix/vitest.config.ts +++ b/packages/remix/vitest.config.ts @@ -3,6 +3,8 @@ import { defineConfig } from 'vitest/config'; export default defineConfig({ test: { globals: true, + disableConsoleIntercept: true, + silent: false, setupFiles: './test/integration/instrument.server.cjs', include: process.env.USE_OTEL === '1' ? ['**/instrumentation-otel/*.test.ts'] : ['**/instrumentation-legacy/*.test.ts'], From d9de72ac75aeb5b37a581d1125d3f2850a44368e Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Fri, 7 Jun 2024 17:26:21 +0100 Subject: [PATCH 14/27] Use Remix `httpIntegration` for both otel and legacy instrumentations. --- packages/remix/playwright.config.ts | 2 +- packages/remix/src/index.server.ts | 8 ++++---- packages/remix/src/utils/errors.ts | 4 ++-- packages/remix/src/utils/instrumentServer.ts | 5 +++-- packages/remix/test/integration/package.json | 2 +- .../test/server/instrumentation-legacy/action.test.ts | 5 ++--- packages/remix/vitest.config.ts | 7 ++++--- 7 files changed, 17 insertions(+), 16 deletions(-) diff --git a/packages/remix/playwright.config.ts b/packages/remix/playwright.config.ts index 596f82334b71..72e9bd749a52 100644 --- a/packages/remix/playwright.config.ts +++ b/packages/remix/playwright.config.ts @@ -13,7 +13,7 @@ const config: PlaywrightTestConfig = { workers: process.env.CI ? 3 : undefined, webServer: { env: { - NODE_OPTIONS: '--require ./instrument.server.cjs', + NODE_OPTIONS: process.env.USE_OTEL === '1' ? '--require ./instrument.server.cjs' : '', }, command: '(cd test/integration/ && yarn build && yarn start)', port: 3000, diff --git a/packages/remix/src/index.server.ts b/packages/remix/src/index.server.ts index cdc10e4e8a9d..d1d24ec5d297 100644 --- a/packages/remix/src/index.server.ts +++ b/packages/remix/src/index.server.ts @@ -140,7 +140,6 @@ export function getRemixDefaultIntegrations(options: RemixOptions): Integration[ return [ ...getDefaultNodeIntegrations(options as NodeOptions).filter(integration => integration.name !== 'Http'), httpIntegration(), - remixIntegration(options), ]; } @@ -168,9 +167,10 @@ export function init(options: RemixOptions): void { return; } - if (options.autoInstrumentRemix) { - options.defaultIntegrations = getRemixDefaultIntegrations(options as NodeOptions); - } + options.defaultIntegrations = [ + ...getRemixDefaultIntegrations(options as NodeOptions), + options.autoInstrumentRemix ? remixIntegration() : undefined, + ].filter(int => int) as Integration[]; nodeInit(options as NodeOptions); diff --git a/packages/remix/src/utils/errors.ts b/packages/remix/src/utils/errors.ts index 32625ce54c72..c5344cc9bf5b 100644 --- a/packages/remix/src/utils/errors.ts +++ b/packages/remix/src/utils/errors.ts @@ -7,6 +7,7 @@ import { handleCallbackErrors, spanToJSON, } from '@sentry/core'; +import type { Span } from '@sentry/types'; import { addExceptionMechanism, isPrimitive, logger, objectify } from '@sentry/utils'; import { DEBUG_BUILD } from './debug-build'; import type { RemixOptions } from './remixOptions'; @@ -135,11 +136,10 @@ export async function errorHandleDataFunction( name: string, args: DataFunctionArgs, isRemixV2: boolean, + span?: Span, ): Promise { return handleCallbackErrors( async () => { - const span = getActiveSpan(); - if (name === 'action' && span) { const options = getClient()?.getOptions() as RemixOptions; diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index be9ed627d9e9..e83c14dfbbc4 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -15,6 +15,7 @@ import { } from '@sentry/core'; import { continueTrace, getDynamicSamplingContextFromSpan } from '@sentry/opentelemetry'; import type { TransactionSource, WrappedFunction } from '@sentry/types'; +import type { Span } from '@sentry/types'; import { dynamicSamplingContextToSentryBaggageHeader, fill, isNodeEnv, loadModule, logger } from '@sentry/utils'; import { DEBUG_BUILD } from './debug-build'; @@ -177,8 +178,8 @@ function makeWrappedDataFunction( name, }, }, - () => { - return errorHandleDataFunction.call(this, origFn, name, args, isRemixV2); + (span: Span) => { + return errorHandleDataFunction.call(this, origFn, name, args, isRemixV2, span); }, ); } else { diff --git a/packages/remix/test/integration/package.json b/packages/remix/test/integration/package.json index 1bf410447ed6..4f954e1b4eea 100644 --- a/packages/remix/test/integration/package.json +++ b/packages/remix/test/integration/package.json @@ -4,7 +4,7 @@ "scripts": { "build": "remix build", "dev": "remix dev", - "start":"remix-serve ./build/index.js" + "start":"remix-serve build" }, "dependencies": { "@remix-run/express": "1.17.0", diff --git a/packages/remix/test/integration/test/server/instrumentation-legacy/action.test.ts b/packages/remix/test/integration/test/server/instrumentation-legacy/action.test.ts index a53d3953417b..572b0a011fb2 100644 --- a/packages/remix/test/integration/test/server/instrumentation-legacy/action.test.ts +++ b/packages/remix/test/integration/test/server/instrumentation-legacy/action.test.ts @@ -7,13 +7,12 @@ describe('Remix API Actions', () => { it('correctly instruments a parameterized Remix API action', async () => { const env = await RemixTestEnv.init(); const url = `${env.url}/action-json-response/123123`; - const envelopes = await env.getMultipleEnvelopeRequest({ + const envelope = await env.getEnvelopeRequest({ url, method: 'post', envelopeType: 'transaction', - count: 1, }); - const transaction = envelopes[0][2]; + const transaction = envelope[2]; assertSentryTransaction(transaction, { transaction: `routes/action-json-response${useV2 ? '.' : '/'}$id`, diff --git a/packages/remix/vitest.config.ts b/packages/remix/vitest.config.ts index 1c4092a5c31e..23c2383b9e8b 100644 --- a/packages/remix/vitest.config.ts +++ b/packages/remix/vitest.config.ts @@ -1,12 +1,13 @@ import { defineConfig } from 'vitest/config'; +const useOtel = process.env.USE_OTEL === '1'; + export default defineConfig({ test: { globals: true, disableConsoleIntercept: true, silent: false, - setupFiles: './test/integration/instrument.server.cjs', - include: - process.env.USE_OTEL === '1' ? ['**/instrumentation-otel/*.test.ts'] : ['**/instrumentation-legacy/*.test.ts'], + setupFiles: useOtel ? './test/integration/instrument.server.cjs' : undefined, + include: useOtel ? ['**/instrumentation-otel/*.test.ts'] : ['**/instrumentation-legacy/*.test.ts'], }, }); From b39929de74039b6516fc226b56a5754dc526384b Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Mon, 10 Jun 2024 12:13:03 +0100 Subject: [PATCH 15/27] Add Remix v1 legacy e2e tests. --- .github/workflows/build.yml | 1 + .../create-remix-app-legacy/.eslintrc.js | 4 + .../create-remix-app-legacy/.gitignore | 6 + .../create-remix-app-legacy/.npmrc | 2 + .../app/entry.client.tsx | 46 +++++ .../app/entry.server.tsx | 114 +++++++++++ .../create-remix-app-legacy/app/root.tsx | 76 +++++++ .../app/routes/_index.tsx | 27 +++ .../app/routes/client-error.tsx | 13 ++ .../app/routes/navigate.tsx | 20 ++ .../app/routes/user.$id.tsx | 3 + .../create-remix-app-legacy/globals.d.ts | 7 + .../create-remix-app-legacy/package.json | 38 ++++ .../playwright.config.mjs | 7 + .../create-remix-app-legacy/remix.config.js | 17 ++ .../start-event-proxy.mjs | 6 + .../tests/behaviour-client.test.ts | 192 ++++++++++++++++++ .../tests/behaviour-server.test.ts | 50 +++++ .../create-remix-app-legacy/tsconfig.json | 22 ++ .../upload-sourcemaps.sh | 3 + .../create-remix-app/package.json | 2 +- .../create-remix-app/playwright.config.ts | 66 ------ 22 files changed, 655 insertions(+), 67 deletions(-) create mode 100644 dev-packages/e2e-tests/test-applications/create-remix-app-legacy/.eslintrc.js create mode 100644 dev-packages/e2e-tests/test-applications/create-remix-app-legacy/.gitignore create mode 100644 dev-packages/e2e-tests/test-applications/create-remix-app-legacy/.npmrc create mode 100644 dev-packages/e2e-tests/test-applications/create-remix-app-legacy/app/entry.client.tsx create mode 100644 dev-packages/e2e-tests/test-applications/create-remix-app-legacy/app/entry.server.tsx create mode 100644 dev-packages/e2e-tests/test-applications/create-remix-app-legacy/app/root.tsx create mode 100644 dev-packages/e2e-tests/test-applications/create-remix-app-legacy/app/routes/_index.tsx create mode 100644 dev-packages/e2e-tests/test-applications/create-remix-app-legacy/app/routes/client-error.tsx create mode 100644 dev-packages/e2e-tests/test-applications/create-remix-app-legacy/app/routes/navigate.tsx create mode 100644 dev-packages/e2e-tests/test-applications/create-remix-app-legacy/app/routes/user.$id.tsx create mode 100644 dev-packages/e2e-tests/test-applications/create-remix-app-legacy/globals.d.ts create mode 100644 dev-packages/e2e-tests/test-applications/create-remix-app-legacy/package.json create mode 100644 dev-packages/e2e-tests/test-applications/create-remix-app-legacy/playwright.config.mjs create mode 100644 dev-packages/e2e-tests/test-applications/create-remix-app-legacy/remix.config.js create mode 100644 dev-packages/e2e-tests/test-applications/create-remix-app-legacy/start-event-proxy.mjs create mode 100644 dev-packages/e2e-tests/test-applications/create-remix-app-legacy/tests/behaviour-client.test.ts create mode 100644 dev-packages/e2e-tests/test-applications/create-remix-app-legacy/tests/behaviour-server.test.ts create mode 100644 dev-packages/e2e-tests/test-applications/create-remix-app-legacy/tsconfig.json create mode 100755 dev-packages/e2e-tests/test-applications/create-remix-app-legacy/upload-sourcemaps.sh delete mode 100644 dev-packages/e2e-tests/test-applications/create-remix-app/playwright.config.ts diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 6f1159480d2d..d894b0b35477 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -1035,6 +1035,7 @@ jobs: 'create-react-app', 'create-next-app', 'create-remix-app', + 'create-remix-app-legacy', 'create-remix-app-v2', 'create-remix-app-express', 'create-remix-app-express-vite-dev', diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-legacy/.eslintrc.js b/dev-packages/e2e-tests/test-applications/create-remix-app-legacy/.eslintrc.js new file mode 100644 index 000000000000..f2faf1470fd8 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/create-remix-app-legacy/.eslintrc.js @@ -0,0 +1,4 @@ +/** @type {import('eslint').Linter.Config} */ +module.exports = { + extends: ['@remix-run/eslint-config', '@remix-run/eslint-config/node'], +}; diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-legacy/.gitignore b/dev-packages/e2e-tests/test-applications/create-remix-app-legacy/.gitignore new file mode 100644 index 000000000000..3f7bf98da3e1 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/create-remix-app-legacy/.gitignore @@ -0,0 +1,6 @@ +node_modules + +/.cache +/build +/public/build +.env diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-legacy/.npmrc b/dev-packages/e2e-tests/test-applications/create-remix-app-legacy/.npmrc new file mode 100644 index 000000000000..070f80f05092 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/create-remix-app-legacy/.npmrc @@ -0,0 +1,2 @@ +@sentry:registry=http://127.0.0.1:4873 +@sentry-internal:registry=http://127.0.0.1:4873 diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-legacy/app/entry.client.tsx b/dev-packages/e2e-tests/test-applications/create-remix-app-legacy/app/entry.client.tsx new file mode 100644 index 000000000000..93eab0f819fb --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/create-remix-app-legacy/app/entry.client.tsx @@ -0,0 +1,46 @@ +/** + * By default, Remix will handle hydrating your app on the client for you. + * You are free to delete this file if you'd like to, but if you ever want it revealed again, you can run `npx remix reveal` ✨ + * For more information, see https://remix.run/file-conventions/entry.client + */ + +import { RemixBrowser, useLocation, useMatches } from '@remix-run/react'; +import * as Sentry from '@sentry/remix'; +import { StrictMode, startTransition, useEffect } from 'react'; +import { hydrateRoot } from 'react-dom/client'; + +Sentry.init({ + environment: 'qa', // dynamic sampling bias to keep transactions + dsn: window.ENV.SENTRY_DSN, + integrations: [Sentry.browserTracingIntegration({ useEffect, useMatches, useLocation }), Sentry.replayIntegration()], + // Performance Monitoring + tracesSampleRate: 1.0, // Capture 100% of the transactions, reduce in production! + // Session Replay + replaysSessionSampleRate: 0.1, // This sets the sample rate at 10%. You may want to change it to 100% while in development and then sample at a lower rate in production. + replaysOnErrorSampleRate: 1.0, // If you're not already sampling the entire session, change the sample rate to 100% when sampling sessions where errors occur. + tunnel: 'http://localhost:3031/', // proxy server +}); + +Sentry.addEventProcessor(event => { + if ( + event.type === 'transaction' && + (event.contexts?.trace?.op === 'pageload' || event.contexts?.trace?.op === 'navigation') + ) { + const eventId = event.event_id; + if (eventId) { + window.recordedTransactions = window.recordedTransactions || []; + window.recordedTransactions.push(eventId); + } + } + + return event; +}); + +startTransition(() => { + hydrateRoot( + document, + + + , + ); +}); diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-legacy/app/entry.server.tsx b/dev-packages/e2e-tests/test-applications/create-remix-app-legacy/app/entry.server.tsx new file mode 100644 index 000000000000..b0f1c5d19f09 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/create-remix-app-legacy/app/entry.server.tsx @@ -0,0 +1,114 @@ +import * as Sentry from '@sentry/remix'; + +Sentry.init({ + tracesSampleRate: 1.0, // Capture 100% of the transactions, reduce in production! + environment: 'qa', // dynamic sampling bias to keep transactions + dsn: process.env.E2E_TEST_DSN, + tunnel: 'http://localhost:3031/', // proxy server +}); + +/** + * By default, Remix will handle generating the HTTP Response for you. + * You are free to delete this file if you'd like to, but if you ever want it revealed again, you can run `npx remix reveal` ✨ + * For more information, see https://remix.run/file-conventions/entry.server + */ + +import { PassThrough } from 'node:stream'; + +import type { AppLoadContext, EntryContext } from '@remix-run/node'; +import { Response } from '@remix-run/node'; +import { RemixServer } from '@remix-run/react'; +import isbot from 'isbot'; +import { renderToPipeableStream } from 'react-dom/server'; + +const ABORT_DELAY = 5_000; + +export const handleError = Sentry.wrapRemixHandleError; + +export default function handleRequest( + request: Request, + responseStatusCode: number, + responseHeaders: Headers, + remixContext: EntryContext, + loadContext: AppLoadContext, +) { + return isbot(request.headers.get('user-agent')) + ? handleBotRequest(request, responseStatusCode, responseHeaders, remixContext) + : handleBrowserRequest(request, responseStatusCode, responseHeaders, remixContext); +} + +function handleBotRequest( + request: Request, + responseStatusCode: number, + responseHeaders: Headers, + remixContext: EntryContext, +) { + return new Promise((resolve, reject) => { + const { pipe, abort } = renderToPipeableStream( + , + { + onAllReady() { + const body = new PassThrough(); + + responseHeaders.set('Content-Type', 'text/html'); + + resolve( + new Response(body, { + headers: responseHeaders, + status: responseStatusCode, + }), + ); + + pipe(body); + }, + onShellError(error: unknown) { + reject(error); + }, + onError(error: unknown) { + responseStatusCode = 500; + console.error(error); + }, + }, + ); + + setTimeout(abort, ABORT_DELAY); + }); +} + +function handleBrowserRequest( + request: Request, + responseStatusCode: number, + responseHeaders: Headers, + remixContext: EntryContext, +) { + return new Promise((resolve, reject) => { + const { pipe, abort } = renderToPipeableStream( + , + { + onShellReady() { + const body = new PassThrough(); + + responseHeaders.set('Content-Type', 'text/html'); + + resolve( + new Response(body, { + headers: responseHeaders, + status: responseStatusCode, + }), + ); + + pipe(body); + }, + onShellError(error: unknown) { + reject(error); + }, + onError(error: unknown) { + console.error(error); + responseStatusCode = 500; + }, + }, + ); + + setTimeout(abort, ABORT_DELAY); + }); +} diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-legacy/app/root.tsx b/dev-packages/e2e-tests/test-applications/create-remix-app-legacy/app/root.tsx new file mode 100644 index 000000000000..e99991f5994d --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/create-remix-app-legacy/app/root.tsx @@ -0,0 +1,76 @@ +import { cssBundleHref } from '@remix-run/css-bundle'; +import { LinksFunction, MetaFunction, json } from '@remix-run/node'; +import { + Links, + LiveReload, + Meta, + Outlet, + Scripts, + ScrollRestoration, + useLoaderData, + useRouteError, +} from '@remix-run/react'; +import { captureRemixErrorBoundaryError, withSentry } from '@sentry/remix'; + +export const links: LinksFunction = () => [...(cssBundleHref ? [{ rel: 'stylesheet', href: cssBundleHref }] : [])]; + +export const loader = () => { + return json({ + ENV: { + SENTRY_DSN: process.env.E2E_TEST_DSN, + }, + }); +}; + +export const meta: MetaFunction = ({ data }) => { + return [ + { + name: 'sentry-trace', + content: data.sentryTrace, + }, + { + name: 'baggage', + content: data.sentryBaggage, + }, + ]; +}; + +export function ErrorBoundary() { + const error = useRouteError(); + const eventId = captureRemixErrorBoundaryError(error); + + return ( +
+ ErrorBoundary Error + {eventId} +
+ ); +} + +function App() { + const { ENV } = useLoaderData(); + + return ( + + + + +