[ Remix ] Incorrect error reporting

InterstellarStella commented 5 months ago

Is there an existing issue for this?

[X] I have checked for existing issues https://github.com/getsentry/sentry-javascript/issues
[X] I have reviewed the documentation https://docs.sentry.io/
[X] I am using the latest SDK release https://github.com/getsentry/sentry-javascript/releases

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/remix

SDK Version

7.93.0

Framework Version

No response

Link to Sentry event

No response

SDK Setup

In monitoring.client.ts:

export function init() {
  Sentry.init({
    dsn: ENV.SENTRY_DSN,
    environment: ENV.MODE,
    integrations: [
      new Sentry.BrowserTracing({
        routingInstrumentation: Sentry.remixRouterInstrumentation(
          useEffect,
          useLocation,
          useMatches,
        ),
      }),
      // Replay is only available in the client
      new Sentry.Replay({
        maskAllText: false,
        maskAllInputs: false,
      }),
      new Sentry.BrowserProfilingIntegration(),
    ],

    // Set tracesSampleRate to 1.0 to capture 100%
    // of transactions for performance monitoring.
    // We recommend adjusting this value in production
    tracesSampleRate: 1.0,

    // Capture Replay for 10% of all sessions,
    // plus for 100% of sessions with an error
    replaysSessionSampleRate: 0.1,
    replaysOnErrorSampleRate: 1.0,
  });
}

In monitoring.server.ts:

export function init() {
  Sentry.init({
    dsn: ENV.SENTRY_DSN,
    environment: ENV.MODE,
    tracesSampleRate: ENV.MODE === "production" ? 1 : 0,
    denyUrls: [
      /healthcheck/,
      // TODO: be smarter about the public assets...
      /\/build\//,
      /\/favicons\//,
      /\/images\//,
      /\/favicon.ico/,
      /\/site\.webmanifest/,
    ],
    integrations: [
      new Sentry.Integrations.Http({ tracing: true }),
      new Sentry.Integrations.Prisma({ client: prisma }),
      new ProfilingIntegration(),
    ],
    beforeSendTransaction(event) {
      // ignore all healthcheck related transactions
      if (event.request?.headers?.["X-Healthcheck"] === "true") return null;

      return event;
    },
  });
}

In entry.client.tsx:

if (enabled && ENV.SENTRY_DSN) {
  import("./utils/monitoring.client").then(({ init }) => init());
}

In entry.server.tsx:

if (enabled && ENV.SENTRY_DSN) {
  console.log("Sentry enabled (server): ", enabled);
  import("./utils/monitoring.server").then(({ init }) => init());
}

In root.tsx:

export function ErrorBoundary({ error }: { error: Error }) {
  Sentry.captureRemixErrorBoundaryError(error);
  return (
    <html className="h-full">
      <head>
        <Meta />
        <Links />
      </head>
      <body className="h-full w-full bg-white">
        <GeneralErrorBoundary />
        <ScrollRestoration />
        <Scripts />
        <LiveReload />
      </body>
    </html>
  );
}

export default Sentry.withSentry(App);

In error.tsx:

export default function Error() {
  return (
    <div className="flex flex-col gap-y-3">
      <Text variant="h1">This is the error page</Text>
      <Text variant="h2">
        Use this button to test client side errors in sentry
      </Text>
      <button
        type="button"
        onClick={() => {
          //@ts-ignore
          throw new Error("Sentry Frontend Error");
        }}
      >
        Throw error
      </button>
      <Text variant="h2">Use this to test server side error in sentry</Text>
      <Form method="post" className="flex flex-col">
        <input type="text" />
        <Button variant="primary" type="submit">
          Submit
        </Button>
      </Form>
    </div>
  );
}

I'm adding the full app in the shadow ticket for debugging.

Steps to Reproduce

Run npm install
Run npm run dev (dotenv -e .env -- npm run start:dev-server)
Click on the button to throw a frontend error

(Optionally we can build the app and upload its source maps to Sentry)

Expected Result

The captured event in Sentry correctly describes the Error("Sentry Frontend Error") that we threw, and the stack trace points to the line where we call throw new Error("Sentry Frontend Error");

Actual Result

Instead, we get an error with descripton Object captured as exception with keys:, and the stack trace points to Sentry's package.

The app and the event link are shared in the shadow ticket.

┆Issue is synchronized with this Jira Improvement by Unito

onurtemizkan commented 5 months ago

Hi @InterstellarStella,

Looking at the code, this seems like a misconfigured ErrorBoundary https://remix.run/docs/en/main/guides/errors#root-error-boundary

Errors are obtained by useRouteError() inside ErrorBoundary, so our recommended usage is as below:

export const ErrorBoundary = () => {
  const error = useRouteError();

  Sentry.captureRemixErrorBoundaryError(error);

  return <div>...</div>;
};

Could you please check if that resolves this problem?

mikedklein commented 5 months ago

Hi @onurtemizkan I am the one that @InterstellarStella was helping with this. That was definitely an error on my part but the GeneralErrorBoundary component was using it correctly. I updated the code to be the following and I am still seeing this error being reported in sentry as @InterstellarStella has above

root.tsx

export function ErrorBoundary() {
  return (
    <html className="h-full">
      <head>
        <Meta />
        <Links />
      </head>
      <body className="h-full w-full bg-white">
        <GeneralErrorBoundary />
        <ScrollRestoration />
        <Scripts />
        <LiveReload />
      </body>
    </html>
  );
}

export default Sentry.withSentry(App);

error-boundary.tsx

import {
  type ErrorResponse,
  isRouteErrorResponse,
  useParams,
  useRouteError,
} from "@remix-run/react";
import { getErrorMessage } from "~/utils/misc";
import { HansaLogo } from "./ui/hansa-logo";
import { captureRemixErrorBoundaryError } from "@sentry/remix";

type StatusHandler = (info: {
  error: ErrorResponse;
  params: Record<string, string | undefined>;
}) => JSX.Element | null;

export function GeneralErrorBoundary({
  defaultStatusHandler = ({ error }) => (
    <div className="my-[16px] px-[24px] font-normal text-gray-600">
      <p>
        Oh no! Something went wrong on our end. We're already working on fixing
        the issue!
      </p>
      <pre className="text-error">
        {error.status} {error.data}
      </pre>
    </div>
  ),
  statusHandlers,
  unexpectedErrorHandler = (error) => (
    <div className="my-[16px] px-[24px] font-normal text-gray-600">
      <p>
        Oh no! Something went wrong on our end. We're already working on fixing
        the issue!
      </p>
      <pre className="text-error">{getErrorMessage(error)}</pre>
    </div>
  ),
}: {
  defaultStatusHandler?: StatusHandler;
  statusHandlers?: Record<number, StatusHandler>;
  unexpectedErrorHandler?: (error: unknown) => JSX.Element | null;
}) {
  const error = useRouteError();
  const params = useParams();

  captureRemixErrorBoundaryError(error);

  if (typeof document !== "undefined") {
    console.error(error);
  }

  return (
    <div className="flex h-full w-full flex-col items-center justify-center gap-[8px] text-center sm:gap-0">
      <HansaLogo className="my-10" />
      {isRouteErrorResponse(error)
        ? (statusHandlers?.[error.status] ?? defaultStatusHandler)({
            error,
            params,
          })
        : unexpectedErrorHandler(error)}
    </div>
  );
}

One thing I did realize though is that the way I was originally throwing the error doesn't trigger a remix error boundary. Throwing like I was doesn't actually cause a react rendering error thus is not caught by the error boundary. I changed the component to throw like this.

function throwError() {
  //@ts-ignore
  throw new Error("This is a forced error for testing purposes");
}

