feat(node): Option to only wrap instrumented modules

[timfish]

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: { onlyIncludeInstrumentedModules: true },
});

So far I've only changed the express integration test to use this new mode.

[github-actions]

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% 🔺)

[mydea]

m: Are we sure we want to put this option in here? The API feels a bit "clunky" to me 🤔 also, does this not conflict with include/exclude options, somehow (or do they play together?)?

What about making this a top-level option? Esp. if we think this may become a default at some point...? (no strong feelings, just want us to consider this!)

[timfish]

does this not conflict with include/exclude options, somehow (or do they play together?)?

include/exclude were always mutually exclusive because It doesn't make much sense to use both. You could potentially enable this new onlyIncludeInstrumentedModules option while still supplying include and this would only include instrumented modules, plus whatever the user supplied. This might be useful if a user is adding an instrumentation later.

Are we sure we want to put this option in here? What about making this a top-level option? Esp. if we think this may become a default at some point...?

Yep valid points. I don't feel that strongly either way.

[lforst]

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. 😄

[timfish]

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.

[AbhiPrasad]

With the current changes I think this is reasonable to merge in, release, and document. @mydea @lforst does that seem reasonable to you?

[timfish]

Shall I mark it as experimental? I'm now thinking the entire registerEsmLoaderHooks should have been marked as experimental from the start!

[mydea]

let's ship it! I think I would still like to have a top-level option for this (esp. if we also want to put this into the default onboarding options, which I think would make sense enventually?), but IMHO we can also add this later as a kind of alias for this more in-depth option.

[mydea]

so, should we merge this? 😅

[timfish]

If we're unsure, I can mark this new option as experimental!

[mydea]

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?

[Xhale1]

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!

[lforst]

I am just gonna press the button