hasTracingEnabled doesn't handle undefined values correctly

nwalters512 commented 4 months ago

Is there an existing issue for this?

[X] I have checked for existing issues https://github.com/getsentry/sentry-javascript/issues
[X] I have reviewed the documentation https://docs.sentry.io/
[X] I am using the latest SDK release https://github.com/getsentry/sentry-javascript/releases

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/node

SDK Version

8.0.0

Framework Version

No response

Link to Sentry event

No response

SDK Setup

const config = await dynamicallyLoadConfigSomehow();

Sentry.init({
  tracesSampleRate: config.sentryTracesSampleRate ?? undefined
});

Steps to Reproduce

Add logs to import-in-the-middle so that you can tell when it's being imported.
Initialize Sentry with tracesSampleRate: undefined.
Observe that even though I don't want Sentry to enable any tracing instrumentation, import-in-the-middle is still loaded by Sentry and instrumentation is still added.

Expected Result

I would expect that an undefined value for tracesSampleRate would be interpreted as "don't enable tracing".

Actual Result

The mere existence of a tracesSampleRate value in the Sentry init options, regardless of what the value is, is interpreted as "enable tracing instrumentation":

https://github.com/getsentry/sentry-javascript/blob/d5568d8efe799dc99b35c21f027898d29a29071e/packages/core/src/utils/hasTracingEnabled.ts#L21

I think that check should probably look more like this:

return !!options && (options.enableTracing || options.tracesSampleRate != undefined || options.tracesSampler != undefined);

I'd be happy to open a PR with this if a maintainer can give the go-ahead!

mydea commented 4 months ago

Hey,

the background for this is based on browser-land, where we want to do certain things only if performance is set in any way. We may revisit this, but for now I will fix this specifically in the node init check for performance integrations, and only load those if this is not undefined!

mydea commented 4 months ago

Hey,

we have updated this in the SDK, so that even if this is set to undefined, performance instrumentation will not be added by default.

(FWIW, we have not updated the hasTracingEnabled() check yet, because we need to double check to make sure there are no other effects there, but we changed this for the perf instrumentation check itself).

I will leave the issue open still because I think the fundamental problem it poses remains valid!

getsentry / sentry-javascript