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

feat(remix): Export a manual wrapper for custom Express servers. #5524

Merged
merged 7 commits into from Aug 12, 2022

Conversation

onurtemizkan
Copy link
Collaborator

@onurtemizkan onurtemizkan commented Aug 4, 2022

Remix allows custom server declarations and exports createRequestHandler functions from each adapter. In some cases, createRequestHandler functions are used before we have a chance to instrument them.

Auto-instrumentation wraps the core createRequestHandler from @remix/server-runtime, which all the adapters use. Each adapter may have their own APIs, so we can't directly re-export our already implemented wrapper.

This PR implements and exports a manual wrapper for Express adapters.

Also:

  • Moves Remix-related type declarations to its own file, as it seems it will continue to grow.

One thing I noticed:
A custom express server is still instrumented if it runs on the same thread as the runtime. For example, the tests run on a single Jest thread, and we can't reproduce this because of that.

If this wrapper is used when it's not needed, loader, action and documentRequest spans are reported twice. Is there a preferred way to prevent multiple wrapping? (I saw __sentry_wrapped__ but don't know if that's applicable here.)

@github-actions
Copy link
Contributor

github-actions bot commented Aug 5, 2022

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 19.39 KB (+0.02% 🔺)
@sentry/browser - ES5 CDN Bundle (minified) 59.99 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 17.96 KB (+0.01% 🔺)
@sentry/browser - ES6 CDN Bundle (minified) 52.88 KB (0%)
@sentry/browser - Webpack (gzipped + minified) 19.73 KB (0%)
@sentry/browser - Webpack (minified) 64.21 KB (0%)
@sentry/react - Webpack (gzipped + minified) 19.75 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified) 44.16 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 25.79 KB (0%)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 24.04 KB (+0.01% 🔺)

@AbhiPrasad AbhiPrasad added this to the Sentry Remix SDK milestone Aug 5, 2022
@onurtemizkan onurtemizkan force-pushed the onur/express-manual-wrapper branch 3 times, most recently from eed4b8f to 68a0569 Compare August 10, 2022 15:35
@onurtemizkan onurtemizkan marked this pull request as ready for review August 10, 2022 15:41
@AbhiPrasad
Copy link
Member

If this wrapper is used when it's not needed, loader, action and documentRequest spans are reported twice. Is there a preferred way to prevent multiple wrapping

We should be able to use __sentry_wrapped__ - just set it on the wrapped function so we don't wrap multiple times. We could also just store some flags internally, and flip them the first time we wrap - then we can just check those again.

@onurtemizkan
Copy link
Collaborator Author

@AbhiPrasad, the updated tests do not test the manual wrapper by itself. (As I could not recreate the scenario with the current testing structure). But they are testing when we use a manual wrapper on an already instrumented backend.

@AbhiPrasad
Copy link
Member

@onurtemizkan That should be fine, we can also dogfood with vanguard to make sure we have added confidence.

@AbhiPrasad AbhiPrasad enabled auto-merge (squash) August 12, 2022 17:46
@AbhiPrasad AbhiPrasad merged commit 1718e98 into master Aug 12, 2022
@AbhiPrasad AbhiPrasad deleted the onur/express-manual-wrapper branch August 12, 2022 17:53
@justinwaite
Copy link

justinwaite commented Aug 12, 2022

Great timing for my company as we are currently implementing Sentry in our newest app which happens to be using Remix (wrapped by express 😄). Looking forward to the next release!

@AbhiPrasad
Copy link
Member

Awesome news @justinwaite - we’ll have a release out early next week!

@onurtemizkan We’ll need to add some docs, can you take care of that?

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

Successfully merging this pull request may close these issues.

None yet

3 participants