ref(tracing): Implement changes to new browserTracingIntegration (getsentry#10393)

mydea · web-flow · commit 7dc92604e4a9 · 2024-01-29T17:27:14.000+01:00
This implements changes to the new integration, based on feedback from
the team.

The main thing this addresses is the way we disable the default span
creation. The original idea was to make this work with custom router
instrumentations (e.g. for react router) that may be added as separate
integrations. This required us to keep track of if another, non-default
implementation has been added, to opt out of the defaults.

With this change, we do not do this anymore. Instead, the defaults
should be configured by wrapping integrations. Some examples for how
this should then work:

1. Angular:
```ts
export function browserTracingIntegration(
  options?: Parameters&lt;typeof originalBrowserTracingIntegration&gt;[0],
): Integration {
  // do not use default navigation, as we provide our own
  // actual code for this is in the angular service
  options.instrumentNavigation = false;
  return originalBrowserTracingIntegration(options);
}
```

2. Vue
```ts
export function browserTracingIntegration(
  options?: Parameters&lt;typeof originalBrowserTracingIntegration&gt;[0] &amp; { router?: VueRouter },
): Integration {

  if (options.router) {
    options.instrumentNavigation = false;
    const integration = originalBrowserTracingIntegration(options);
    const originalSetupAfterAll = integration.setupAfterAll;
    integration.setupAfterAll = (client: Client) =&gt; {
      setupVueRoutingInstrumentation(client); // some method
      originalSetupAfterAll(client);
    }
  }

  return originalBrowserTracingIntegration(options);
}
```

3. React
```ts
export function browserTracingIntegration(
  options?: Parameters&lt;typeof originalBrowserTracingIntegration&gt;[0] &amp; { router?: ReactRouterInstrumentation },
): Integration {

  if (options.router) {
    options.instrumentNavigation = false;
    options.instrumentPageLoad = false;
    const integration = originalBrowserTracingIntegration(options);
    const originalSetupAfterAll = integration.setupAfterAll;
    integration.setupAfterAll = (client: Client) =&gt; {
      // setup custom routing instrumentation
      options.router(client); // or whatever API we want there
      originalSetupAfterAll(client);
    }
  }

  return originalBrowserTracingIntegration(options);
}
```
diff --git a/packages/browser/src/index.ts b/packages/browser/src/index.ts
@@ -60,8 +60,6 @@ export {
   browserTracingIntegration,
   startBrowserTracingNavigationSpan,
   startBrowserTracingPageLoadSpan,
-  disableDefaultBrowserTracingNavigationSpan,
-  disableDefaultBrowserTracingPageLoadSpan,
 } from '@sentry-internal/tracing';
 export type { RequestInstrumentationOptions } from '@sentry-internal/tracing';
 export {
diff --git a/packages/tracing-internal/src/browser/browserTracingIntegration.ts b/packages/tracing-internal/src/browser/browserTracingIntegration.ts
@@ -74,12 +74,14 @@ export interface BrowserTracingOptions extends RequestInstrumentationOptions {
 
   /**
    * If a span should be created on page load.
+   * If this is set to `false`, this integration will not start the default page load span.
    * Default: true
    */
   instrumentPageLoad: boolean;
 
   /**
    * If a span should be created on navigation (history change).
+   * If this is set to `false`, this integration will not start the default navigation spans.
    * Default: true
    */
   instrumentNavigation: boolean;
@@ -143,9 +145,6 @@ const DEFAULT_BROWSER_TRACING_OPTIONS: BrowserTracingOptions = {
   ...defaultRequestInstrumentationOptions,
 };
 
-let shouldUseDefaultPageLoadSpan = true;
-let shouldUseDefaultNavigationSpan = true;
-
 /**
  * The Browser Tracing integration automatically instruments browser pageload/navigation
  * actions as transactions, and captures requests, metrics and errors as spans.
@@ -308,13 +307,6 @@ export const _browserTracingIntegration = ((_options: Partial<BrowserTracingOpti
 
       if (client.on) {
         client.on('startNavigationSpan', (context: StartSpanOptions) => {
-          // We check this inside of the hook handler, so that if a custom instrumentation triggers this,
-          // we don't need to check this option in the instrumentation, but can simply invoke it
-          // without needing to know the options of this integration
-          if (!options.instrumentNavigation) {
-            return;
-          }
-
           if (activeSpan) {
             DEBUG_BUILD && logger.log(`[Tracing] Finishing current transaction with op: ${spanToJSON(activeSpan).op}`);
             // If there's an open transaction on the scope, we need to finish it before creating an new one.
@@ -324,13 +316,6 @@ export const _browserTracingIntegration = ((_options: Partial<BrowserTracingOpti
         });
 
         client.on('startPageLoadSpan', (context: StartSpanOptions) => {
-          // We check this inside of the hook handler, so that if a custom instrumentation triggers this,
-          // we don't need to check this option in the instrumentation, but can simply invoke it
-          // without needing to know the options of this integration
-          if (!options.instrumentPageLoad) {
-            return;
-          }
-
           if (activeSpan) {
             DEBUG_BUILD && logger.log(`[Tracing] Finishing current transaction with op: ${spanToJSON(activeSpan).op}`);
             // If there's an open transaction on the scope, we need to finish it before creating an new one.
@@ -340,7 +325,7 @@ export const _browserTracingIntegration = ((_options: Partial<BrowserTracingOpti
         });
       }
 
-      if (options.instrumentPageLoad && client.emit && shouldUseDefaultPageLoadSpan) {
+      if (options.instrumentPageLoad && client.emit) {
         const context: StartSpanOptions = {
           name: WINDOW.location.pathname,
           // pageload should always start at timeOrigin (and needs to be in s, not ms)
@@ -370,17 +355,14 @@ export const _browserTracingIntegration = ((_options: Partial<BrowserTracingOpti
 
           if (from !== to) {
             startingUrl = undefined;
-            // We check this in here again, as a custom instrumentation may have been triggered in the meanwhile
-            if (shouldUseDefaultNavigationSpan) {
-              const context: StartSpanOptions = {
-                name: WINDOW.location.pathname,
-                op: 'navigation',
-                origin: 'auto.navigation.browser',
-                metadata: { source: 'url' },
-              };
-
-              startBrowserTracingNavigationSpan(context);
-            }
+            const context: StartSpanOptions = {
+              name: WINDOW.location.pathname,
+              op: 'navigation',
+              origin: 'auto.navigation.browser',
+              metadata: { source: 'url' },
+            };
+
+            startBrowserTracingNavigationSpan(context);
           }
         });
       }
@@ -420,7 +402,6 @@ export function startBrowserTracingPageLoadSpan(spanOptions: StartSpanOptions):
   }
 
   client.emit('startPageLoadSpan', spanOptions);
-  shouldUseDefaultPageLoadSpan = false;
 }
 
 /**
@@ -434,23 +415,6 @@ export function startBrowserTracingNavigationSpan(spanOptions: StartSpanOptions)
   }
 
   client.emit('startNavigationSpan', spanOptions);
-  shouldUseDefaultNavigationSpan = false;
-}
-
-/**
- * Use this method if you want to disable the default navigation span.
- * This is useful if you want to add custom routing instrumentation.
- */
-export function disableDefaultBrowserTracingNavigationSpan(disable = true): void {
-  shouldUseDefaultNavigationSpan = !disable;
-}
-
-/**
- * Use this method if you want to disable the default page load span.
- * This is useful if you want to add custom routing instrumentation.
- */
-export function disableDefaultBrowserTracingPageLoadSpan(disable = true): void {
-  shouldUseDefaultPageLoadSpan = !disable;
 }
 
 /** Returns the value of a meta tag */
diff --git a/packages/tracing-internal/src/browser/index.ts b/packages/tracing-internal/src/browser/index.ts
@@ -7,8 +7,6 @@ export {
   browserTracingIntegration,
   startBrowserTracingNavigationSpan,
   startBrowserTracingPageLoadSpan,
-  disableDefaultBrowserTracingNavigationSpan,
-  disableDefaultBrowserTracingPageLoadSpan,
 } from './browserTracingIntegration';
 
 export { instrumentOutgoingRequests, defaultRequestInstrumentationOptions } from './request';
diff --git a/packages/tracing-internal/src/index.ts b/packages/tracing-internal/src/index.ts
@@ -17,8 +17,6 @@ export {
   browserTracingIntegration,
   startBrowserTracingNavigationSpan,
   startBrowserTracingPageLoadSpan,
-  disableDefaultBrowserTracingNavigationSpan,
-  disableDefaultBrowserTracingPageLoadSpan,
   BROWSER_TRACING_INTEGRATION_ID,
   instrumentOutgoingRequests,
   defaultRequestInstrumentationOptions,
