review changes part 1

Author: s1gr1d

dev-packages/browser-integration-tests/suites/profiling/traceLifecycleMode_multiple-chunks/test.ts

@@ -164,7 +164,6 @@ sentryTest(
     expect(envelopeItemPayload2.profile).toBeDefined();
     expect(envelopeItemPayload2.version).toBe('2');
     expect(envelopeItemPayload2.platform).toBe('javascript');
-    expect(envelopeItemPayload2?.profile).toBeDefined();
 
     // Required profile metadata (Sample Format V2)
     // https://develop.sentry.dev/sdk/telemetry/profiles/sample-format-v2/

dev-packages/browser-integration-tests/suites/profiling/traceLifecycleMode_overlapping-spans/subject.js

@@ -48,6 +48,5 @@ await new Promise(r => setTimeout(r, 21));
 
 firstSpan.end();
 
-// Ensure envelope flush
 const client = Sentry.getClient();
 await client?.flush(5000);

packages/browser/src/profiling/integration.ts

@@ -28,12 +28,12 @@ const _browserProfilingIntegration = (() => {
     setup(client) {
       const options = client.getOptions() as BrowserOptions;
 
-      if (options && !hasLegacyProfiling(options) && !options.profileLifecycle) {
+      if (!hasLegacyProfiling(options) && !options.profileLifecycle) {
         // Set default lifecycle mode
         options.profileLifecycle = 'manual';
       }
 
-      if (!options || (hasLegacyProfiling(options) && !options.profilesSampleRate)) {
+      if (hasLegacyProfiling(options) && !options.profilesSampleRate) {
         DEBUG_BUILD && debug.log('[Profiling] Profiling disabled, no profiling options found.');
         return;
       }

packages/browser/src/profiling/lifecycleMode/traceLifecycleProfiler.ts

@@ -7,7 +7,6 @@ import {
   getGlobalScope,
   getRootSpan,
   getSdkMetadataForEnvelopeHeader,
-  spanToJSON,
   uuid4,
 } from '@sentry/core';
 import { DEBUG_BUILD } from '../../debug-build';
@@ -30,7 +29,6 @@ export class BrowserTraceLifecycleProfiler {
   private _client: Client | undefined;
   private _profiler: JSSelfProfiler | undefined;
   private _chunkTimer: ReturnType<typeof setTimeout> | undefined;
-  private _activeRootSpanCount: number;
   // For keeping track of active root spans
   private _activeRootSpanIds: Set<string>;
   private _profilerId: string | undefined;
@@ -41,7 +39,6 @@ export class BrowserTraceLifecycleProfiler {
     this._client = undefined;
     this._profiler = undefined;
     this._chunkTimer = undefined;
-    this._activeRootSpanCount = 0;
     this._activeRootSpanIds = new Set<string>();
     this._profilerId = undefined;
     this._isRunning = false;
@@ -58,37 +55,36 @@ export class BrowserTraceLifecycleProfiler {
     this._sessionSampled = sessionSampled;
 
     client.on('spanStart', span => {
-      if (span !== getRootSpan(span)) {
-        return;
-      }
       if (!this._sessionSampled) {
         DEBUG_BUILD && debug.log('[Profiling] Session not sampled because of negative sampling decision.');
         return;
       }
+      if (span !== getRootSpan(span)) {
+        return;
+      }
       // Only count sampled root spans
       if (!span.isRecording()) {
         DEBUG_BUILD && debug.log('[Profiling] Discarding profile because root span was not sampled.');
         return;
       }
 
-      const rootSpanJSON = spanToJSON(span);
-      const spanId = rootSpanJSON.span_id as string | undefined;
+      const spanId = span.spanContext().spanId;
       if (!spanId) {
         return;
       }
       if (this._activeRootSpanIds.has(spanId)) {
         return;
       }
+
       this._activeRootSpanIds.add(spanId);
+      const rootSpanCount = this._activeRootSpanIds.size;
+
+      if (rootSpanCount === 1) {
+        DEBUG_BUILD &&
+          debug.log(
+            `[Profiling] Root span with ID ${spanId} started. Will continue profiling for as long as there are active root spans (currently: ${rootSpanCount}).`,
+          );
 
-      const wasZero = this._activeRootSpanCount === 0;
-      this._activeRootSpanCount++; // Increment before eventually starting the profiler
-      DEBUG_BUILD &&
-        debug.log(
-          `[Profiling] Root span ${rootSpanJSON.description} started. Active root spans:`,
-          this._activeRootSpanCount,
-        );
-      if (wasZero) {
         this.start();
       }
     });
@@ -98,19 +94,19 @@ export class BrowserTraceLifecycleProfiler {
         return;
       }
 
-      const spanJSON = spanToJSON(span);
-      const spanId = spanJSON.span_id as string | undefined;
+      const spanId = span.spanContext().spanId;
       if (!spanId || !this._activeRootSpanIds.has(spanId)) {
         return;
       }
 
       this._activeRootSpanIds.delete(spanId);
-      this._activeRootSpanCount = Math.max(0, this._activeRootSpanCount - 1);
+      const rootSpanCount = this._activeRootSpanIds.size;
+
       DEBUG_BUILD &&
         debug.log(
-          `[Profiling] Root span ${spanJSON.description} ended. Active root spans: ${this._activeRootSpanCount}`,
+          `[Profiling] Root span with ID ${spanId} ended. Will continue profiling for as long as there are active root spans (currently: ${rootSpanCount}).`,
         );
-      if (this._activeRootSpanCount === 0) {
+      if (rootSpanCount === 0) {
         this._collectCurrentChunk().catch(() => /* no catch */ {});
 
         this.stop();
@@ -121,29 +117,24 @@ export class BrowserTraceLifecycleProfiler {
   /**
    * Handle an already-active root span at integration setup time.
    */
-  public notifyRootSpanActive(span: Span): void {
+  public notifyRootSpanActive(rootSpan: Span): void {
     if (!this._sessionSampled) {
       return;
     }
-    if (span !== getRootSpan(span)) {
-      return;
-    }
-    const spanId = spanToJSON(span)?.span_id;
+
+    const spanId = rootSpan.spanContext().spanId;
     if (!spanId || this._activeRootSpanIds.has(spanId)) {
       return;
     }
 
-    const wasZero = this._activeRootSpanCount === 0;
-
     this._activeRootSpanIds.add(spanId);
-    this._activeRootSpanCount++;
-    DEBUG_BUILD &&
-      debug.log(
-        '[Profiling] Detected already active root span during setup. Active root spans:',
-        this._activeRootSpanCount,
-      );
-
-    if (wasZero) {
+
+    const rootSpanCount = this._activeRootSpanIds.size;
+
+    if (rootSpanCount === 1) {
+      DEBUG_BUILD &&
+        debug.log('[Profiling] Detected already active root span during setup. Active root spans now:', rootSpanCount);
+
       this.start();
     }
   }

packages/browser/src/profiling/utils.ts

@@ -218,7 +218,8 @@ export function createProfileChunkPayload(
   client: Client,
   profilerId?: string,
 ): ProfileChunk {
-  if (jsSelfProfile === undefined || jsSelfProfile === null) {
+  // only == to catch null and undefined
+  if (jsSelfProfile == null) {
     throw new TypeError(
       `Cannot construct profiling event envelope without a valid profile. Got ${jsSelfProfile} instead.`,
     );
@@ -293,13 +294,13 @@ export function validateProfileChunk(chunk: ProfileChunk): { valid: boolean; rea
       return { valid: false, reason: 'missing profile data' };
     }
 
-    if (!Array.isArray(profile.frames) || profile.frames.length === 0) {
+    if (!Array.isArray(profile.frames) || !profile.frames.length) {
       return { valid: false, reason: 'profile has no frames' };
     }
-    if (!Array.isArray(profile.stacks) || profile.stacks.length === 0) {
+    if (!Array.isArray(profile.stacks) || !profile.stacks.length) {
       return { valid: false, reason: 'profile has no stacks' };
     }
-    if (!Array.isArray(profile.samples) || profile.samples.length === 0) {
+    if (!Array.isArray(profile.samples) || !profile.samples.length) {
       return { valid: false, reason: 'profile has no samples' };
     }
 
@@ -701,7 +702,7 @@ export function shouldProfileSpanLegacy(span: Span): boolean {
 /**
  * Determine if a profile should be created for the current session (lifecycle profiling mode).
  */
-export function shouldProfileSession(options?: BrowserOptions): boolean {
+export function shouldProfileSession(options: BrowserOptions): boolean {
   // If constructor failed once, it will always fail, so we can early return.
   if (PROFILING_CONSTRUCTOR_FAILED) {
     if (DEBUG_BUILD) {
@@ -710,16 +711,12 @@ export function shouldProfileSession(options?: BrowserOptions): boolean {
     return false;
   }
 
-  if (!options || options.profileLifecycle !== 'trace') {
+  if (options.profileLifecycle !== 'trace') {
     return false;
   }
 
   //  Session sampling: profileSessionSampleRate gates whether profiling is enabled for this session
-  const profileSessionSampleRate: number | boolean | undefined = (
-    options as unknown as {
-      profileSessionSampleRate?: number | boolean;
-    }
-  ).profileSessionSampleRate;
+  const profileSessionSampleRate = options.profileSessionSampleRate;
 
   if (!isValidSampleRate(profileSessionSampleRate)) {
     DEBUG_BUILD && debug.warn('[Profiling] Discarding profile because of invalid profileSessionSampleRate.');
@@ -732,15 +729,14 @@ export function shouldProfileSession(options?: BrowserOptions): boolean {
     return false;
   }
 
-  return profileSessionSampleRate === true ? true : Math.random() <= profileSessionSampleRate;
+  return Math.random() <= profileSessionSampleRate;
 }
 
 /**
  * Checks if legacy profiling is configured.
  */
-export function hasLegacyProfiling(options: BrowserOptions = {} as unknown as BrowserOptions): boolean {
-  // eslint-disable-next-line deprecation/deprecation
-  return typeof (options as unknown as { profilesSampleRate?: number | boolean }).profilesSampleRate !== 'undefined';
+export function hasLegacyProfiling(options: BrowserOptions): boolean {
+  return typeof options.profilesSampleRate !== 'undefined';
 }
 
 /**
@@ -820,14 +816,11 @@ export function attachProfiledThreadToEvent(event: Event): void {
   };
 
   // Attach thread info to individual spans so that spans can be associated with the profiled thread on the UI even if contexts are missing.
-  if (Array.isArray(event.spans)) {
-    const spans = event.spans;
-    for (const span of spans) {
-      span.data = {
-        ...(span.data || {}),
-        ['thread.id']: PROFILER_THREAD_ID_STRING,
-        ['thread.name']: PROFILER_THREAD_NAME,
-      };
-    }
-  }
+  event.spans?.forEach(span => {
+    span.data = {
+      ...(span.data || {}),
+      ['thread.id']: PROFILER_THREAD_ID_STRING,
+      ['thread.name']: PROFILER_THREAD_NAME,
+    };
+  });
 }

packages/core/src/types-hoist/browseroptions.ts

@@ -28,8 +28,8 @@ export type BrowserClientProfilingOptions = {
   /**
    * Sets profiling session sample rate for the entire profiling session.
    *
-   * A profiling session corresponds to a user session, so this rate determines what percentage of user sessions will have profiling enabled.
-   *
+   * A profiling session corresponds to a user session, meaning it is set once at integration initialization and
+   * persisted until the next page reload. This rate determines what percentage of user sessions will have profiling enabled.
    * @default 0
    */
   profileSessionSampleRate?: number;