getsentry / sentry-javascript

Official Sentry SDKs for JavaScript
https://sentry.io
MIT License
8.01k stars 1.59k forks source link

[v9] Change default of `onlyIncludeInstrumentedModules` in Node SDK #14332

Open mydea opened 6 days ago

mydea commented 6 days ago

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({
  skipOpenTelemetryInstrumentation: 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 skipOpenTelemetryInstrumentation: true): This is the tricky scenario.

There are a few possible ways we could go there:

  1. If skipOpenTelemetryInstrumentation, registerEsmLoaderHooks is automatically false (so we never set up the loader for users that do skipOpenTelemetryInstrumentation. 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 skipOpenTelemetryInstrumentation 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 commented 6 days ago

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 commented 6 days ago

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?