Skip to content

Nextjs "server Sentry.Init()" is never called if npm run build happened in different environment than hosting #3648

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

Closed
4 of 9 tasks
jtn-junedesk opened this issue Jun 4, 2021 · 9 comments · Fixed by #3649 or #3786
Assignees
Labels
Package: nextjs Issues related to the Sentry Nextjs SDK

Comments

@jtn-junedesk
Copy link

jtn-junedesk commented Jun 4, 2021

Package + Version

  • @sentry/browser
  • @sentry/node
  • raven-js
  • raven-node (raven for node)
  • other: @sentry/nextjs

Version:

6.5.1

Description

I use Github Actions to build my nextjs application, then deploy artifacts to a NodeJS Server hosted elsewhere.
https://nextjs.org/docs/deployment#nodejs-server

During npm run build @sentry/nextjs creates a file .env.local and sets an environment variable SENTRY_SERVER_INIT_PATH. The value of said variable is an absolute path i.e. C:\some_path_to_project\.next\server\sentry\initServerSDK.js

During npm run start process.env.SENTRY_SERVER_INIT_PATH is used to require initServerSDK.js. However, npm run start is called after artifacts have been uploaded to a different environment running a NodeJS server. Because of this, the absolute path of process.env.SENTRY_SERVER_INIT_PATH is no longer valid.

initServerSDK.js cannot be found and Sentry.Init() is never called - serverside logging does not happen.

Source code

setRuntimeEnvVars(projectDir, { SENTRY_SERVER_INIT_PATH: serverSDKInitOutputPath });

require(process.env.SENTRY_SERVER_INIT_PATH as string);

@edoko
Copy link

edoko commented Jun 4, 2021

I am having the same problem and same version.

I running the production mode, and it created .env.local, it has SENTRY_SERVER_INIT_PATH=/project_path/.next/server/sentry/initServerSDK.js.
But, .next/server/sentry is not here.
And Sentry logger is not working.

@AbhiPrasad AbhiPrasad added Package: nextjs Issues related to the Sentry Nextjs SDK Type: Bug and removed Status: Needs Triage labels Jun 4, 2021
@AbhiPrasad AbhiPrasad self-assigned this Jun 4, 2021
AbhiPrasad added a commit that referenced this issue Jun 4, 2021
This fixes a bug caused by next build being run on a different machine than next start. Because we've been setting the env var SENTRY_SERVER_INIT_PATH (which we do during build) as an absolute, rather than relative, path, changing machines has made that path invalid when it's later used during server startup. This switches to using a relative path, so that it will work regardless of the machine on which it's run.

Separately, it also fixes a bug where the path (absolute or relative) to the server init file was incorrect when using webpack 5. Under webpack 5, the path in the webpack config's output.path variable points to a different location (one directory deeper) than in webpack 4. To compensate for that difference, the config (as created by next) has a different output.filename value (one including ../ at the beginning), so that then combination points to the same spot as under webpack 4. This PR takes advantage of that by including filename in it derivation of the server init file's final location.

Closes #3648

Co-authored-by: Abhijeet Prasad <[email protected]>
Co-authored-by: Katie Byers <[email protected]>
@AbhiPrasad
Copy link
Member

Hey! Thank you for reporting, this was definitely an oversight on our part. We’ve merged in a fix for this bug, which will go out in the next release of the SDK.

I will update this issue when that release is out.

@jtn-junedesk
Copy link
Author

@AbhiPrasad Thank you for fixing this so quickly! Looking forward to the release 👍

@jtn-junedesk
Copy link
Author

@AbhiPrasad Looking through the code, I am a bit curious why you have chosen a 2 step model (create/consume) which creates a disk file dependency (.env.local).

Could instrumentServer.ts not simply do this:

import * as path from 'path';
import { SERVER_SDK_INIT_PATH } from './utils/config';

[...]

const serverSdk = path.join(this.server.serverBuildDir, config.SERVER_SDK_INIT_PATH);
require(serverSdk);

With this solution you would not need to create .env.local during build and could remove some complexity :)

@AbhiPrasad
Copy link
Member

I am a bit curious why you have chosen a 2 step model (create/consume) which creates a disk file dependency (.env.local).

Great question @jtn-junedesk. We chose to take this approach because we can't really make any assumptions where webpack will place Sentry assets. In addition, a user's custom setup might move things around also.

To get an idea of all the possible permutations of dir/path locations, take a look at all the conditionals here: https://github.com/vercel/next.js/blob/3f2379a9ff88dd4a1c775862006d233c1510e5fd/packages/next/build/webpack-config.ts#L928-L939. Serverless options also can change the path: https://github.com/vercel/next.js/blob/3f2379a9ff88dd4a1c775862006d233c1510e5fd/packages/next/build/webpack-config.ts#L303-L309. I'm sure they will add even more options in the future.

We decided that the 2-step solution was the easiest for us to implement while minimizing possible conflicts with user/default configurations. It is also much less likely to break if (when) next or webpack changes the way they do things in the future.

Despite what I said above, it might be possible to find a way to get the paths to work properly here. If someone finds a way, suggestions + PRs always welcome :)

@jtn-junedesk
Copy link
Author

Thank you for a great reply @AbhiPrasad - and for providing references.

However, from reading next-server.ts, my understanding is that next.js is already handling those scenarios (path locations, distDir, serverless) when they expose .serverBuildDir. If we use the property which next.js has already exposed, my feeling is that this would be the most safe option. This means that we have to make zero assumptions and give next.js 100% responsibility for telling us where the directory is.

If they add more options in the future, I would expect .serverBuildDir to reflect this.

You can see their current implementation here:
https://github.com/vercel/next.js/blob/0402fc459eb12a775372172a351f3a3f4fee83e3/packages/next/next-server/server/next-server.ts#L243-L246

I case this makes sense, I would gladly dedicate some time to creating a PR :)

@AbhiPrasad
Copy link
Member

AbhiPrasad commented Jun 12, 2021

@jtn-junedesk Your thoughts definitely make sense to me. If you could open a PR we can discuss further.

Thank you so much for helping us iterate on this sdk, it’s greatly appreciated!

@jacob-indieocean
Copy link

I would really prefer sentry did not modify my .env.local file (especially without documentation!).

I was really surprised to see that there and I have no idea if I should duplicate that manually in all my environments.

@lobsterkatie
Copy link
Member

Hi, @jacob-indieocean - we in fact no longer do.

Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: nextjs Issues related to the Sentry Nextjs SDK
Projects
None yet
5 participants