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

extremely slow builds / vercel crashes and fails to build our app #26588

Closed
zallarak opened this issue Jun 25, 2021 · 13 comments · Fixed by getsentry/sentry-javascript#3845
Closed

Comments

@zallarak
Copy link

zallarak commented Jun 25, 2021

Describe the feature you'd like to request

next.js version: 10.2.0

here is our next.js config

const withAntdLess = require("next-plugin-antd-less");
const nextConstants = require("next/constants");
const path = require("path");

const makeCommitSHA = () => {
  const {
    VERCEL_GITHUB_COMMIT_SHA,
    GITHUB_SHA,
    GCLOUD_COMMIT_SHA,
  } = process.env;
  return (
    GCLOUD_COMMIT_SHA || VERCEL_GITHUB_COMMIT_SHA || GITHUB_SHA || "unknown"
  );
};
const makeSentryReleaseID = () => {
  const {
    VERCEL_GITHUB_COMMIT_SHA,
    GITHUB_SHA,
    GCLOUD_COMMIT_SHA,
    SENTRY_RELEASE_ID,
  } = process.env;
  if (SENTRY_RELEASE_ID) {
    return SENTRY_RELEASE_ID;
  }
  if (GCLOUD_COMMIT_SHA) {
    return `gcloud-${GCLOUD_COMMIT_SHA || "unknown"}`;
  }
  if (VERCEL_GITHUB_COMMIT_SHA) {
    return VERCEL_GITHUB_COMMIT_SHA || "unknown";
  }
  if (GITHUB_SHA) {
    return GITHUB_SHA || "unknown";
  }
  return "unknown";
};

const sentryReleaseIDBase = `frontend@${makeSentryReleaseID()}`;
const shouldGenerateSourceMaps =
  process.env.SENTRY_AUTH_TOKEN && sentryReleaseIDBase !== "frontend@unknown";

module.exports = (phase) => {
  const config = withAntdLess({
    reactStrictMode: true,
    productionBrowserSourceMaps: shouldGenerateSourceMaps,
    compress: true,
    images: {
      domains: ["logo.clearbit.com"],
    },
    serverRuntimeConfig: {
      NODE_ENV: process.env.NODE_ENV,
      DOMAIN: process.env.DOMAIN,
      // Third-parties
      LOGROCKET_APP_ID: process.env.LOGROCKET_APP_ID,
      SEGMENT_WRITE_KEY: process.env.SEGMENT_WRITE_KEY,
      PLAID_ENV: process.env.PLAID_ENV,
      STRIPE_PUBLIC_KEY: process.env.STRIPE_PUBLIC_KEY,
      STRIPE_OAUTH_CLIENT_ID: process.env.STRIPE_OAUTH_CLIENT_ID,
      GOOGLE_MAPS_API_KEY: process.env.GOOGLE_MAPS_API_KEY,
      SENTRY_ENVIRONMENT: process.env.SENTRY_ENVIRONMENT,
      // optional
      MODE: process.env.MODE,
      FEATURE_FLAGS: process.env.FEATURE_FLAGS,
      SENTRY_DSN: process.env.SENTRY_DSN,
    },
    lessVarsFilePath: "./antdThemeVariables.less",
    webpack: (config, { isServer, webpack }) => {
      // sentry stuff
      // In `pages/_app.js`, Sentry is imported from @sentry/node. While
      // @sentry/react will run in a Node.js environment, @sentry/node will use
      // Node.js-only APIs to catch even more unhandled exceptions.
      //
      // This works well when Next.js is SSRing your page on a server with
      // Node.js, but it is not what we want when your client-side bundle is being
      // executed by a browser.
      //
      // Luckily, Next.js will call this webpack function twice, once for the
      // server and once for the client. Read more:
      // https://nextjs.org/docs/api-reference/next.config.js/custom-webpack-config
      //
      // So ask Webpack to replace @sentry/node imports with @sentry/react when
      // building the browser's bundle
      // and vice versa
      if (isServer) {
        config.resolve.alias["@sentry/react"] = "@sentry/node";
        config.resolve.alias["@sentry/node"] = "@sentry/node";
      } else {
        config.resolve.alias["@sentry/node"] = "@sentry/react";
        config.resolve.alias["@sentry/react"] = "@sentry/react";
      }

      // See: https://github.com/momopig/react-antd-icons
      config.resolve.alias["@ant-design/icons/lib"] = path.resolve(
        __dirname,
        "./styleMock.tsx"
      );

      config.plugins = config.plugins || [];

      // ignore calls to graphql
      config.plugins.push(
        new webpack.ProvidePlugin({
          graphql: "lodash/noop",
        })
      );

      if (phase === nextConstants.PHASE_PRODUCTION_BUILD) {
        const SENTRY_RELEASE_ID = `${sentryReleaseIDBase}${
          isServer ? "-server" : ""
        }`;

        // Sentry release ID should be hardcoded in the build
        config.plugins.push(
          new webpack.DefinePlugin({
            COMMIT_SHA: JSON.stringify(makeCommitSHA()),
            SENTRY_RELEASE_ID: JSON.stringify(SENTRY_RELEASE_ID),
          })
        );

        const { SENTRY_AUTH_TOKEN } = process.env;
        if (SENTRY_AUTH_TOKEN) {
          console.log(
            `Publishing sourcemaps to Sentry under release id ${SENTRY_RELEASE_ID}`
          );
          const SentryWebpackPlugin = require("@sentry/webpack-plugin");
          config.plugins.push(
            new SentryWebpackPlugin({
              // sentry-cli configuration
              authToken: SENTRY_AUTH_TOKEN,
              org: "pipe",
              project: "frontend",
              include: ".next",
              ignore: ["node_modules"],
              stripPrefix: ["webpack://_N_E/"],
              urlPrefix: `~/_next`,
              release: SENTRY_RELEASE_ID,
            })
          );
        }
        if (shouldGenerateSourceMaps) {
          // This lets Sentry have the sourcemaps without
          // exposing the URL of the source code to the client.
          // In production, we also delete sourcemaps.
          // TODO(igm): private server to host sourcemaps
          // See https://webpack.js.org/configuration/devtool/
          config.devtool = "hidden-source-map";
        }
      } else {
        // Default commit sha/sentry release ID for local dev
        config.plugins.push(
          new webpack.DefinePlugin({
            COMMIT_SHA: JSON.stringify("local-unknown"),
            SENTRY_RELEASE_ID: JSON.stringify("local-unknown"),
          })
        );
      }

      return config;
    },
    typescript: {
      // !! WARN !!
      // Dangerously allow production builds to successfully complete even if
      // your project has type errors.
      // !! WARN !!
      // this is OK because we have type checking in a GH action
      ignoreBuildErrors: true,
    },
    async redirects() {
      return [
        // vendor
        {
          source: "/",
          destination: "/inbox",
          permanent: true,
        },
        {
          source: "/link",
          destination: "/welcome",
          permanent: true,
        },
        {
          source: "/onboard",
          destination: "/welcome",
          permanent: true,
        },
        {
          source: "/settings/payments",
          destination: "/finance/lists/all-transactions",
          permanent: true,
        },
        {
          source: "/settings/payments/:paymentID",
          destination: "/finance/payouts/:paymentID",
          permanent: true,
        },
        {
          source: "/finance/payments",
          destination: "/finance/lists/all-payments",
          permanent: true,
        },
        {
          source: "/finance/payouts",
          destination: "/finance/lists/all-payouts",
          permanent: true,
        },

        // admin
        {
          source: "/admin/subscriptions",
          destination: "/admin/lists/create/subscriptionsv2",
          permanent: true,
        },
        {
          source: "/admin/users",
          destination: "/admin/lists/pipe-admin-users",
          permanent: true,
        },
        {
          source: "/admin/vendors",
          destination: "/admin/lists/create/vendors",
          permanent: true,
        },
        {
          source: "/admin/connectors",
          destination: "/admin/lists/create/connectors",
          permanent: true,
        },
      ];
    },
  });

  if (process.env.ANALYZE === "true") {
    const withBundleAnalyzer = require("@next/bundle-analyzer")({
      enabled: true,
    });
    return withBundleAnalyzer(config);
  }

  return config;
};

Describe the solution you'd like

help debugging or resolution - right now vercel is not usable and we must use google cloud

Describe alternatives you've considered

moving to react or using higher performance machines

@rauchg
Copy link
Member

rauchg commented Jun 25, 2021

Would you mind turning off as many extensions as you can that don't involve breaking your app or having to modify it in a meaningful way, and reporting build times?

I'd suggest starting with source maps, which we have known to be problematic.

@timneutkens
Copy link
Member

Can you try disabling productionSourceMaps and sentry-webpack-plugin

@sokra
Copy link
Member

sokra commented Jun 25, 2021

If this is related to the webpack build (not the static generation or typechecking) there is now a env variable you can set to get more insight into the build:

  • NEXT_WEBPACK_LOGGING=1 prints some basic webpack information to the console, including build times for client and server builds.
  • NEXT_WEBPACK_LOGGING=profile-client or NEXT_WEBPACK_LOGGING=profile-server or even NEXT_WEBPACK_LOGGING=profile-client,profile-server displays very detailed timing information of the webpack builds. Maybe put that into a gist and post it here so we can look into that.

Note: just seeing you are still on webpack 4, where this isn't available...

@zallarak
Copy link
Author

Did the profiling and the verbose logging (NEXT_WEBPACK_LOGGING=profile-client,profile-server) as @sokra specified.

I did this on my extremely fast desktop (32gb ram, ryzen 5950x). Doing it on my laptop is substantially slower.

Profiles and logs:

Some naive thoughts:

  • Clearly, sentry adds a ton of time and from the live output, much of the time was spent uploading ~400mb worth of sourcemaps related code. Is there a way to perhaps push a build out and asynchronously do sourcemaps? There could be downsides to this.

@MohdAhmad1
Copy link

I have just cloned a project, done yarn install, and the first build takes more than 5 seconds, the next builds take 2-3 seconds.
In next js 10 it was faster

@lobsterkatie
Copy link
Contributor

Hi, all. Sentry JS SDK maintainer here.

I checked out the logs (thanks, @zallarak!) and TL;DR, sourcemap upload is taking forever because more stuff is getting uploaded than necessary.

Because this is really an issue with our SDK, I've filed an issue in our repo with lots more detail (linked above). @sokra, @timneutkens, there are some questions there having to do with webpack and/or nextjs which might benefit from your expertise, if you wouldn't mind taking a look.

P.S. @zallarak - I notice in the logs that you're still using the @sentry/browser/@sentry/node combo. You might consider giving our new @sentry/nextjs SDK a try, as it's where fixes for this issue will land.

@zallarak
Copy link
Author

Thanks @lobsterkatie , we switched to @sentry/nextjs along with upgrading next to 11 and webpack to 5. Will run more benchmarks soon to see the changes.

@HazAT
Copy link

HazAT commented Jul 5, 2021

Just for reference, please follow this issue (getsentry/sentry-javascript#3769) we are going to improve this.

@kamilogorek
Copy link
Contributor

@zallarak can you please update our webpack plugin to 1.16.0 and post new logs? It should now include all necessary timings to see where the bottleneck is. Thanks!

lobsterkatie added a commit to getsentry/sentry-javascript that referenced this issue Aug 5, 2021
…les (#3845)

Though sourcemap uploading using `SentryWebpackPlugin` is generally quite fast, even when there are a lot of files[1], there are situations in which it can slow things down a lot[2].

[1] #3769 (comment)
[2] vercel/next.js#26588 (comment)

There are a number of reasons for that, but one of them is that, in the current state, `@sentry/nextjs`'s use of the plugin is pretty indiscriminate - it just uploads anything and everything in the `.next` folder (which is where nextjs puts all of its built files), during both client and server builds. The lack of specificity leads us to upload files we don't need, and the fact that we don't distinguish between client and server builds means that whichever one happens second not only uploads its own unnecessary files, it also uploads all of the files from the other build (which have already been uploaded), both necessary and unnecessary. More details in #3769.

This change makes it so that we're much more specific in terms of what we upload, in order to fix both the specificity and the duplication problems.

Notes:

- There's discussion in the linked issue about which non-user sources/maps to upload (things like code from webpack, nextjs itself, etc). In the end, I chose to restrict it to user code (almost - see below), for two reasons. First, non-user code is unlikely to change much (or often) between releases, so if third-party code were included, we'd be uploading lots and lots of copies of the same files. Second, though it's occasionally helpful to be able to see such code in the stacktrace, the vast majority of the time the problem lies in user code. For both reasons, then, including third-party files didn't feel worth it , either in terms of time spent uploading them or space spent storing them.

  (I say it's "almost" restricted to user code because, among other files, the server build generates bundles which are labeled only by numbers (`1226.js` and such), some of which are user code and some of which are third-party code, and - in this PR at least - I didn't include a way to filter out the latter while making sure to still include the former. This is potentially still a worthwhile thing to do, but the fact that it would mean actually examining the contents of files (rather than just their paths) makes it a more complicated change, which will have to wait for a future PR.)

-  In that issue, the topic of HMR (hot module reloading) files also came up. For the moment I also chose to skip those (even though they contain user code), as a) the plugin isn't meant for use in a dev environment, where every change triggers a new build, potentially leading to hundreds of sets of release files being uploaded, and b) we'd face a similar issue of needing to examine more than just the path in order to know what to upload (to avoid uploading all previous iterations of the code at each rebuild).

- Another small issue I fixed, while I was in the relevant code, was to prevent the webpack plugin from injecting the release into bundles which don't need it (like third-party code, or any bundle with no `Sentry.init()` call). Since this lines up exactly with the files into which we need to inject `sentry.server.config.js` or `sentry.client.config.js`, I pulled it out into a function, `shouldAddSentryToEntryPoint()`.

Fixes #3769
Fixes vercel/next.js#26588
@lobsterkatie
Copy link
Contributor

lobsterkatie commented Aug 6, 2021

FYI, all - 6.11.0 is out with this fix. I did some testing in my small example app, to see how many files would be uploaded under different scenarios, both under 6.10.0 and 6.11.0 (target: est means target: experimental-serverless-trace):

@sentry/nextjs 6.10.0              @sentry/nextjs 6.11.0

                   next 10, webpack 4

target: server - 90                target: server - 44
target: est - 86                   target: est - 40

                    next 10, webpack 5

target: server - 100               target: server - 36
target: est - 89                   target: est - 56

                    next 11, webpack 4

target: server - 90                target: server - 44
target: est - 86                   target: est - 40

                    next 11, webpack 5

target: server - 86                target: server - 36
target: est - 89                   target: est - 55

(In the above tests, I started with a clean slate each time. The change will be more dramatic for folks using next 10 and either running yarn build after yarn dev, or running yarn build multiple times, since next 10 doesn't nuke the .next folder in between builds, which can lead to a buildup of artifacts.)

Please give it a try and let me know how you do!

[UPDATE: If you've changed the default and don't suppress the SentryWebpackPlugin logs, under webpack 4 you may see an error about a missing file path. It's a false positive - nothing is actually broken. A fix for this is in the works.]

@zallarak
Copy link
Author

We're on webpack 5 and that builds are going well. we upgraded to next 11 and webpack 5 at the same time, and in doing so changed the way we did sourcemap generation, so it's hard to say exactly which thing was slowing us down before. right now we're pretty sure our build time bottlenecks are due to some specific node_modules dependencies and not due to vercel.

@leerob
Copy link
Member

leerob commented Aug 18, 2021

Awesome @zallarak! Really glad updating to latest Next.js and webpack has improved things 😄

If you have further questions, please let us know!

@balazsorban44
Copy link
Member

This issue has been automatically locked due to no recent activity. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@vercel vercel locked as resolved and limited conversation to collaborators Jan 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants