getsentry / sentry-javascript

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

Always add `browserTracingIntegration` in Meta Framework SDKs #13010

Open Lms24 opened 1 month ago

Lms24 commented 1 month ago

Problem Statement

Currently, our meta-framework SDKs (in contrast to the frontend framework and base browser SDKs) automatically add browserTracingIntegration if:

  1. The __SENTRY_TRACING__ tree-shaking flag is not replaced at build time
  2. hasTracingEnabled returns true, meaning tracesSampleRate, tracesSampler or enableTracing options are set

The second (2) condition unfortunately breaks "Tracing without Performance" which in our frontend framework SDKs is enabled as soon as browserTracingIntegration is added but no sample rates are set. Adding to this, there is no bundle size advantage in not adding browserTracingIntegration because the hasTracingEnabled check is performed at runtime. This results in the integration code always being added to the bundle.

We realized this while working on and reviewing #13005

Solution Brainstorm

We're going to change the behaviour here to remove condition 2 entirely. Meaning, by default, browserTracingIntegration will always be added, unless users configure the tree shaking flag (1). This change should be implemented in all our meta framework SDKs that currently automatically add browserTracingIntegration.

The benefits of always adding the integration are:

Implementing this behaviour requires two changes:

  1. Remove the hasTracingEnabled guard in the SDK initialization
  2. Add a higher-level option to the meta framework's build config (e.g. sentrySvelteKit vite plugin or Astro integration options) for users to easily opt out of tracing. This option should pass the boolean to the Sentry bundler plugin's bundleSizeOptimizations.excludePerformanceMonitoring option.
s1gr1d commented 1 month ago

For reference: https://github.com/getsentry/sentry-javascript/issues/9095

s1gr1d commented 1 month ago

The function hasTracingEnabled checked if certain options are truthy. tracesSampleRate and tracesSampler still make sense as they allow modifying the enabled tracing configuration. But what should be the behavior for enableTracing: false?

mydea commented 1 month ago

The function hasTracingEnabled checked if certain options are truthy. tracesSampleRate and tracesSampler still make sense as they allow modifying the enabled tracing configuration. But what should be the behavior for enableTracing: false?

IMHO this check can remain as it is? as soon as this is defined that is enough, for all of these fields!

Lms24 commented 1 month ago

Just to confirm: Are we talking about the correctness of hasTracingEnabled itself or about how it us used to guard adding the browserTracingIntegrations?

s1gr1d commented 1 month ago

No, not about the correctness of hasTracingEnabled, but what options it considered before and which action to take now based on the options, but without the function. For example, what should be the outcome if enableTracing is false?

Lms24 commented 1 month ago

I'd say we can safely ignore enableTracing here because there should be a check within browserTracingIntegration if a span should be started and even if not, a negative sampling decision would be returned based on enableTracing (but preferrably other options, given that enableTracing is deprecated).

If users would still like to avoid TwP scenarios there's still another escape hatch: tracePropagationTargets: [] means tracing headers aren't attached to any outgoing requests.