[v9] Change default of onlyIncludeInstrumentedModules in Node SDK

[mydea]

Description

Today, by default we instrument all packages with import-in-the-middle in ESM.

For v9, we may flip the default of onlyIncludeInstrumentedModules from false to true, so that by default only the things you are adding instrumentation for in the --import hook are wrapped. This will work for everybody using the SDK to handle OTEL for them; only for users with a custom OTEL setup this may have problems.

To clarify, having this flag enabled can lead to issues in rare cases, e.g. if users run node --import instrument.mjs app.mjs with code like this:

// instrument.mjs 
import * as Sentry from '@sentry/node';

Sentry.init({
  skipOpenTelemetrySetup: true,
});

customOtelSetup();

// app.mjs
startApp();

// Now add more otel instrumentation, outside of --import
registerInstrumentations({ instrumentations });
// this will only affect stuff that is imported _after_ this code, so it will not work for many/most cases anyhow,
// but it is technically possible...

So we have a few possible permutations here, which are unfortunate:

1. Default case: We register the loader hook & our default OTEL setup: ✅ onlyIncludeInstrumentedModules: true will always work and makes sense
2. Users set registerEsmLoaderHooks: false: Users have to set up their own OTEL loader hook, in which case they have to configure things like what onlyIncludeInstrumentedModules themselves. ✅
   onlyIncludeInstrumentedModules is ignored anyhow, so default does not matter.
3. Users use the Sentry ESM loader hook, but their own OTEL setup (so registerEsmLoaderHooks: true (default) and skipOpenTelemetrySetup: true): This is the tricky scenario.

There are a few possible ways we could go there:

1. If skipOpenTelemetrySetup, registerEsmLoaderHooks is automatically false (so we never set up the loader for users that do skipOpenTelemetrySetup. While this kind of makes sense, it is definitely breaking for users of this setup (instrumentation will just stop). While setting this up yourself technically makes sense if you are shipping your own OTEL SDK, some users probably rely on us doing this for them right now.
2. Just always default onlyIncludeInstrumentedModules to true, and if users rely on a scenario where they register instrumentation after --import, they need to flip this themselves. --> it's worth noting that I suspect even for custom otel setups this is a rather rare (but not impossible) scenario.
3. Use a different default for onlyIncludeInstrumentedModules depending on if skipOpenTelemetrySetup is enabled

I think I would tend to solution 2 🤔 this is breaking but unlikely to affect a lot of users - it would only affect

a) People using a custom otel setup
b) ... who ALSO register instrumentation outside of --import

And those people only have to flip the setting back to false to get stuff working again 🤔

[timfish]

Looks good!

We could try and detect those who will see this as a breaking change and warn them to set this to false long before v9.

[mydea]

Looks good!

We could try and detect those who will see this as a breaking change and warn them to set this to false long before v9.

Is this possible? if so, let's do this I'd say?

[lforst]

I would go with the option to flip the default for onlyIncludeInstrumentedModules to true, however we need a callout somewhere that you need to set onlyIncludeInstrumentedModules: false to use non-off-the-sentry-shelf OTEL instrumentations. Makes me think about whether we should name it something like registerEsmLoaderHooks.registerForAllImports or something similar.

Also, I think the option to base behavior off of what skipOpenTelemetrySetup is set to is simply wrong, because those options are technically unrelated. It is just that when skipOpenTelemetrySetup is set to true, it is way more likely that you would want onlyIncludeInstrumentedModules to be false.

[lforst]

I feel like we have to put even more clarity behind this. Some status quo for all of our supported use-cases with desired states:

1. User uses Sentry in combination with a complete custom OTEL setup:

• Indicator: The user has configured skipOpenTelemetrySetup: true
• In this case, we can rely on the fact that the user adds loader hooks themselves because it is part of the ESM setup for OTEL.
• We can still register our own ESM loader hooks limited to imports that are required for our instrumentations.

2. User uses Sentry but wants to add individual OTEL instrumentation:

• Indicator: The user either uses Sentry.addOpenTelemetryInstrumentation(...) or otel.registerInstrumentations(...)
• We should warn if the user calls Sentry.addOpenTelemetryInstrumentation() in a place where it will no-op (i.e. outside of an --import call when in ESM, but only when ESM hooks haven't been registered in another way) with a message like "You called addOpenTelemetryInstrumentation within a context that is not supported. Please either register OpenTelemetry loader hooks, or call addOpenTelemetryInstrumentation within an --import'ed module" (exact wording TBD).
• We don't care if people use otel.registerInstrumentations(...).

3. User uses Sentry without any custom OTEL stuff:

• Indicator: none
• We should only ever apply ESM wrapping to modules that we actually need to wrap - meaning we can also just apply loader hooks for modules that are required for our instrumentations.

All of the above begs the question: Do we need the onlyIncludeInstrumentedModules option at all? It seems like it should always be true by default and none of our supported use-cases require unsetting it.

---

Tests we need to conduct to verify the above:

1. Set up IITM loader in a way that wraps everything, then re-register IITM with limited modules, then check if everything is still wrapped. This is to verify user group 1.

• Ok apparently this is a problem and causes things to be done more than one time Sentry creates three traces for a single http request (ESM, custom opentelemetry setup) #14065 (comment)

3. Check whether it is possible to detect whether no loader hooks have been registered yet so we can emit a warning. This is to verify user group 2 so we can emit a warning when Sentry.addOpenTelemetryInstrumentation(...) is called.
4. Check whether it is possible to detect if Sentry.addOpenTelemetryInstrumentation(...) is called within an unsupported context (ie. inside a non---imported module). This is also to verify user group 2 so we can warn.

[lforst]

We settled on the following:

• We are deprecating Sentry.addOpenTelemetryInstrumentation() because it is a foot gun timing-wise. It is replaced with an option in Sentry.init() and the Node Client called openTelemetryInstrumentations where users can provide a list of OTEL instrumentations they want to add.
• Next, because we think it works well, we make onlyIncludeInstrumentedModules: true the default behavior.
• In fact onlyIncludeInstrumentedModules: true is so awesome, we want it to be the only behavior. Allowing to disable makes little sense API-wise because it would really only affect OTEL, which is out of the Scope of our SDK.
• We will deprecate/remove the fine-grained options for registerEsmLoaderHooks. The hooks will still be applied by default, but users can turn them off.
• Turning the hooks off will be the recommended solution for people with a custom OTEL setup. We will say this in our docs and the JSDoc for skipOpenTelemetrySetup.