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

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

Ref: #11040

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

This PR removes:

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
Express server adapter (This breakingly changes the DX for the better.)
Auto-instrumentation for http as default integration for Remix SDK.

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...?

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...!

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 2 times, most recently from 615c949 to 14990c4 Compare June 7, 2024 14:19

onurtemizkan added 6 commits June 7, 2024 15:21

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

62597c4

Fix linter.

26bf715

Return jest config for unit tests back.

519da6f

Don't run Remix integration tests on Node 14

d688093

Instrument function builds properly.

95f8842

Add ErrorBoundary propagation test.

5783565

onurtemizkan added 7 commits June 7, 2024 15:21

Export a no-op function in place of wrapExpressCreateRequestHandler.

534c382

Add a custom httpIntegration for Remix.

31a8d75

Switch to import pattern to load instrumentation on Express tests

52d8506

Remove extra Sentry.inits in v2 e2e test app.

b774248

Make auto-instrumentation opt-in.

dd67b4b

Add legacy integration tests back.

6db0e05

Fix broken request transactions.

0ae09ee

onurtemizkan force-pushed the onur/remix-otel-migration branch from 14990c4 to 0ae09ee Compare June 7, 2024 14:21

Use Remix httpIntegration for both otel and legacy instrumentations.

dd9404b

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

mydea Jun 4, 2024

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

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

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

onurtemizkan commented May 17, 2024 •

edited