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

[Remix]: Sentry server instrumentation breaks loadContext parameter to handleRequest #8265

Closed
3 tasks done
dawnmist opened this issue May 31, 2023 · 3 comments
Closed
3 tasks done
Assignees

Comments

@dawnmist
Copy link
Contributor

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/remix

SDK Version

^7.53.1

Framework Version

Remix 1.16.1

Link to Sentry event

https://support-budget.sentry.io/issues/4217820559/?project=4505236358037504&referrer=project-issue-stream

SDK Setup

Server-side (these break the application):

Sentry.init({
	dsn: ENV.SENTRY_DSN,
	// Set tracesSampleRate to 1.0 to capture 100%
	// of transactions for performance monitoring.
	// We recommend adjusting this value in production
	tracesSampleRate: 1.0,
})

(Also reproduced with the Prisma integration on the server, but the issue occurs regardless of any integrations provided).

Client-side (though testing has shown that the site runs ok with only the client-side integration present, so client options are only included for completeness - they do not affect the bug):

Sentry.init({
	dsn: ENV.SENTRY_DSN,
	integrations: [
		new Sentry.BrowserTracing({
			// Set `tracePropagationTargets` to control for which URLs distributed tracing should be enabled
			tracePropagationTargets: ['localhost'],
			routingInstrumentation: Sentry.remixRouterInstrumentation(
				useEffect,
				useLocation,
				useMatches,
			),
		}),
		// Replay is only available in the client
		new Sentry.Replay(),
	],

	// Set tracesSampleRate to 1.0 to capture 100%
	// of transactions for performance monitoring.
	// We recommend adjusting this value in production
	tracesSampleRate: 1.0,

	// Capture Replay for 10% of all sessions,
	// plus for 100% of sessions with an error
	replaysSessionSampleRate: 0.1,
	replaysOnErrorSampleRate: 1.0,
})

I have created a sample repository with a reproduction of the problem, starting from Kent C Dodd's Epic Web Stack.

Repository: https://github.com/dawnmist/sentry-remix-load-context-undefined.git

Branches:

Note: the linked Sentry error is from my actual application when I was trying to trace what was going wrong, as I had tried to implement the nonce handling from the Epic Stack into my existing application then found that it completely broke. It took several hours and completely replacing my entry.server.ts file with a fresh one from that stack, then slowly adding changes back one at a time, to track the issue down to being due to the Sentry instrumentation. Once I had that, I then did a test using a fresh initialisation of the stack with only the sentry changes added to establish that it really was the addition of the sentry instrumentation that was causing the issue and to create a reproducible example I could provide for this report.

Steps to Reproduce

Working project prior to Sentry instrumentation:

  • Check out the main branch of the repository
  • Copy the .env.example file to .env
  • Run npm run install
  • Run npm run dev - you should get output like:
🔶 Mock server installed
⚠️  Port 3000 is not available, using 3008 instead.
🚀  We have liftoff!
Local:            http://localhost:3008
On Your Network:  http://192.168.1.20:3008
Press Ctrl+C to stop
App server took 1.5s
  • Load the url specified in the console log in a browser window
    ==> you should see the "EPIC STACK" landing page.

To reproduce the Sentry problem:

  • Stop running the server
  • Check out the branch sentry-remix-load-context-undefined
  • Copy the .env.example file to .env, and update the (new to the branch) SENTRY_DSN variable to something you can use
  • Run npm run install to pick up the sentry libraries
  • Run npm run dev - you should get the same output as before
  • Load the url specified in the console log in a browser window
    ==> The page throws the error:
Unexpected Server Error

TypeError: Cannot read properties of undefined (reading 'cspNonce')

Testing variations locally has shown that if either of the modifications to ./app/entry.server.ts (Sentry.init on the remix server) or ./server/index.ts (using wrapExpressCreateRequestHandler) are present the remix server will not receive the value created for the loadContext parameter to the handleRequest function in ./app/entry.server.ts file. The client side Sentry.init does not affect the issue at all.

Expected Result

The function handleRequest in ./app/entry.server.ts should be properly receiving the context information that is being set in the express server in ./server/index.ts regardless of whether Sentry instrumentation has been applied to the server.

Actual Result

If the server has either or both of the Sentry.init on the ./app/entry.server.ts file OR wrapExpressCreateRequestHandler used in an express server that runs the remix server, the context being set with the getLoadContext option to the createRequestHandler function is not passed through to the remix application in the handleRequest function's loadContext parameter.

@scefali
Copy link
Member

scefali commented May 31, 2023

This is impacting me as well and is blocking me from adding Sentry to EpicStack. Here's the branch where you can recreate the bug: https://github.com/scefali/epic-stack/tree/scefali/sentry

dawnmist added a commit to dawnmist/sentry-javascript that referenced this issue Jun 1, 2023
dawnmist added a commit to dawnmist/sentry-javascript that referenced this issue Jun 1, 2023
@Lms24
Copy link
Member

Lms24 commented Jun 1, 2023

Hi @dawnmist thanks for reporting! We'll take a look at this soon (cc @onurtemizkan).

@Lms24 Lms24 assigned Lms24 and onurtemizkan and unassigned Lms24 Jun 1, 2023
Lms24 pushed a commit that referenced this issue Jun 1, 2023
…ion (#8268)

Ensure that the loadContext created in the remix server is passed
through to `app/entry.server.ts` in the document request handler.

Fixes issue #8265.
@Lms24
Copy link
Member

Lms24 commented Jun 1, 2023

closed via #8268

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants