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

fix(nextjs): Use domains to prevent scope bleed #3788

Merged
merged 2 commits into from Jul 14, 2021

Conversation

lobsterkatie
Copy link
Member

@lobsterkatie lobsterkatie commented Jul 9, 2021

This PR uses domains to keep scope data attached to simultaneous requests from bleeding between those requests. (For example, when testing tracing on API routes deployed to Vercel, in certain cases all transactions were using the same traceId, even though the original transaction corresponding to that id had long since finished.)

@lobsterkatie lobsterkatie force-pushed the kmclb-nextjs-fix-api-scope-bleed branch from 35fa46c to 3491bfe Compare July 9, 2021 07:08
@lobsterkatie lobsterkatie changed the title Kmclb nextjs fix api scope bleed fix(nextjs): Use domains to prevent scope bleed Jul 9, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Jul 9, 2021

size-limit report

Path Size
@sentry/browser - CDN Bundle (gzipped) 21.46 KB (-0.01% 🔽)
@sentry/browser - Webpack 22.47 KB (0%)
@sentry/react - Webpack 22.5 KB (0%)
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped) 28.95 KB (-0.01% 🔽)

@kamilogorek
Copy link
Contributor

Tbh I'm not sure if domain.run behaves correctly with async functions, as the API requires it to be a regular function, but as long as we can make all the tests pass, it should be fine.

@AbhiPrasad
Copy link
Member

We should think about using async_hooks instead of domains for the next sdk. We can also do it in another PR.

Copy link
Contributor

@kamilogorek kamilogorek left a comment

Choose a reason for hiding this comment

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

Although I agree with @AbhiPrasad that it might be a good playground for async_hooks

@lobsterkatie lobsterkatie force-pushed the kmclb-nextjs-fix-api-scope-bleed branch from 11df2e6 to d8cf291 Compare July 14, 2021 05:56
@lobsterkatie lobsterkatie merged commit 90eb35c into master Jul 14, 2021
@lobsterkatie lobsterkatie deleted the kmclb-nextjs-fix-api-scope-bleed branch July 14, 2021 23:30
masonmcelvain added a commit to iFixit/react-commerce that referenced this pull request Aug 9, 2022
We were clearing the scope because we were worried that different requests might pollute each other's scopes on the server.

Howevever, it looks like Sentry handles this for us, so we don't need to risk blowing away valuable debugging info:

getsentry/sentry-javascript#3788
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants