sentry-javascript
sentry-javascript copied to clipboard
getsentry
feat(node): Option to only wrap instrumented modules
Likely closes many issues but I don't want to auto-close anything specific here. We should probably confirm the issues are closed individually.
import-in-the-middle by default wraps every ES module with a wrapping module that later allows it exports to be modified. This has issues though because the wrapping output of import-in-the-middle is not compatible with all modules.
To help work around this I added a feature to import-in-the-middle that allows us to only wrap modules that we specifically need to instrument.
import * as Sentry from '@sentry/node';
Sentry.init({
dsn: '__DSN__',
registerEsmLoaderHooks: { onlyHookedModules: true },
});
So far I've only changed the express integration test to use this new mode.
size-limit report 📦
| Path | Size |
|---|---|
| @sentry/browser | 22.45 KB (0%) |
| @sentry/browser (incl. Tracing) | 34.22 KB (0%) |
| @sentry/browser (incl. Tracing, Replay) | 70.28 KB (0%) |
| @sentry/browser (incl. Tracing, Replay) - with treeshaking flags | 63.62 KB (0%) |
| @sentry/browser (incl. Tracing, Replay with Canvas) | 74.68 KB (0%) |
| @sentry/browser (incl. Tracing, Replay, Feedback) | 87.26 KB (0%) |
| @sentry/browser (incl. Tracing, Replay, Feedback, metrics) | 89.11 KB (0%) |
| @sentry/browser (incl. metrics) | 26.75 KB (0%) |
| @sentry/browser (incl. Feedback) | 39.37 KB (0%) |
| @sentry/browser (incl. sendFeedback) | 27.06 KB (0%) |
| @sentry/browser (incl. FeedbackAsync) | 31.7 KB (0%) |
| @sentry/react | 25.22 KB (0%) |
| @sentry/react (incl. Tracing) | 37.22 KB (0%) |
| @sentry/vue | 26.6 KB (0%) |
| @sentry/vue (incl. Tracing) | 36.06 KB (0%) |
| @sentry/svelte | 22.58 KB (0%) |
| CDN Bundle | 23.64 KB (0%) |
| CDN Bundle (incl. Tracing) | 35.88 KB (0%) |
| CDN Bundle (incl. Tracing, Replay) | 70.31 KB (0%) |
| CDN Bundle (incl. Tracing, Replay, Feedback) | 75.57 KB (0%) |
| CDN Bundle - uncompressed | 69.37 KB (0%) |
| CDN Bundle (incl. Tracing) - uncompressed | 106.31 KB (0%) |
| CDN Bundle (incl. Tracing, Replay) - uncompressed | 218.16 KB (0%) |
| CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed | 230.99 KB (0%) |
| @sentry/nextjs (client) | 37.07 KB (0%) |
| @sentry/sveltekit (client) | 34.79 KB (0%) |
| @sentry/node | 115.57 KB (+0.81% 🔺) |
| @sentry/node - without tracing | 90.26 KB (+1.05% 🔺) |
| @sentry/aws-serverless | 99.43 KB (+0.94% 🔺) |
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
ah, I need to add some integration tests that ensure that this feature is in fact working and limiting the iitm wrapping
I was a bit torn on the option because I kinda thought that it should just be the default behaviour and we shouldn't give people the option at all, because we do not define a contract anywhere (code or docs) that the Sentry SDK will provide you with an OTEL setup you can bounce off-of. Then I realized, dang we probably actually want people to be able to add random OTEL instrumentation.
I still have the thought that if people jump through the hoops to add other (non-sentry-approved) OTEL instrumentation they basically must have read the paragraphs around ESM in the OTEL docs and have likely added their own hook, which would be an argument for making this the default.
Regardless, we should at least write down in the PR description that this PR aims to solve that IITM is breaking random packages that are not contributing to any instrumentation. 😄
Added some mode context to the description!
I was a bit torn on the option because I kinda thought that it should just be the default behaviour and we shouldn't give people the option
Unfortunately it has the potential to break things for too many users for it to be released default enabled as a bug fix 😭
I don't even think users adding otel instrumentations later is the biggest risk. Users might not even be using --import and import-in-the-middle can make instrumentation work in this case. Users upgrading from v7 might have not read the docs, bumped the version and everything is working and this change as the default would could that.
Shall I mark it as experimental? I'm now thinking the entire registerEsmLoaderHooks should have been marked as experimental from the start!
As long as it's opt-in I am totally fine with adding this as is...? As users need to opt-in to it, feels safe enough?
Hello hello! Is this on the roadmap for a near-future Sentry release? I'm eager to adopt v8 at my company and wanted to wait for this PR to merge before attempting the migration again.
No worries if not, thank you all for your continued contributions to the js ecosystem!
