ref(tracing): Clean up various things before fixing sampling (#2944)

Author: lobsterkatie

packages/tracing/src/browser/browsertracing.ts

@@ -13,7 +13,7 @@ import {
   registerRequestInstrumentation,
   RequestInstrumentationOptions,
 } from './request';
-import { defaultBeforeNavigate, defaultRoutingInstrumentation } from './router';
+import { defaultRoutingInstrumentation } from './router';
 
 export const DEFAULT_MAX_TRANSACTION_DURATION_SECONDS = 600;
 
@@ -66,7 +66,7 @@ export interface BrowserTracingOptions extends RequestInstrumentationOptions {
    *
    * If undefined is returned, a pageload/navigation transaction will not be created.
    */
-  beforeNavigate(context: TransactionContext): TransactionContext | undefined;
+  beforeNavigate?(context: TransactionContext): TransactionContext | undefined;
 
   /**
    * Instrumentation that creates routing change transactions. By default creates
@@ -80,7 +80,6 @@ export interface BrowserTracingOptions extends RequestInstrumentationOptions {
 }
 
 const DEFAULT_BROWSER_TRACING_OPTIONS = {
-  beforeNavigate: defaultBeforeNavigate,
   idleTimeout: DEFAULT_IDLE_TIMEOUT,
   markBackgroundTransactions: true,
   maxTransactionDuration: DEFAULT_MAX_TRANSACTION_DURATION_SECONDS,
@@ -189,20 +188,21 @@ export class BrowserTracing implements Integration {
     const { beforeNavigate, idleTimeout, maxTransactionDuration } = this.options;
 
     // if beforeNavigate returns undefined, we should not start a transaction.
-    const ctx = beforeNavigate({
+    const expandedContext = {
       ...context,
       ...getHeaderContext(),
       trimEnd: true,
-    });
+    };
+    const modifiedContext = typeof beforeNavigate === 'function' ? beforeNavigate(expandedContext) : expandedContext;
 
-    if (ctx === undefined) {
+    if (modifiedContext === undefined) {
       logger.log(`[Tracing] Did not create ${context.op} idleTransaction due to beforeNavigate`);
       return undefined;
     }
 
     const hub = this._getCurrentHub();
-    logger.log(`[Tracing] starting ${ctx.op} idleTransaction on scope`);
-    const idleTransaction = startIdleTransaction(hub, ctx, idleTimeout, true);
+    const idleTransaction = startIdleTransaction(hub, modifiedContext, idleTimeout, true);
+    logger.log(`[Tracing] Starting ${modifiedContext.op} transaction on scope`);
     idleTransaction.registerBeforeFinishCallback((transaction, endTimestamp) => {
       this._metrics.addPerformanceEntries(transaction);
       adjustTransactionDuration(secToMs(maxTransactionDuration), transaction, endTimestamp);
@@ -217,7 +217,7 @@ export class BrowserTracing implements Integration {
  *
  * @returns Transaction context data from the header or undefined if there's no header or the header is malformed
  */
-function getHeaderContext(): Partial<TransactionContext> | undefined {
+export function getHeaderContext(): Partial<TransactionContext> | undefined {
   const header = getMetaContent('sentry-trace');
   if (header) {
     return extractTraceparentData(header);

packages/tracing/src/browser/request.ts

@@ -41,20 +41,24 @@ export interface RequestInstrumentationOptions {
 /** Data returned from fetch callback */
 export interface FetchData {
   // eslint-disable-next-line @typescript-eslint/no-explicit-any
-  args: any[];
+  args: any[]; // the arguments passed to the fetch call itself
   fetchData?: {
     method: string;
     url: string;
     // span_id
     __span?: string;
   };
-  response?: Response;
+
+  // TODO Should this be unknown instead? If we vendor types, make it a Response
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  response?: any;
+
   startTimestamp: number;
   endTimestamp?: number;
 }
 
 /** Data returned from XHR request */
-interface XHRData {
+export interface XHRData {
   xhr?: {
     __sentry_xhr__?: {
       method: string;
@@ -64,8 +68,8 @@ interface XHRData {
       data: Record<string, any>;
     };
     __sentry_xhr_span_id__?: string;
-    __sentry_own_request__: boolean;
     setRequestHeader?: (key: string, val: string) => void;
+    __sentry_own_request__?: boolean;
   };
   startTimestamp: number;
   endTimestamp?: number;
@@ -114,7 +118,7 @@ export function registerRequestInstrumentation(_options?: Partial<RequestInstrum
   if (traceFetch) {
     addInstrumentationHandler({
       callback: (handlerData: FetchData) => {
-        _fetchCallback(handlerData, shouldCreateSpan, spans);
+        fetchCallback(handlerData, shouldCreateSpan, spans);
       },
       type: 'fetch',
     });
@@ -133,7 +137,7 @@ export function registerRequestInstrumentation(_options?: Partial<RequestInstrum
 /**
  * Create and track fetch request spans
  */
-export function _fetchCallback(
+export function fetchCallback(
   handlerData: FetchData,
   shouldCreateSpan: (url: string) => boolean,
   spans: Record<string, Span>,
@@ -147,6 +151,8 @@ export function _fetchCallback(
     if (span) {
       const response = handlerData.response;
       if (response) {
+        // TODO (kmclb) remove this once types PR goes through
+        // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
         span.setHttpStatus(response.status);
       }
       span.finish();
@@ -198,7 +204,7 @@ export function _fetchCallback(
 /**
  * Create and track xhr request spans
  */
-function xhrCallback(
+export function xhrCallback(
   handlerData: XHRData,
   shouldCreateSpan: (url: string) => boolean,
   spans: Record<string, Span>,
@@ -217,11 +223,10 @@ function xhrCallback(
     return;
   }
 
+  // check first if the request has finished and is tracked by an existing span which should now end
   if (handlerData.endTimestamp && handlerData.xhr.__sentry_xhr_span_id__) {
     const span = spans[handlerData.xhr.__sentry_xhr_span_id__];
     if (span) {
-      span.setData('url', xhr.url);
-      span.setData('method', xhr.method);
       span.setHttpStatus(xhr.status_code);
       span.finish();
 
@@ -231,12 +236,15 @@ function xhrCallback(
     return;
   }
 
+  // if not, create a new span to track it
   const activeTransaction = getActiveTransaction();
   if (activeTransaction) {
     const span = activeTransaction.startChild({
       data: {
         ...xhr.data,
         type: 'xhr',
+        method: xhr.method,
+        url: xhr.url,
       },
       description: `${xhr.method} ${xhr.url}`,
       op: 'http',

packages/tracing/src/browser/router.ts

@@ -54,8 +54,3 @@ export function defaultRoutingInstrumentation<T extends TransactionType>(
     });
   }
 }
-
-/** default implementation of Browser Tracing before navigate */
-export function defaultBeforeNavigate(context: TransactionContext): TransactionContext | undefined {
-  return context;
-}

packages/tracing/src/hubextensions.ts

@@ -184,8 +184,9 @@ function isValidSampleRate(rate: unknown): boolean {
 /**
  * Creates a new transaction and adds a sampling decision if it doesn't yet have one.
  *
- * The Hub.startTransaction method delegates to this method to do its work, passing the Hub instance in as `this`.
- * Exists as a separate function so that it can be injected into the class as an "extension method."
+ * The Hub.startTransaction method delegates to this method to do its work, passing the Hub instance in as `this`, as if
+ * it had been called on the hub directly. Exists as a separate function so that it can be injected into the class as an
+ * "extension method."
  *
  * @param this: The Hub starting the transaction
  * @param transactionContext: Data used to configure the transaction

packages/tracing/test/browser/browsertracing.test.ts

@@ -11,6 +11,7 @@ import {
 } from '../../src/browser/browsertracing';
 import { defaultRequestInstrumentionOptions } from '../../src/browser/request';
 import { defaultRoutingInstrumentation } from '../../src/browser/router';
+import * as hubExtensions from '../../src/hubextensions';
 import { DEFAULT_IDLE_TIMEOUT, IdleTransaction } from '../../src/idletransaction';
 import { getActiveTransaction, secToMs } from '../../src/utils';
 
@@ -76,7 +77,6 @@ describe('BrowserTracing', () => {
     const browserTracing = createBrowserTracing();
 
     expect(browserTracing.options).toEqual({
-      beforeNavigate: expect.any(Function),
       idleTimeout: DEFAULT_IDLE_TIMEOUT,
       markBackgroundTransactions: true,
       maxTransactionDuration: DEFAULT_MAX_TRANSACTION_DURATION_SECONDS,
@@ -210,12 +210,20 @@ describe('BrowserTracing', () => {
       const name = 'sentry-trace';
       const content = '126de09502ae4e0fb26c6967190756a4-b6e54397b12a2a0f-1';
       document.head.innerHTML = `<meta name="${name}" content="${content}">`;
+      const startIdleTransaction = jest.spyOn(hubExtensions, 'startIdleTransaction');
+
       createBrowserTracing(true, { routingInstrumentation: customRoutingInstrumentation });
-      const transaction = getActiveTransaction(hub) as IdleTransaction;
 
-      expect(transaction.traceId).toBe('126de09502ae4e0fb26c6967190756a4');
-      expect(transaction.parentSpanId).toBe('b6e54397b12a2a0f');
-      expect(transaction.sampled).toBe(true);
+      expect(startIdleTransaction).toHaveBeenCalledWith(
+        expect.any(Object),
+        expect.objectContaining({
+          traceId: '126de09502ae4e0fb26c6967190756a4',
+          parentSpanId: 'b6e54397b12a2a0f',
+          parentSampled: true,
+        }),
+        expect.any(Number),
+        expect.any(Boolean),
+      );
     });
 
     describe('idleTimeout', () => {
@@ -341,22 +349,35 @@ describe('BrowserTracing', () => {
       });
     });
   });
-});
 
-describe('getMeta', () => {
-  it('returns a found meta tag contents', () => {
-    const name = 'sentry-trace';
-    const content = '126de09502ae4e0fb26c6967190756a4-b6e54397b12a2a0f-1';
-    document.head.innerHTML = `<meta name="${name}" content="${content}">`;
+  describe('sentry-trace <meta> element', () => {
+    describe('getMetaContent', () => {
+      it('finds the specified tag and extracts the value', () => {
+        const name = 'sentry-trace';
+        const content = '126de09502ae4e0fb26c6967190756a4-b6e54397b12a2a0f-1';
+        document.head.innerHTML = `<meta name="${name}" content="${content}">`;
 
-    const meta = getMetaContent(name);
-    expect(meta).toBe(content);
-  });
+        const metaTagValue = getMetaContent(name);
+        expect(metaTagValue).toBe(content);
+      });
 
-  it('only returns meta tags queried for', () => {
-    document.head.innerHTML = `<meta name="not-test">`;
+      it("doesn't return meta tags other than the one specified", () => {
+        document.head.innerHTML = `<meta name="cat-cafe">`;
 
-    const meta = getMetaContent('test');
-    expect(meta).toBe(null);
+        const metaTagValue = getMetaContent('dogpark');
+        expect(metaTagValue).toBe(null);
+      });
+
+      it('can pick the correct tag out of multiple options', () => {
+        const name = 'sentry-trace';
+        const content = '126de09502ae4e0fb26c6967190756a4-b6e54397b12a2a0f-1';
+        const sentryTraceMeta = `<meta name="${name}" content="${content}">`;
+        const otherMeta = `<meta name="cat-cafe">`;
+        document.head.innerHTML = `${sentryTraceMeta} ${otherMeta}`;
+
+        const metaTagValue = getMetaContent(name);
+        expect(metaTagValue).toBe(content);
+      });
+    });
   });
 });