export default function Error() {
  const [error, setError] = React.useState(false);

  if (error) {
    throwError();
  } else {
    return (
      <div className="flex flex-col gap-y-3">
        <Text variant="h1">This is the error page</Text>
        <Text variant="h2">
          Use this button to test client side errors in sentry
        </Text>
        <button
          type="button"
          onClick={() => {
            setError(true);
          }}
        >
          Throw error
        </button>
        <Text variant="h2">Use this to test server side error in sentry</Text>
        <Form method="post" className="flex flex-col">
          <input type="text" />
          <Button variant="primary" type="submit">
            Submit
          </Button>
        </Form>
      </div>
    );
  }
}

Then that stack trace works as expected.

I am not sure if the original is intended behavior it still feels like that should be mapped back to the offending code instead of being surfaced in the sentry SDK.

Lms24 commented 5 months ago

Hi @mikedklein apologies for the delayed reply! I looked at this issue, your repro app and what you changed ultimately (last reply) and I think I know what's happening:

I think you're right - throwing the error on click (as according to the repro app you did initially) won't trigger the error boundary but another global error handler from the browser which we instrument catches it and sends it to Sentry. This makes sense to me, given that React isn't rendering anything at this time. When looking at your linked test event, I could verify this because the error tags say mechanism: instrument and function: addEventListener. If it was caught by captureRemixErrorBoundaryError, the function tag would be function: ReactError.
Usually, the error caught via the event listener, should still show up with the error message though and not with Object captured as exception with keys. I'm not sure why that's happening but my guess is that Remix or React still somehow interfere with the error object. Maybe @AbhiPrasad or @onurtemizkan know more here.

So all in all, I believe this is expected behaviour but I can see why it's hard to decipher this correctly. Also might be worth looking into if we can do better at extracting the actual error from the object in this case.

mikedklein commented 5 months ago

@Lms24 no worries at all. Thanks for the update and that line of reasoning makes sense to me. But, yeah, any way of making the error more helpful in Sentry would be a nice to have on this one, I don't anticipate having a lot of client side errors that don't trigger the error boundary but you never know.

The other thing is that I would also suggest an update to the docs if this is intended behavior because this makes it feel like it should be a readable react/remix error. Maybe just mentioning this is just testing that client side errors are being sent and will be handled by a global error handler.

lforst commented 5 months ago

@mikedklein Fyi, react error boundaries are only triggered when errors are thrown during rendering. Any attached event handlers, like onClick do not trigger error boundaries when thrown. This is React behaviour and not necessarily Sentry SDK behaviour.

mikedklein commented 5 months ago

@lforst yeah I mentioned that in my follow up.

I think the problem here is two-fold:

The Sentry Remix docs suggest using that onClick thrown error to test things are being sent to Sentry and it wasn't clear that this would not be mapped to source code.
The error is handled higher up and when looking in Sentry it looks like it originates from the SentrySDK in node_modules rather than the code that triggered it. As well as what @Lms24 mentioned that it is reported as Object captured as exception with keys. rather than the actual message body of the error.

lforst commented 5 months ago

All errors should definitely be mapped to source code. However, if a non-Error object is thrown (i.e. some entity that does not have a stack trace), we will create a synthetic stack trace pointing as close as possible to the place where the object may have been thrown. It appears that we're not doing a good job in that regard here.

SnailOwO commented 1 month ago

Is there an existing issue for this?

[x] I have checked for existing issues https://github.com/getsentry/sentry-javascript/issues

[x] I have reviewed the documentation https://docs.sentry.io/

[x] I am using the latest SDK release https://github.com/getsentry/sentry-javascript/releases

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/remix

SDK Version

7.93.0

Framework Version

No response

Link to Sentry event

No response

SDK Setup

In monitoring.client.ts:
export function init() {
  Sentry.init({
    dsn: ENV.SENTRY_DSN,
    environment: ENV.MODE,
    integrations: [
      new Sentry.BrowserTracing({
        routingInstrumentation: Sentry.remixRouterInstrumentation(
          useEffect,
          useLocation,
          useMatches,
        ),
      }),
      // Replay is only available in the client
      new Sentry.Replay({
        maskAllText: false,
        maskAllInputs: false,
      }),
      new Sentry.BrowserProfilingIntegration(),
    ],

    // Set tracesSampleRate to 1.0 to capture 100%
    // of transactions for performance monitoring.
    // We recommend adjusting this value in production
    tracesSampleRate: 1.0,

    // Capture Replay for 10% of all sessions,
    // plus for 100% of sessions with an error
    replaysSessionSampleRate: 0.1,
    replaysOnErrorSampleRate: 1.0,
  });
}
In monitoring.server.ts:
export function init() {
  Sentry.init({
    dsn: ENV.SENTRY_DSN,
    environment: ENV.MODE,
    tracesSampleRate: ENV.MODE === "production" ? 1 : 0,
    denyUrls: [
      /healthcheck/,
      // TODO: be smarter about the public assets...
      /\/build\//,
      /\/favicons\//,
      /\/images\//,
      /\/favicon.ico/,
      /\/site\.webmanifest/,
    ],
    integrations: [
      new Sentry.Integrations.Http({ tracing: true }),
      new Sentry.Integrations.Prisma({ client: prisma }),
      new ProfilingIntegration(),
    ],
    beforeSendTransaction(event) {
      // ignore all healthcheck related transactions
      if (event.request?.headers?.["X-Healthcheck"] === "true") return null;

      return event;
    },
  });
}
In entry.client.tsx:
if (enabled && ENV.SENTRY_DSN) {
  import("./utils/monitoring.client").then(({ init }) => init());
}
In entry.server.tsx:
if (enabled && ENV.SENTRY_DSN) {
  console.log("Sentry enabled (server): ", enabled);
  import("./utils/monitoring.server").then(({ init }) => init());
}
In root.tsx:
export function ErrorBoundary({ error }: { error: Error }) {
  Sentry.captureRemixErrorBoundaryError(error);
  return (
    <html className="h-full">
      <head>
        <Meta />
        <Links />
      </head>
      <body className="h-full w-full bg-white">
        <GeneralErrorBoundary />
        <ScrollRestoration />
        <Scripts />
        <LiveReload />
      </body>
    </html>
  );
}

export default Sentry.withSentry(App);
In error.tsx:
export default function Error() {
  return (
    <div className="flex flex-col gap-y-3">
      <Text variant="h1">This is the error page</Text>
      <Text variant="h2">
        Use this button to test client side errors in sentry
      </Text>
      <button
        type="button"
        onClick={() => {
          //@ts-ignore
          throw new Error("Sentry Frontend Error");
        }}
      >
        Throw error
      </button>
      <Text variant="h2">Use this to test server side error in sentry</Text>
      <Form method="post" className="flex flex-col">
        <input type="text" />
        <Button variant="primary" type="submit">
          Submit
        </Button>
      </Form>
    </div>
  );
}
I'm adding the full app in the shadow ticket for debugging.

Steps to Reproduce

Run npm install

Run npm run dev (dotenv -e .env -- npm run start:dev-server)

Click on the button to throw a frontend error

(Optionally we can build the app and upload its source maps to Sentry)

Expected Result

The captured event in Sentry correctly describes the Error("Sentry Frontend Error") that we threw, and the stack trace points to the line where we call throw new Error("Sentry Frontend Error");

Actual Result

Instead, we get an error with descripton Object captured as exception with keys:, and the stack trace points to Sentry's package.

The app and the event link are shared in the shadow ticket.

┆Issue is synchronized with this Jira Improvement by Unito

watching

Lms24 commented 1 month ago

Hey @SnailOwO thx for writing in! just to confirm, do you have the same problem as in the original issue?

Tbh this issue hasn't received much attention since the last reply, mostly because we're busy with version 8 of the SDK.

getsantry[bot] commented 2 weeks ago

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you remove the label Waiting for: Community, I will leave it alone ... forever!

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

getsentry / sentry-javascript