feat(browser): send profiles in same envelope as transactions

packages/browser/src/profiling/hubextensions.ts

@@ -1,5 +1,6 @@
-import { getCurrentHub, getMainCarrier } from '@sentry/core';
-import type { CustomSamplingContext, Hub, Transaction, TransactionContext } from '@sentry/types';
+/* eslint-disable complexity */
+import { getCurrentHub } from '@sentry/core';
+import type { Transaction } from '@sentry/types';
 import { logger, uuid4 } from '@sentry/utils';
 
 import { WINDOW } from '../helpers';
@@ -9,21 +10,13 @@ import type {
   JSSelfProfilerConstructor,
   ProcessedJSSelfProfile,
 } from './jsSelfProfiling';
-import { sendProfile } from './sendProfile';
+import { addProfileToMap,isValidSampleRate } from './utils';
 
-// Max profile duration.
-const MAX_PROFILE_DURATION_MS = 30_000;
+export const MAX_PROFILE_DURATION_MS = 30_000;
 // Keep a flag value to avoid re-initializing the profiler constructor. If it fails
 // once, it will always fail and this allows us to early return.
 let PROFILING_CONSTRUCTOR_FAILED = false;
 
-// While we experiment, per transaction sampling interval will be more flexible to work with.
-type StartTransaction = (
-  this: Hub,
-  transactionContext: TransactionContext,
-  customSamplingContext?: CustomSamplingContext,
-) => Transaction | undefined;
-
 /**
  * Check if profiler constructor is available.
  * @param maybeProfiler
@@ -55,7 +48,7 @@ export function onProfilingStartRouteTransaction(transaction: Transaction | unde
  * startProfiling is called after the call to startTransaction in order to avoid our own code from
  * being profiled. Because of that same reason, stopProfiling is called before the call to stopTransaction.
  */
-function wrapTransactionWithProfiling(transaction: Transaction): Transaction {
+export function wrapTransactionWithProfiling(transaction: Transaction): Transaction {
   // Feature support check first
   const JSProfilerConstructor = WINDOW.Profiler;
 
@@ -68,14 +61,6 @@ function wrapTransactionWithProfiling(transaction: Transaction): Transaction {
     return transaction;
   }
 
-  // profilesSampleRate is multiplied with tracesSampleRate to get the final sampling rate.
-  if (!transaction.sampled) {
-    if (__DEBUG_BUILD__) {
-      logger.log('[Profiling] Transaction is not sampled, skipping profiling');
-    }
-    return transaction;
-  }
-
   // If constructor failed once, it will always fail, so we can early return.
   if (PROFILING_CONSTRUCTOR_FAILED) {
     if (__DEBUG_BUILD__) {
@@ -86,21 +71,52 @@ function wrapTransactionWithProfiling(transaction: Transaction): Transaction {
 
   const client = getCurrentHub().getClient();
   const options = client && client.getOptions();
+  if (!options) {
+    __DEBUG_BUILD__ && logger.log('[Profiling] Profiling disabled, no options found.');
+    return transaction;
+  }
 
-  // @ts-ignore not part of the browser options yet
-  const profilesSampleRate = (options && options.profilesSampleRate) || 0;
-  if (profilesSampleRate === undefined) {
-    if (__DEBUG_BUILD__) {
-      logger.log('[Profiling] Profiling disabled, enable it by setting `profilesSampleRate` option to SDK init call.');
-    }
+  // @ts-ignore profilesSampler is not part of the browser options yet
+  const profilesSampler = options.profilesSampler;
+  // @ts-ignore profilesSampleRate is not part of the browser options yet
+  let profilesSampleRate: number | boolean | undefined = options.profilesSampleRate;
+
+  // Prefer sampler to sample rate if both are provided.
+  if (typeof profilesSampler === 'function') {
+    profilesSampleRate = profilesSampler({ transactionContext: transaction.toContext() });
+  }
+
+  // Since this is coming from the user (or from a function provided by the user), who knows what we might get. (The
+  // only valid values are booleans or numbers between 0 and 1.)
+  if (!isValidSampleRate(profilesSampleRate)) {
+    __DEBUG_BUILD__ && logger.warn('[Profiling] Discarding profile because of invalid sample rate.');
+    return transaction;
+  }
+
+  // if the function returned 0 (or false), or if `profileSampleRate` is 0, it's a sign the profile should be dropped
+  if (!profilesSampleRate) {
+    __DEBUG_BUILD__ &&
+      logger.log(
+        `[Profiling] Discarding profile because ${
+          typeof profilesSampler === 'function'
+            ? 'profileSampler returned 0 or false'
+            : 'a negative sampling decision was inherited or profileSampleRate is set to 0'
+        }`,
+      );
     return transaction;
   }
 
+  // Now we roll the dice. Math.random is inclusive of 0, but not of 1, so strict < is safe here. In case sampleRate is
+  // a boolean, the < comparison will cause it to be automatically cast to 1 if it's true and 0 if it's false.
+  const sampled = profilesSampleRate === true ? true : Math.random() < profilesSampleRate;
   // Check if we should sample this profile
-  if (Math.random() > profilesSampleRate) {
-    if (__DEBUG_BUILD__) {
-      logger.log('[Profiling] Skip profiling transaction due to sampling.');
-    }
+  if (!sampled) {
+    __DEBUG_BUILD__ &&
+      logger.log(
+        `[Profiling] Discarding profile because it's not included in the random sample (sampling rate = ${Number(
+          profilesSampleRate,
+        )})`,
+      );
     return transaction;
   }
 
@@ -147,19 +163,19 @@ function wrapTransactionWithProfiling(transaction: Transaction): Transaction {
   // event of an error or user mistake (calling transaction.finish multiple times), it is important that the behavior of onProfileHandler
   // is idempotent as we do not want any timings or profiles to be overriden by the last call to onProfileHandler.
   // After the original finish method is called, the event will be reported through the integration and delegated to transport.
-  let processedProfile: ProcessedJSSelfProfile | null = null;
+  const processedProfile: ProcessedJSSelfProfile | null = null;
 
   /**
    * Idempotent handler for profile stop
    */
-  function onProfileHandler(): void {
+  async function onProfileHandler(): Promise<null> {
     // Check if the profile exists and return it the behavior has to be idempotent as users may call transaction.finish multiple times.
     if (!transaction) {
-      return;
+      return null;
     }
     // Satisfy the type checker, but profiler will always be defined here.
     if (!profiler) {
-      return;
+      return null;
     }
     if (processedProfile) {
       if (__DEBUG_BUILD__) {
@@ -169,12 +185,12 @@ function wrapTransactionWithProfiling(transaction: Transaction): Transaction {
           'already exists, returning early',
         );
       }
-      return;
+      return null;
     }
 
-    profiler
+    return profiler
       .stop()
-      .then((p: JSSelfProfile): void => {
+      .then((p: JSSelfProfile): null => {
         if (maxDurationTimeoutID) {
           WINDOW.clearTimeout(maxDurationTimeoutID);
           maxDurationTimeoutID = undefined;
@@ -192,16 +208,23 @@ function wrapTransactionWithProfiling(transaction: Transaction): Transaction {
               'this may indicate an overlapping transaction or a call to stopProfiling with a profile title that was never started',
             );
           }
-          return;
+          return null;
         }
 
         // If a profile has less than 2 samples, it is not useful and should be discarded.
         if (p.samples.length < 2) {
-          return;
+          return null;
+        }
+
+        // Discard a profile if it has no frames - happens if code was idling or sampling
+        // did not collect any frames.
+        if(!p.frames.length){
+          return null;
         }
 
-        processedProfile = { ...p, profile_id: profileId };
-        sendProfile(profileId, processedProfile);
+
+        addProfileToMap({...p, profile_id: profileId });
+        return null;
       })
       .catch(error => {
         if (__DEBUG_BUILD__) {
@@ -219,6 +242,7 @@ function wrapTransactionWithProfiling(transaction: Transaction): Transaction {
         transaction.name || transaction.description,
       );
     }
+    // If the timeout exceeds, we want to stop profiling, but not finish the transaction
     void onProfileHandler();
   }, MAX_PROFILE_DURATION_MS);
 
@@ -230,73 +254,25 @@ function wrapTransactionWithProfiling(transaction: Transaction): Transaction {
    * startProfiling is called after the call to startTransaction in order to avoid our own code from
    * being profiled. Because of that same reason, stopProfiling is called before the call to stopTransaction.
    */
-  function profilingWrappedTransactionFinish(): Promise<Transaction> {
+  function profilingWrappedTransactionFinish(): Transaction {
     if (!transaction) {
       return originalFinish();
     }
     // onProfileHandler should always return the same profile even if this is called multiple times.
     // Always call onProfileHandler to ensure stopProfiling is called and the timeout is cleared.
-    onProfileHandler();
-
-    // Set profile context
-    transaction.setContext('profile', { profile_id: profileId });
+    void onProfileHandler()
+      .then(() => {
+        transaction.setContext('profile', { profile_id: profileId });
+        originalFinish();
+      })
+      .catch(() => {
+        // If onProfileHandler fails, we still want to call the original finish method.
+        originalFinish();
+      });
 
-    return originalFinish();
+    return transaction;
   }
 
   transaction.finish = profilingWrappedTransactionFinish;
   return transaction;
 }
-
-/**
- * Wraps startTransaction with profiling logic. This is done automatically by the profiling integration.
- */
-function __PRIVATE__wrapStartTransactionWithProfiling(startTransaction: StartTransaction): StartTransaction {
-  return function wrappedStartTransaction(
-    this: Hub,
-    transactionContext: TransactionContext,
-    customSamplingContext?: CustomSamplingContext,
-  ): Transaction | undefined {
-    const transaction: Transaction | undefined = startTransaction.call(this, transactionContext, customSamplingContext);
-    if (transaction === undefined) {
-      if (__DEBUG_BUILD__) {
-        logger.log('[Profiling] Transaction is undefined, skipping profiling');
-      }
-      return transaction;
-    }
-
-    return wrapTransactionWithProfiling(transaction);
-  };
-}
-
-/**
- * Patches startTransaction and stopTransaction with profiling logic.
- */
-export function addProfilingExtensionMethods(): void {
-  const carrier = getMainCarrier();
-  if (!carrier.__SENTRY__) {
-    if (__DEBUG_BUILD__) {
-      logger.log("[Profiling] Can't find main carrier, profiling won't work.");
-    }
-    return;
-  }
-  carrier.__SENTRY__.extensions = carrier.__SENTRY__.extensions || {};
-
-  if (!carrier.__SENTRY__.extensions['startTransaction']) {
-    if (__DEBUG_BUILD__) {
-      logger.log(
-        '[Profiling] startTransaction does not exists, profiling will not work. Make sure you import @sentry/tracing package before @sentry/profiling-node as import order matters.',
-      );
-    }
-    return;
-  }
-
-  if (__DEBUG_BUILD__) {
-    logger.log('[Profiling] startTransaction exists, patching it with profiling functionality...');
-  }
-
-  carrier.__SENTRY__.extensions['startTransaction'] = __PRIVATE__wrapStartTransactionWithProfiling(
-    // This is already patched by sentry/tracing, we are going to re-patch it...
-    carrier.__SENTRY__.extensions['startTransaction'] as StartTransaction,
-  );
-}

packages/browser/src/profiling/integration.ts

@@ -1,8 +1,16 @@
-import type { Event, EventProcessor, Integration } from '@sentry/types';
+import type { EventProcessor, Hub, Integration, Transaction } from '@sentry/types';
+import type { Profile } from '@sentry/types/src/profiling';
 import { logger } from '@sentry/utils';
 
-import { PROFILING_EVENT_CACHE } from './cache';
-import { addProfilingExtensionMethods } from './hubextensions';
+import type { BrowserClient } from './../client';
+import { wrapTransactionWithProfiling } from './hubextensions';
+import type { ProfiledEvent } from './utils';
+import {
+  addProfilesToEnvelope,
+  createProfilingEvent,
+  findProfiledTransactionsFromEnvelope,
+  PROFILE_MAP,
+} from './utils';
 
 /**
  * Browser profiling integration. Stores any event that has contexts["profile"]["profile_id"]
@@ -15,34 +23,67 @@ import { addProfilingExtensionMethods } from './hubextensions';
  */
 export class BrowserProfilingIntegration implements Integration {
   public readonly name: string = 'BrowserProfilingIntegration';
+  public getCurrentHub?: () => Hub = undefined;
 
   /**
    * @inheritDoc
    */
-  public setupOnce(addGlobalEventProcessor: (callback: EventProcessor) => void): void {
-    // Patching the hub to add the extension methods.
-    // Warning: we have an implicit dependency on import order and we will fail patching if the constructor of
-    // BrowserProfilingIntegration is called before @sentry/tracing is imported. This is because we need to patch
-    // the methods of @sentry/tracing which are patched as a side effect of importing @sentry/tracing.
-    addProfilingExtensionMethods();
-
-    // Add our event processor
-    addGlobalEventProcessor(this.handleGlobalEvent.bind(this));
-  }
+  public setupOnce(addGlobalEventProcessor: (callback: EventProcessor) => void, getCurrentHub: () => Hub): void {
+    this.getCurrentHub = getCurrentHub;
+    const client = this.getCurrentHub().getClient() as BrowserClient;
 
-  /**
-   * @inheritDoc
-   */
-  public handleGlobalEvent(event: Event): Event {
-    const profileId = event.contexts && event.contexts['profile'] && event.contexts['profile']['profile_id'];
-
-    if (profileId && typeof profileId === 'string') {
-      if (__DEBUG_BUILD__) {
-        logger.log('[Profiling] Profiling event found, caching it.');
-      }
-      PROFILING_EVENT_CACHE.add(profileId, event);
-    }
+    if (client && typeof client.on === 'function') {
+      client.on('startTransaction', (transaction: Transaction) => {
+        wrapTransactionWithProfiling(transaction);
+      });
+
+      client.on('beforeEnvelope', (envelope): void => {
+        // if not profiles are in queue, there is nothing to add to the envelope.
+        if (!PROFILE_MAP.size) {
+          return;
+        }
+
+        const profiledTransactionEvents = findProfiledTransactionsFromEnvelope(envelope);
+        if (!profiledTransactionEvents.length) {
+          return;
+        }
 
-    return event;
+        const profilesToAddToEnvelope: Profile[] = [];
+
+        for (const profiledTransaction of profiledTransactionEvents) {
+          const profile_id =
+            profiledTransaction &&
+            profiledTransaction.contexts &&
+            profiledTransaction.contexts['profile'] &&
+            profiledTransaction.contexts['profile']['profile_id'] as string
+
+          if (!profile_id) {
+            throw new TypeError('[Profiling] cannot find profile for a transaction without a profile context');
+          }
+
+          // Remove the profile from the transaction context before sending, relay will take care of the rest.
+          if (profiledTransaction && profiledTransaction.contexts && profiledTransaction.contexts['.profile']) {
+            delete profiledTransaction.contexts.profile;
+          }
+
+          const profile = PROFILE_MAP.get(profile_id);
+          if (!profile) {
+            __DEBUG_BUILD__ && logger.log(`[Profiling] Could not retrieve profile for transaction: ${profile_id}`);
+            continue;
+          }
+
+          PROFILE_MAP.delete(profile_id);
+          const profileEvent = createProfilingEvent(profile, profiledTransaction as ProfiledEvent);
+
+          if (profileEvent) {
+            profilesToAddToEnvelope.push(profileEvent);
+          }
+        }
+
+        addProfilesToEnvelope(envelope, profilesToAddToEnvelope);
+      });
+    } else {
+      logger.warn('[Profiling] Client does not support hooks, profiling will be disabled');
+    }
   }
 }