Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

V8 Integrations are broken #12582

Closed
3 tasks done
depsimon opened this issue Jun 20, 2024 · 9 comments · Fixed by #12589
Closed
3 tasks done

V8 Integrations are broken #12582

depsimon opened this issue Jun 20, 2024 · 9 comments · Fixed by #12589
Assignees
Labels
Package: remix Issues related to the Sentry Remix SDK Type: Bug

Comments

@depsimon
Copy link

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/remix

SDK Version

8.10.0

Framework Version

Remix 2.9.2

Link to Sentry event

No response

SDK Setup

// apps/project/app/modules/sentry/monitoring.server.ts
import { nodeProfilingIntegration } from "@sentry/profiling-node";
import * as Sentry from "@sentry/remix";

export function init() {
  Sentry.init({
    dsn: import.meta.env.VITE_SENTRY_DSN,
    environment: import.meta.env.VITE_ENVIRONMENT,
    debug: true,
    autoInstrumentRemix: true,

    // Set tracesSampleRate to 1.0 to capture 100%
    // of transactions for performance monitoring.
    // We recommend adjusting this value in production
    tracesSampleRate: 1.0,
    profilesSampleRate: 1.0, // Profiling sample rate is relative to tracesSampleRate

    denyUrls: [
      /\/healthcheck/,
      // TODO: be smarter about the public assets...
      /\/build\//,
      /\/favicons\//,
      /\/images\//,
      /\/fonts\//,
      /\/apple-touch-.*/,
      /\/robots.txt/,
      /\/favicon.ico/,
      /\/site\.webmanifest/,
    ],

    integrations: [
      // Temporary disabling this because of https://github.com/getsentry/sentry-javascript/issues/12568
      // nodeProfilingIntegration(),
      // Add profiling integration to list of integrations
      Sentry.nativeNodeFetchIntegration(),
      Sentry.httpIntegration(),
      Sentry.httpClientIntegration(),
    ],

    beforeSendTransaction(event) {
      // ignore all healthcheck related transactions
      if (event.request?.headers?.["X-Healthcheck"] === "true") return null;

      return event;
    },
  });
}
// apps/project/app/entry.server.tsx

if (import.meta.env.VITE_SENTRY_DSN) {
  await import("./modules/sentry/monitoring.server.ts");
}

Steps to Reproduce

  1. Launch remix app with remix vite:build
  2. Open app in browser
  3. An Internal server error occurs
__vite_ssr_import_0__.nativeNodeFetchIntegration is not a function
    at eval (/Users/simon/Code/Project/monorepo/apps/project/app/modules/sentry/monitoring.server.ts:32:27)
    at async instantiateModule (file:///Users/simon/Code/Project/monorepo/node_modules/.pnpm/vite@5.2.11_@types+node@20.12.12/node_modules/vite/dist/node/chunks/dep-cNe07EU9.js:55058:9

Expected Result

Integrations being added.

Actual Result

Integrations are not recognized and break the app.

@github-actions github-actions bot added the Package: remix Issues related to the Sentry Remix SDK label Jun 20, 2024
@AbhiPrasad
Copy link
Member

@onurtemizkan could you take a look?

@onurtemizkan
Copy link
Collaborator

Hi @depsimon, are you using a built-in Remix server or an Express custom server in your project?

@depsimon
Copy link
Author

depsimon commented Jun 20, 2024

@onurtemizkan It's the built-in Remix server locally. Deployed on Vercel if that matters, but the issue is happening locally as well.

@onurtemizkan
Copy link
Collaborator

onurtemizkan commented Jun 20, 2024

So I guess the problem is that with the new version, the Sentry SDK should be initialized before any other imports to your server.entry. (This only applies for built-in Remix servers)

Even moving it to the top of the file before any other imports won't help because ESBuild reorders the imports (which is out of our control) in the bundle. So the workaround for that for the time being is running the remix server with --require or --import like:

NODE_OPTIONS='--import=./instrument.server.mjs' remix-serve build
# or
NODE_OPTIONS='--require=./instrument.server.cjs' remix-serve build

If that doesn't help, you can also try removing autoInstrumentRemix from your init config to fall back to the non-OTEL implementation to check if it resolves the issue.

The docs are updated so you can refer to: https://docs.sentry.io/platforms/javascript/guides/remix/manual-setup/

@AbhiPrasad
Copy link
Member

@onurtemizkan should we make the wizard add NODE_OPTIONS=... to the remix-serve command?

@depsimon
Copy link
Author

depsimon commented Jun 20, 2024

I'm not sure this solves the issue.
I've tried the NODE_OPTIONS & removing autoInstrumentRemix but I still have the exact same issue.

To test I removed the dynamic import of monitoring.server.js, moved its content to ./monitoring.mjs and updated the dev script like so: "dev": "NODE_OPTIONS='--import=./monitoring.mjs' remix vite:dev",

It's really relative to the integrations exported by @senty/remix I believe.

I tried to remove them while keeping the autoInstrumentRemix and this works (the classic way, by dynamically importing it in my entry.server.tsx)

Side note, we don't use the remix-serve command anymore since Vite has been brought to Remix.

@onurtemizkan
Copy link
Collaborator

should we make the wizard add NODE_OPTIONS=... to the remix-serve command?

@AbhiPrasad, yes makes sense. I'll open a PR for that 👍

@depsimon, that's interesting. I'll try to reproduce this locally.

@onurtemizkan
Copy link
Collaborator

@depsimon, we have updated the exports from Remix SDK which should resolve this issue. Please reopen if you have problems with the next release.

@AbhiPrasad
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: remix Issues related to the Sentry Remix SDK Type: Bug
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants