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

RemixJS - Cloudflare Server Adapter - Sentry Wrapper #5610

Open
3 tasks done
Tracked by #12620
jcpaulco opened this issue Aug 19, 2022 · 16 comments
Open
3 tasks done
Tracked by #12620

RemixJS - Cloudflare Server Adapter - Sentry Wrapper #5610

jcpaulco opened this issue Aug 19, 2022 · 16 comments
Assignees
Labels
Package: cloudflare Issues related to the Sentry Cloudflare Workers SDK Package: remix Issues related to the Sentry Remix SDK Type: Improvement Waiting for: Product Owner

Comments

@jcpaulco
Copy link

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Which package are you using?

@sentry/remix

SDK Version

7.11.0

Framework Version

Remix-Run: 1.6.7 & React 17.0.2

Link to Sentry event

No response

Steps to Reproduce

  1. Opted to use Cloudflare workers as opposed to Remix's built in app server
  2. Followed Sentry integration instructions
  3. Sentry.captureException(new Error("Error) is only being reported to Sentry if in primary tsx component, Not in the loader / action functions.

It looks like if Express is used as the server adapter as opposed to cloudflare-workers then there exists the wrapExpressCreateRequestHandler to handle it but nothing exists for the cloudflare-workers.

See current server.js below utilizing cloudflare-workers:

import { createEventHandler } from "@remix-run/cloudflare-workers";
import * as build from "@remix-run/dev/server-build";

addEventListener(
  "fetch",
  createEventHandler({ build, mode: process.env.NODE_ENV })
);

Current Sentry.init in app/entry.server.tsx:

Sentry.init({
  dsn: process.env.SENTRY_DSN,
  tracesSampleRate: 0.2,
});

Expected Result

Sentry capture exceptions should be reported in the server code as well.

Actual Result

No sentry event is reported from the server, only client.

@AbhiPrasad
Copy link
Member

Hey, There is currently no support for cloudflare workers since the Sentry SDK does not officially support the cloudflare worker runtime. Backlogging for the team, but PRs welcome in the meantime!

@AbhiPrasad
Copy link
Member

Related: #2484

@huw
Copy link

huw commented Nov 27, 2022

Spent the morning on a workaround until we have proper WinterCG support. It’s a first pass, largely hacky, and relying on what we have in Toucan today.

First, copy-paste this into server/instrumentBuild.ts (create it next to server/index.ts):

import { type EntryContext, type HandleDocumentRequestFunction, type ServerBuild } from "@remix-run/cloudflare";
import { extractData, type AppData } from "@remix-run/react/dist/data";
import { isResponse } from "@remix-run/server-runtime/dist/responses";
import { type ActionFunction, type LoaderFunction } from "@remix-run/server-runtime/dist/routeModules";
import { type Exception, type WrappedFunction } from "@sentry/types";
import { fill, normalize } from "@sentry/utils";
import * as cookie from "cookie";
import type Toucan from "toucan-js";
import { type Options } from "toucan-js";

const extractResponseError = async (response: Response): Promise<unknown> => {
  const responseData = (await extractData(response)) as unknown;

  if (typeof responseData === "string") {
    return responseData;
  }

  if (response.statusText) {
    return response.statusText;
  }

  return responseData;
};

const captureRemixServerException = async (
  sentry: Toucan,
  error: unknown,
  name: string,
  request: Request
): Promise<void> => {
  // Skip capturing if the thrown error is not a 5xx response
  // https://remix.run/docs/en/v1/api/conventions#throwing-responses-in-loaders
  if (isResponse(error) && error.status < 500) {
    return;
  }

  const exception = isResponse(error) ? await extractResponseError(error) : error;

  sentry.withScope((scope) => {
    // Transformed & handled later in `beforeSend`
    // Attempts to match https://github.com/getsentry/sentry-javascript/blob/b49082e9aa0f0c1d7ffff5116fdf0412269e0204/packages/remix/src/utils/instrumentServer.ts#L107
    scope.setExtra("request", request);
    scope.setExtra("exceptionMechanism", {
      type: "instrument",
      handled: true,
      data: {
        function: name,
      },
    });

    sentry.captureException(exception);
  });
};

const makeWrappedDocumentRequestFunction =
  (sentry: Toucan) =>
  (origDocumentRequestFunction: HandleDocumentRequestFunction): HandleDocumentRequestFunction => {
    return async function (
      this: unknown,
      request: Request,
      responseStatusCode: number,
      responseHeaders: Headers,
      context: EntryContext
    ): Promise<Response> {
      let response: Response;

      try {
        // eslint-disable-next-line @babel/no-invalid-this -- We need to be able to pass `this` to the original function to wrap it correctly.
        response = await origDocumentRequestFunction.call(this, request, responseStatusCode, responseHeaders, context);
      } catch (error) {
        await captureRemixServerException(sentry, error, "documentRequest", request);
        throw error;
      }

      return response;
    };
  };

interface MakeWrappedDataFunction {
  (sentry: Toucan, dataFunctionType: "action", origFunction: ActionFunction): ActionFunction;
  (sentry: Toucan, dataFunctionType: "loader", origFunction: LoaderFunction): LoaderFunction;
}

const makeWrappedDataFunction: MakeWrappedDataFunction = (sentry: Toucan, dataFunctionType, origFunction) => {
  return async function (this: unknown, args: Parameters<typeof origFunction>[0]): Promise<Response | AppData> {
    let response: unknown;

    try {
      // eslint-disable-next-line @babel/no-invalid-this -- We need to be able to pass `this` to the original function to wrap it correctly.
      response = (await origFunction.call(this, args)) as unknown;
    } catch (error) {
      await captureRemixServerException(sentry, error, dataFunctionType, args.request);
      throw error;
    }

    return response;
  };
};

const makeWrappedAction =
  (sentry: Toucan) =>
  (origAction: ActionFunction): ActionFunction => {
    return makeWrappedDataFunction(sentry, "action", origAction);
  };

const makeWrappedLoader =
  (sentry: Toucan) =>
  (origLoader: LoaderFunction): LoaderFunction => {
    return makeWrappedDataFunction(sentry, "loader", origLoader);
  };

export const beforeSend: Options["beforeSend"] = (event) => {
  // Add data from the `request` extra to the event request object.
  // Mostly adapted from https://github.com/getsentry/sentry-javascript/blob/b49082e9aa0f0c1d7ffff5116fdf0412269e0204/packages/node/src/requestdata.ts#L140
  if (event.extra?.request) {
    const request = event.extra.request as Request;
    const headers = Object.fromEntries(request.headers.entries());
    const cookieHeader = headers.cookie;
    delete headers.cookie;

    event.request = {
      ...event.request,
      url: request.url,
      method: request.method,
      data: normalize(request.body) as unknown,
      query_string: new URL(request.url).search.replace("?", ""),
      cookies: (cookieHeader && cookie.parse(cookieHeader)) || {},
      // Add more to the allowlist later if you'd like.
      headers: { user_agent: headers["user-agent"] },
    };

    delete event.extra.request;
  }

  // Add the exception mechanism from extra to the event object.
  // Adapted from https://github.com/getsentry/sentry-javascript/blob/b49082e9aa0f0c1d7ffff5116fdf0412269e0204/packages/utils/src/misc.ts#L93
  if (event.extra?.exceptionMechanism) {
    const firstException = event.exception?.values?.[0];
    const newMechanism = event.extra?.exceptionMechanism as Exception["mechanism"];

    if (firstException) {
      const defaultMechanism = { type: "generic", handled: true };
      const currentMechanism = firstException.mechanism;
      firstException.mechanism = { ...defaultMechanism, ...currentMechanism, ...newMechanism };

      if (newMechanism && "data" in newMechanism) {
        const mergedData = { ...currentMechanism?.data, ...newMechanism?.data };
        firstException.mechanism.data = mergedData;
      }
    }

    delete event.extra.exceptionMechanism;
  }

  return event;
};

/**
 * Instruments `remix` ServerBuild for performance tracing and error tracking.
 */
export const instrumentBuild = (sentry: Toucan, build: ServerBuild): ServerBuild => {
  const routes: ServerBuild["routes"] = {};

  const wrappedEntry = { ...build.entry, module: { ...build.entry.module } };

  // Not keeping boolean flags like it's done for `requestHandler` functions,
  // Because the build can change between build and runtime.
  // So if there is a new `loader` or`action` or `documentRequest` after build.
  // We should be able to wrap them, as they may not be wrapped before.
  if (!(wrappedEntry.module.default as WrappedFunction).__sentry_original__) {
    fill(wrappedEntry.module, "default", makeWrappedDocumentRequestFunction(sentry));
  }

  for (const [id, route] of Object.entries(build.routes)) {
    const wrappedRoute = { ...route, module: { ...route.module } };

    if (wrappedRoute.module.action && !(wrappedRoute.module.action as WrappedFunction).__sentry_original__) {
      fill(wrappedRoute.module, "action", makeWrappedAction(sentry));
    }

    if (wrappedRoute.module.loader && !(wrappedRoute.module.loader as WrappedFunction).__sentry_original__) {
      fill(wrappedRoute.module, "loader", makeWrappedLoader(sentry));
    }

    routes[id] = wrappedRoute;
  }

  return { ...build, routes, entry: wrappedEntry };
};

export default instrumentBuild;

If you’re keeping score at home, this modifies instrumentServer.ts to use Toucan instead of @sentry/node. It drops anything that Toucan doesn’t natively support, which means you’ll lose some metadata about what action/loader was run as well as all tracing support.

Then modify server/index.ts:

import { createEventHandler } from "@remix-run/cloudflare-workers";
import * as build from "@remix-run/dev/server-build";
import Toucan from "toucan-js";
import instrumentBuild, { beforeSend } from "./instrumentBuild";

addEventListener("fetch", (event: FetchEvent) => {
  const sentry = new Toucan({
    dsn: SENTRY_DSN,
    context: event,
    beforeSend,
    // `allowedHeaders` and `allowedSearchParams` are ignored, see `beforeSend` in `instrumentBuild.ts`
    // allowedHeaders: ["user-agent"],
    // allowedSearchParams: /(.*)/u,
  });

  createEventHandler({
    build: instrumentBuild(sentry, build),
    mode: process.env.NODE_ENV,
  })(event);
});

Hope this helps anyone stuck on this one ^_^ I’ll try and patch it up a bit further to get metadata & tracing in.

@huw
Copy link

huw commented Feb 18, 2023

I finally got around to updating the above thanks to Toucan v3. Everything’s in this gist, but essentially you need to:

  1. Rewrite instrumentBuild to accept a passed-through Hub instance, rather than using getCurrentHub() (including, perniciously, in helper functions such as hasTracingEnabled())
  2. In server/index.ts, create a root transaction from the current route’s name and wrap the loaders and actions in spans via instrumentBuild
  3. In root.tsx, pass through your tracing & baggage into the meta function
  4. In root.tsx, include the current domain in tracePropagationTargets (because Remix fetchers will fetch from the entire URL rather than / root, which will confuse Sentry)

One major caveat is that server-side timing will be extremely inaccurate. Cloudflare freeze Date.now() on the server to avoid side-channel timing attacks. This means that within Workers, the time will never increment during a single request, and in Durable Objects, it will increment with the time of the last I/O. Relative times of multiple worker invocations during a single trace will be accurate, and so will anything measured on the client-side.

If anyone stumbles across this in the future and is having trouble with the code gimme a yell. (And if Sentry are interested I’ll happily modify the existing code to support Workers, but it’ll be a decent refactor because of getCurrentHub()).

@huw
Copy link

huw commented Jul 31, 2023

I’ve updated my Gist for Toucan v3.2.0 and Remix v2. It’s a lot more complicated & hacky now thanks to Remix v2, but still very possible. Please reply there if you have any questions :)

@huw
Copy link

huw commented Oct 13, 2023

@AbhiPrasad (or others) I’d like to start work on contributing back some stuff that should make non-Node runtime support a bit easier, but I wanted to run it by the team first in case you have better ideas than me :) (I know you’re working on Deno next—unsure how much of that would be reusable here)

The main change I’d like to start with would be (1) exporting the utility functions such as instrumentBuild and (2) refactoring the utility functions to accept getCurrentHub as an argument. This would allow Cloudflare Workers & Toucan users to run ex.:

const hub = new Toucan({});
const getCurrentHub = () => hub;
const newBuild = instrumentBuild(getCurrentHub, build);

This would go 90% of the way to preventing me from having to vendor & maintain your code in my repo while remaining fully backwards-compatible with the existing implementation. Would you be open to this?

@AbhiPrasad
Copy link
Member

We have #9225 - feel free to take a look at that.

(2) refactoring the utility functions to accept getCurrentHub as an argument

What might be the easiest is just to change the behaviour of getCurrentHub by defining your own async context strategy. Something that looks like so https://github.com/getsentry/sentry-javascript/blob/develop/packages/vercel-edge/src/async.ts

@huw
Copy link

huw commented Oct 18, 2023

@AbhiPrasad I can’t think of an async context strategy that wouldn’t risk context bleeding between concurrently running workers (Cloudflare run concurrent workers in the same data centre in the same isolate, meaning they’ll share global variables, but they don’t support Async Context Strategies which is usually the way around this)

But I’ll look into that PR, looks promising!

@AbhiPrasad
Copy link
Member

yeah I just realized my suggestion really only works if you have nodejs_compat, so we have to find another way around it. will leave the rest of my thoughts in the PR.

@Knaackee
Copy link

Hey @huw

did you made progress?

Thanks!

@huw
Copy link

huw commented Dec 12, 2023

No, there is no way to support Async Context in Workers without node_compat. Any other strategy risks context bleeding across concurrent requests in the same Worker.

@vladinator1000
Copy link

vladinator1000 commented Jun 4, 2024

I bypassed the Sentry SDK entirely, relying on Toucan, but I couldn't get source maps to work. Here's a minimal repo https://github.com/vladinator1000/remix-cloudflare-pages-sentry

Discord discussion https://discord.com/channels/621778831602221064/1245726488905650239/1245726488905650239

@AbhiPrasad
Copy link
Member

AbhiPrasad commented Aug 1, 2024

We've just released a pages plugin with @sentry/cloudflare. This isn't built-in to @sentry/remix, so you'll have to install both @sentry/remix and @sentry/cloudflare for now, we'll make this better!

To use it, first add the Remix SDK to your app, but only configure it client-side. Do not add Sentry.init from @sentry/remix to your server-side code. Instead add sentryPagesPlugin as request middleware to your app. For example you could add it above the createPagesFunctionHandler handler, or create a separate _middleware.ts file.

import { createPagesFunctionHandler } from '@remix-run/cloudflare-pages';
import { sentryPagesPlugin } from '@sentry/cloudflare';

// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore - the server build file is generated by `remix vite:build`
// eslint-disable-next-line import/no-unresolved
import * as build from '../build/server';

export const onRequest = [
  (context) => sentryPagesPlugin({ dsn: context.env.E2E_TEST_DSN, tracesSampleRate: 1.0 })(context),
  createPagesFunctionHandler({ build }),
];

You'll need the nodejs_compat or nodejs_als compatibility flags in your wrangler.toml. This is because the SDK needs access to the AsyncLocalStorage API to work correctly.

compatibility_flags = ["nodejs_compat"]
# compatibility_flags = ["nodejs_als"]

You can run npx @sentry/wizard@latest -i sourcemap to then set up sourcemaps uploading for your project (for modern Remix projects, it should go through the vite workflow).

Note: Remix sourcemaps seem to be not pointing to the correct files. I'm actively debugging why this is, but it seems like a cloudflare issue and not a sentry one.

For more details, see the README: https://github.com/getsentry/sentry-javascript/blob/develop/packages/cloudflare/README.md

@aaronadamsCA
Copy link
Contributor

aaronadamsCA commented Aug 28, 2024

Middleware is a great idea, but unfortunately the Pages plugin only seems to receive sanitized errors from Remix. Every error is just "Unexpected Server Error" with no stack trace.

I think what's needed is something lower-level that can be called inside a handleError function.

Any ideas?

@andreiborza
Copy link
Member

@aaronadamsCA thanks for the feedback. We'll look at this next week as some of us are out of the office currently.

@nick
Copy link

nick commented Sep 20, 2024

I believe the incorrect source maps issue is because Sentry is uploading the source maps from build/server/index.js, but the worker that is uploaded to Cloudflare is actually a separate file that itself includes build/server/index.js. When an exception is thrown, the line numbers that are returned are from the uploaded worker.js (named functionsWorker-RAND_NUM.js) and not server/index.js

You can see what actually gets uploaded to Cloudflare by running this command from your Remix root:

npx wrangler pages functions build --outdir tmp --sourcemap --build-output-directory tmp

Then look at tmp/index.js and tmp/index.js.map.

To get to the 'true' source of the error, I used this script pointing to the tmp/index.js.map which returned:

{
  source: '/Projects/remix-project/build/server/index.js',
  line: 5718,
  column: 10,
  name: null
}

I then used the same script but this time pointing to remix-project/build/server/index.js.map with line 5718 column 10 to get the actual source of the error:

{ source: '../../app/routes/err.tsx', line: 8, column: 10, name: null }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: cloudflare Issues related to the Sentry Cloudflare Workers SDK Package: remix Issues related to the Sentry Remix SDK Type: Improvement Waiting for: Product Owner
Projects
Status: No status
Status: Waiting for: Product Owner
Development

No branches or pull requests

10 participants