feat(remix): Migrate to `opentelemetry-instrumentation-remix`. #12110

onurtemizkan · 2024-05-17T14:43:26Z

Migrates Remix server-side SDK to opentelemetry-instrumentation-remix.

This PR also keeps the original implementation not the break the developer experience for non-Express Remix projects.
Remix projects using Express are supported as is using the new autoInstrumentRemix option.

Usage with Express:

// instrument.(cjs | mjs)
const Sentry = require('@sentry/remix');

Sentry.init({
  dsn: YOUR_DSN
  // ...
  // auto instrument Remix with OpenTelemetry
  autoInstrumentRemix: true,
  // Optionally capture action formData attributes with errors.
  // This requires `sendDefaultPii` set to true as well.
  captureActionFormDataKeys: {
    file: true,
    text: true,
  },
  // To capture action formData attributes.
  sendDefaultPii: true
});

// server.(cjs | mjs)
// import the Sentry instrumentation file before anything else.
import './instrument.cjs';
// alternatively `require('./instrument.cjs')`

// ...

const app = express();

// ...

Usage with built-in Remix server:

You need to run the Remix server with NODE_OPTIONS=--require(...)` set.

// package.json

// ...
  "scripts": {
    "start": "NODE_OPTIONS='--require=./instrument.server.cjs' remix-serve build"
    // or
    "start": "NODE_OPTIONS='--import=./instrument.server.mjs' remix-serve build"
  }
// ...

This PR removes:

Express server adapter.
There is no need to use wrapExpressCreateRequestHandler anymore even if you don't opt-in to autoInstrumentRemix. wrapExpressCreateRequestHandler is kept exported as a no-op function.
Built in HTTP incoming request instrumentation for both legacy and otel implementations. Instead we mark requestHandler spans as the root http.server spans.

When autoInstrumentRemix is set to true, this update replaces:

Performance tracing on action / loader / documentRequest functions. Leaving them to be traced by opentelemetry-instrumentation-remix
Request handler instrumentation as they are also traced by opentelemetry-instrumentation-remix
Auto-instrumentation for http as default integration for Remix SDK.
With the new instrumentation, pageload span is the child of the loader span. (Example: Trace)
Legacy instrumentation keeps recording pageload span as the child of the http.server span. (Example: Trace)

Also:

Migrates Remix integration tests from Jest to Vitest.

Related issues: [Bug]: OpenTelemetry Span Not Being Captured in Jest Tests with Instrumentation (But it works with other test runners) jestjs/jest#15033 Can't be tested with Jest elastic/require-in-the-middle#50

Fixes Backlogged Issues:

packages/remix/src/index.server.ts

.github/workflows/build.yml

mydea · 2024-05-22T08:57:36Z

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' &&


hmm, we should fix this to be http.server - this is important I believe!

Marked requestHandler spans as http.server 👍

mydea · 2024-05-22T08:58:53Z

packages/remix/src/index.server.ts

+ */
+export function getDefaultIntegrations(options: RemixOptions): Integration[] {
+  return [
+    ...getDefaultNodeIntegrations(options).filter(integration => integration.name !== 'Http'),


Hmm, do we still instrument outgoing requests without this? Do we really need to remove this? Most otel instrumentation is on top of this...?

Http instrumentation's functionality is covered by requestHandler spans by the opentelemetry-instrumentation-remix.

In this context, HTTP instrumentation also ends up setting an unparameterised route as the root transaction name as I've seen from the tests.

Does the http span become the parent of the remix request handler spans?

If so, could we just extend the HTTP integration for remix to filter out server spans by default? So we still get instrumentation for outgoing requests?

Otherwise I wonder if we should actually vendor in https://github.com/justindsmith/opentelemetry-instrumentations-js/blob/main/packages/instrumentation-remix/src/instrumentation.ts and change it that way?

Yes, makes sense to me, just extended the Http integration the same way it's done in Next.js. Also marked requestHandler spans as http.server.

packages/remix/src/utils/integrations/opentelemetry.ts

mydea · 2024-05-22T09:01:22Z

dev-packages/e2e-tests/test-applications/create-remix-app/playwright.config.ts

@@ -57,7 +57,7 @@ const config: PlaywrightTestConfig = {
      port: eventProxyPort,
    },
    {
-      command: `PORT=${port} pnpm start`,
+      command: `PORT=${port} NODE_OPTIONS='--require ./instrument.server.cjs' pnpm start`,


Is it necessary to run it like this, does it not work with require on top? Just to clarify!

Yes, we need to run it like this. esbuild does not bundle the instrumentation inside the entry files correctly.

hmmm I wonder if this qualifies as a breaking change 😬 thoughts cc @AbhiPrasad ?

I think this does qualify as a breaking change 😬

I guess we have to opt-in to this mode, we can't break the people who already put the work in to upgrade their sdk to v8.

We can make this the default during onboarding though.

I think that makes sense. So I would say we do:

Expose remixIntegration but do not add it by default

Expose remixHttpIntegration but do not add it by default

Users have to add both of these if they want to opt-in (the latter "overwrites" the default http integration)

Does that sound good? 🤔

Questions that remain:

a. Can we find a way to make 2 obsolete? Can't think of a good way to do it sadly 😬
b. Should we then (eventually) document both paths, or just the "new" path? I guess the "old" way could remain as an alternative installation method...?

Can we try adding another init option like useOtel to switch to this mode, and initialise the integrations without (or also with) exposing them?

This change also doesn't affect Express servers (or I guess any other custom server), a top-level import on the server file works in those cases. So we can still get rid of the Express adapter instrumentation, and keep the core instrumentation as an alternative for built-in server users (which I also guess is not very popular among Remix users looking at our issue reports)

I have updated the e2e tests using Express custom servers. (0e2f73a)

Yes, that sounds good - let's make an option for this and adjust default integrations based on this!

AbhiPrasad · 2024-05-31T14:48:46Z

@onurtemizkan could you add the new setup instructions to the readme of this PR? and also add a short note about what the migration looks like?

mydea · 2024-06-04T08:41:05Z

packages/remix/src/index.server.ts

+export function getDefaultIntegrations(options: RemixOptions): Integration[] {
+  return [
+    ...getDefaultNodeIntegrations(options).filter(integration => integration.name !== 'Http'),
+    httpIntegration(),


Let's refactor this to something like:

function getRemixDefaultIntegrations() { return [...] } // further down: if (options.autoInstrumentRemix) { options.defaultIntegrations = getRemixDefaultIntegrations(options); } else { instrumentServer(); }

This way users can opt in to the new behavior, but keep the old behavior working if needed?

This also means that wrapExpressCreateRequestHandler should probably remain as it was before, users need to opt-in to not use that anymore...?

Skipping incoming requests worked well both on legacy and otel implementations. I think I managed to keep the old behaviour, without requiring wrapExpressCreateRequestHandler, creating http.server span manually.
Dropping a sample event here

mydea · 2024-06-04T08:42:17Z

packages/remix/src/utils/integrations/opentelemetry.ts

+    name: 'Remix',
+    setupOnce() {
+      addOpenTelemetryInstrumentation(
+        new RemixInstrumentation({


m: Let's re-write this based on the changes done in #12213, which means extracting this into a instrumentRemix method which is called in setupOnce. You'll have to put the options into module scope so they can be updated later...!

github-actions · 2024-06-11T09:29:00Z

size-limit report 📦

Path	Size
@sentry/browser	22.04 KB (0%)
@sentry/browser (incl. Tracing)	33.23 KB (0%)
@sentry/browser (incl. Tracing, Replay)	68.95 KB (0%)
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags	62.27 KB (0%)
@sentry/browser (incl. Tracing, Replay with Canvas)	73.02 KB (0%)
@sentry/browser (incl. Tracing, Replay, Feedback)	85.14 KB (0%)
@sentry/browser (incl. Tracing, Replay, Feedback, metrics)	86.98 KB (0%)
@sentry/browser (incl. metrics)	26.23 KB (0%)
@sentry/browser (incl. Feedback)	38.21 KB (0%)
@sentry/browser (incl. sendFeedback)	26.63 KB (0%)
@sentry/browser (incl. FeedbackAsync)	31.18 KB (0%)
@sentry/react	24.81 KB (0%)
@sentry/react (incl. Tracing)	36.27 KB (0%)
@sentry/vue	26.05 KB (0%)
@sentry/vue (incl. Tracing)	35.07 KB (0%)
@sentry/svelte	22.17 KB (0%)
CDN Bundle	23.39 KB (0%)
CDN Bundle (incl. Tracing)	34.91 KB (0%)
CDN Bundle (incl. Tracing, Replay)	69.02 KB (0%)
CDN Bundle (incl. Tracing, Replay, Feedback)	74.15 KB (0%)
CDN Bundle - uncompressed	68.71 KB (0%)
CDN Bundle (incl. Tracing) - uncompressed	103.3 KB (0%)
CDN Bundle (incl. Tracing, Replay) - uncompressed	213.75 KB (0%)
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed	226.21 KB (0%)
@sentry/nextjs (client)	35.63 KB (0%)
@sentry/sveltekit (client)	33.86 KB (0%)
@sentry/node	111.93 KB (0%)
@sentry/node - without tracing	89.4 KB (0%)
@sentry/aws-serverless	98.46 KB (0%)

onurtemizkan force-pushed the onur/remix-otel-migration branch 3 times, most recently from 81e86d3 to 8941895 Compare May 20, 2024 11:13

onurtemizkan requested review from mydea and Lms24 May 20, 2024 11:46

onurtemizkan marked this pull request as ready for review May 20, 2024 11:46

mydea reviewed May 21, 2024

View reviewed changes

packages/remix/src/index.server.ts Show resolved Hide resolved

onurtemizkan force-pushed the onur/remix-otel-migration branch from e5d54a3 to fb620b8 Compare May 21, 2024 10:06

mydea requested a review from AbhiPrasad May 22, 2024 07:28

mydea reviewed May 22, 2024

View reviewed changes

.github/workflows/build.yml Show resolved Hide resolved

mydea reviewed May 22, 2024

View reviewed changes

packages/remix/src/utils/integrations/opentelemetry.ts Outdated Show resolved Hide resolved

mydea reviewed May 22, 2024

View reviewed changes

onurtemizkan force-pushed the onur/remix-otel-migration branch 2 times, most recently from 3ec1138 to 06a185e Compare May 24, 2024 15:56

AbhiPrasad mentioned this pull request May 29, 2024

Adding sentry to Remix increases latency by a lot #12285

Open

3 tasks

onurtemizkan force-pushed the onur/remix-otel-migration branch from 06a185e to 0e2f73a Compare May 29, 2024 17:35

mydea mentioned this pull request Jun 4, 2024

feat(remix): Add Fastify server adapter #11261

Open

2 tasks

mydea reviewed Jun 4, 2024

View reviewed changes

onurtemizkan force-pushed the onur/remix-otel-migration branch 6 times, most recently from 39d7148 to cd0d656 Compare June 10, 2024 15:09

feat(remix): Migrate to opentelemetry-instrumentation-remix.

b17e28d

onurtemizkan added 21 commits June 11, 2024 10:40

Fix linter.

541eb52

Return jest config for unit tests back.

affc59d

Don't run Remix integration tests on Node 14

75edb91

Instrument function builds properly.

a351f28

Add ErrorBoundary propagation test.

a627334

Export a no-op function in place of wrapExpressCreateRequestHandler.

049c4bc

Add a custom httpIntegration for Remix.

765a18e

Switch to import pattern to load instrumentation on Express tests

28873db

Remove extra Sentry.inits in v2 e2e test app.

f49dbeb

Make auto-instrumentation opt-in.

08e66ee

Add legacy integration tests back.

e9f91d0

Fix broken request transactions.

3847232

Use Remix httpIntegration for both otel and legacy instrumentations.

755e015

Add Remix v1 legacy e2e tests.

92b1f6a

Add Remix v2 legacy e2e tests.

546d8a9

Test error boundary rendering on v2 integration tests.

372c707

Add Remix/Express legacy e2e tests.

97c00ed

Fix passing client options.

3355979

Fix typo

d346621

Fix test init

390c9b7

Add missing generateInstrumentOnce exports.

8c9330a

onurtemizkan force-pushed the onur/remix-otel-migration branch from 0bd6f5e to 8c9330a Compare June 11, 2024 09:40

onurtemizkan added 2 commits June 11, 2024 10:49

Update JSDocs.

a1d8a9c

Add missing generateInstrumentOnce export to @sentry/astro.

d04f8db

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(remix): Migrate to `opentelemetry-instrumentation-remix`. #12110

feat(remix): Migrate to `opentelemetry-instrumentation-remix`. #12110

onurtemizkan commented May 17, 2024 •

edited

mydea May 22, 2024

onurtemizkan May 24, 2024

mydea May 22, 2024

onurtemizkan May 23, 2024

AbhiPrasad May 24, 2024

onurtemizkan May 24, 2024

mydea May 22, 2024

onurtemizkan May 24, 2024

mydea May 27, 2024

AbhiPrasad May 27, 2024

AbhiPrasad May 27, 2024

mydea May 28, 2024

onurtemizkan May 30, 2024

mydea May 31, 2024

AbhiPrasad commented May 31, 2024

mydea Jun 4, 2024

onurtemizkan Jun 11, 2024

mydea Jun 4, 2024

onurtemizkan Jun 11, 2024

github-actions bot commented Jun 11, 2024 •

edited

feat(remix): Migrate to opentelemetry-instrumentation-remix. #12110

Are you sure you want to change the base?

feat(remix): Migrate to opentelemetry-instrumentation-remix. #12110

Conversation

onurtemizkan commented May 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AbhiPrasad commented May 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 11, 2024 • edited

size-limit report 📦

feat(remix): Migrate to `opentelemetry-instrumentation-remix`. #12110

feat(remix): Migrate to `opentelemetry-instrumentation-remix`. #12110

onurtemizkan commented May 17, 2024 •

edited

github-actions bot commented Jun 11, 2024 •

edited