getsentry / sentry-javascript

Official Sentry SDKs for JavaScript
https://sentry.io
MIT License
7.98k stars 1.57k forks source link

Adding sentry to Remix increases latency by a lot #12285

Closed aon closed 4 months ago

aon commented 5 months ago

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

Framework Version

2.9.1

Link to Sentry event

No response

SDK Setup

No response

Steps to Reproduce

  1. Create a remix project with express as backend
  2. Add sentry to your project following instructions in https://docs.sentry.io/platforms/javascript/guides/remix/manual-setup/#installation
  3. Add a loader to a page
  4. Build and start application
  5. See how latency increases

Expected Result

As we're using a localhost environment, latency should be in the order of 1 ms.

image

Actual Result

Latency is in the order of 200/300ms.

image

aon commented 5 months ago

See also https://github.com/epicweb-dev/epic-stack/issues/744 and https://github.com/remix-run/remix/issues/9396

AbhiPrasad commented 5 months ago

@aon if you disable performance monitoring by removing tracesSampleRate, does the latency still apply?

aon commented 5 months ago

Removing tracesSampleRate removes the latency as well.

AbhiPrasad commented 5 months ago

So it seems the issue is with performance monitoring. Hopefully this is addressed by https://github.com/getsentry/sentry-javascript/pull/12110 then cc @onurtemizkan

aon commented 5 months ago

Hopefully then. Thanks for the quick answer! Let me know how could I test this @onurtemizkan or how could I help out.

onurtemizkan commented 5 months ago

Thanks for reporting this @aon!

This is weird, I have tried to reproduce this locally but the Content Download stayed in microseconds in my test app. I think this will probably be fixed by #12110 (since we're removing most of the express server custom logic), but I'm still curious about what can be the culprit here.

Could you provide us with a minimal reproduction in the meantime? Also alternatively you can build from the branch https://github.com/getsentry/sentry-javascript/tree/onur/remix-otel-migration, and test to see if that update helps.

aon commented 5 months ago

Hi @onurtemizkan! I created it here https://github.com/aon/minimum-sentry-remix. Please let me know your findings. Thanks!

aon commented 5 months ago

BTW @onurtemizkan I've tried building the branch and I couldn't.

❯ yarn build
lerna notice cli v7.1.1

    ✔  @sentry/types:build:transpile (2s)

    ✖  @sentry/utils:build:transpile
       Usage Error: Couldn't find a script named "ts-node".

       $ yarn run [--inspect] [--inspect-brk] [-T,--top-level] [-B,--binaries-only] <scriptName> ...

    ✔  @sentry-internal/replay-worker:build:transpile (5s)

    ✖  @sentry/types:build:types
       Usage Error: Couldn't find a script named "downlevel-dts".

       $ yarn run [--inspect] [--inspect-brk] [-T,--top-level] [-B,--binaries-only] <scriptName> ...
       ERROR: "build:types:downlevel" exited with 1.

    ✖  @sentry-internal/replay-worker:build:types
       Usage Error: Couldn't find a script named "downlevel-dts".

       $ yarn run [--inspect] [--inspect-brk] [-T,--top-level] [-B,--binaries-only] <scriptName> ...
       ERROR: "build:types:downlevel" exited with 1.

 —————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————

 >  Lerna (powered by Nx)   Ran targets build:transpile, build:types, build:bundle for 31 projects (6s)

    ✔    2/5 succeeded [0 read from cache]

    ✖    3/5 targets failed, including the following:
         - @sentry/utils:build:transpile
         - @sentry/types:build:types
         - @sentry-internal/replay-worker:build:types

   Hint: Try "nx view-logs" to get structured, searchable errors logs in your browser.
onurtemizkan commented 5 months ago

Thanks for the reproduction @aon!

Yes, I can see this issue exists, and also can confirm that it's fixed on the PR #12110's branch!

AbhiPrasad commented 4 months ago

We've released the changes with https://github.com/getsentry/sentry-javascript/releases/tag/8.10.0. To start using it, add autoInstrumentRemix: true to your server-side Sentry.init call!

https://docs.sentry.io/platforms/javascript/guides/remix/manual-setup/#server-side-configuration