Skip to content

Conversation

@vladanpaunovic
Copy link
Contributor

Today, when adding a Sentry integration trough the Vercel marketplace, we set Sentry related env vars only for the production environment.

This is not great because Vercel automatically integrates with Github and once users next.js app (hosted on Vercel) adds Sentry, we break their deployments, like this:

Screen Shot 2022-08-10 at 12 35 34

To fix this, I propose we start storing our env vars in all Vercel environments: production, development and preview

@vladanpaunovic vladanpaunovic requested a review from a team August 10, 2022 10:37
@vladanpaunovic vladanpaunovic self-assigned this Aug 10, 2022
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Aug 10, 2022
@armenzg
Copy link
Member

armenzg commented Aug 10, 2022

Passing it to @ceorourke since I can't figure out what the consequences of this change would be.

"key": key,
"value": value,
"target": ["production"],
"target": ["production", "preview", "development"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we make this a constant or ENUM?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, this is the only place where we need it in this file and I think it is okay to make it explicit like this.

We could, of course, make a constant out of this, and then share this constant with our test in test_integration.py but :) :

  1. I don't know Python that much
  2. I don't know if that is a good practice to assert against a variable that is constant in code

@leeandher
Copy link
Member

leeandher commented Aug 10, 2022

@vladanpaunovic, so by default we only have the production target because this is generating a release to send over to Sentry. We probably do not want to be sending releases over to Sentry for the preview and development environments by default, since those often are not interacted with directly by users. These are just the preview deploys and branch deploys that Vercel enables for developers.

I believe you can disable attempting to create releases for these environments directly in the Next.js SDK.

@vladanpaunovic
Copy link
Contributor Author

@vladanpaunovic, so by default we only have the production target because this is generating a release to send over to Sentry. We probably do not want to be sending releases over to Sentry for the preview and development environments by default, since those often are not interacted with directly by users. These are just the preview deploys and branch deploys that Vercel enables for developers.

I believe you can disable attempting to create releases for these environments directly in the Next.js SDK.

Thanks @leeandher,

This is how I look at it, from a simple user's perspective:

  1. I install Sentry integration on Vercel
  2. I expect that everything works, but it doesn't. Instead, there is an error thrown
  3. The only way to fix the error is to add these env variables manually

TLDR; People will end up adding these env vars regardless. The difference that this PR makes is that it is smooth for them and removes frustrations.

@vladanpaunovic
Copy link
Contributor Author

FYI: I will review this with the team and think about a better solution. Will update this PR accordingly in a day or two.

@lobsterkatie
Copy link
Member

@vladanpaunovic, so by default we only have the production target because this is generating a release to send over to Sentry. We probably do not want to be sending releases over to Sentry for the preview and development environments by default, since those often are not interacted with directly by users. These are just the preview deploys and branch deploys that Vercel enables for developers.

I believe you can disable attempting to create releases for these environments directly in the Next.js SDK.

We discussed this as a team and this is correct - we shouldn't be generating a release and uploading sourcemaps in non-prod vercel deployments. Fixing that specifically is easy, but we also might consider ways to provide better guidance when this error happens, whether in vercel or not. That might mean changes to the nextjs SDK, but it might make more sense to do it directly in the webpack plugin. Needs further investigation.

@lobsterkatie
Copy link
Member

UPDATE: Here's the dumb/fast fix for vercel specifically: getsentry/sentry-javascript#5603.

I'm not going to close this yet, because I think a more general solution is still worth discussing.

@lobsterkatie
Copy link
Member

Closing this in favor of getsentry/sentry-javascript#5619, which describes the remaining issues.

@lobsterkatie lobsterkatie deleted the fix-vercel-integration branch August 19, 2022 20:15
@github-actions github-actions bot locked and limited conversation to collaborators Sep 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Component: Integrations Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants