Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always add browserTracingIntegration in Meta Framework SDKs #13010

Closed
Lms24 opened this issue Jul 23, 2024 · 6 comments
Closed

Always add browserTracingIntegration in Meta Framework SDKs #13010

Lms24 opened this issue Jul 23, 2024 · 6 comments

Comments

@Lms24
Copy link
Member

Lms24 commented Jul 23, 2024

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:

  • Tracing without performance works, i.e. tracing headers are propagated to get a trace for an error but no spans are created or sent to Sentry
  • The error transaction field is populated via the framework's browserTracingIntegration setting scope.transactionName. This means higher quality transaction names out of the box.
  • Bundle size isn't wasted like before

Implementation

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
Copy link
Member

s1gr1d commented Jul 23, 2024

For reference: #9095

@s1gr1d s1gr1d self-assigned this Jul 30, 2024
@s1gr1d
Copy link
Member

s1gr1d commented Aug 5, 2024

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
Copy link
Member

mydea commented Aug 5, 2024

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
Copy link
Member Author

Lms24 commented Aug 5, 2024

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

@s1gr1d
Copy link
Member

s1gr1d commented Aug 5, 2024

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
Copy link
Member Author

Lms24 commented Aug 5, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants