From b08a3c8bddd6468a4baaa16ab741ccbc0acf88b9 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 11 Mar 2024 11:15:49 +0000 Subject: [PATCH] ref(browser): Update browser profiling to avoid deprecated APIs Esp. migrate away from `startTransaction` to `spanStart` hooks. --- packages/browser/src/profiling/integration.ts | 36 ++++---- ...rTransaction.ts => startProfileForSpan.ts} | 91 +++++++++---------- packages/browser/src/profiling/utils.ts | 10 +- 3 files changed, 62 insertions(+), 75 deletions(-) rename packages/browser/src/profiling/{startProfileForTransaction.ts => startProfileForSpan.ts} (54%) diff --git a/packages/browser/src/profiling/integration.ts b/packages/browser/src/profiling/integration.ts index 5b6137e10027..af88bd8d91e6 100644 --- a/packages/browser/src/profiling/integration.ts +++ b/packages/browser/src/profiling/integration.ts @@ -1,18 +1,17 @@ -import { defineIntegration, getCurrentScope } from '@sentry/core'; -import type { EventEnvelope, IntegrationFn, Transaction } from '@sentry/types'; +import { defineIntegration, getActiveSpan, getRootSpan } from '@sentry/core'; +import type { EventEnvelope, IntegrationFn, Span } from '@sentry/types'; import type { Profile } from '@sentry/types/src/profiling'; import { logger } from '@sentry/utils'; import { DEBUG_BUILD } from '../debug-build'; -import { startProfileForTransaction } from './startProfileForTransaction'; +import { startProfileForSpan } from './startProfileForSpan'; import type { ProfiledEvent } from './utils'; +import { isAutomatedPageLoadSpan, shouldProfileSpan } from './utils'; import { addProfilesToEnvelope, createProfilingEvent, findProfiledTransactionsFromEnvelope, getActiveProfilesCount, - isAutomatedPageLoadTransaction, - shouldProfileTransaction, takeProfileFromGlobalCache, } from './utils'; @@ -21,22 +20,19 @@ const INTEGRATION_NAME = 'BrowserProfiling'; const _browserProfilingIntegration = (() => { return { name: INTEGRATION_NAME, - // TODO v8: Remove this setup(client) { - const scope = getCurrentScope(); + const activeSpan = getActiveSpan(); + const rootSpan = activeSpan && getRootSpan(activeSpan); - // eslint-disable-next-line deprecation/deprecation - const transaction = scope.getTransaction(); - - if (transaction && isAutomatedPageLoadTransaction(transaction)) { - if (shouldProfileTransaction(transaction)) { - startProfileForTransaction(transaction); + if (rootSpan && isAutomatedPageLoadSpan(rootSpan)) { + if (shouldProfileSpan(rootSpan)) { + startProfileForSpan(rootSpan); } } - client.on('startTransaction', (transaction: Transaction) => { - if (shouldProfileTransaction(transaction)) { - startProfileForTransaction(transaction); + client.on('spanStart', (span: Span) => { + if (span === getRootSpan(span) && shouldProfileSpan(span)) { + startProfileForSpan(span); } }); @@ -59,23 +55,23 @@ const _browserProfilingIntegration = (() => { const start_timestamp = context && context['profile'] && context['profile']['start_timestamp']; if (typeof profile_id !== 'string') { - DEBUG_BUILD && logger.log('[Profiling] cannot find profile for a transaction without a profile context'); + DEBUG_BUILD && logger.log('[Profiling] cannot find profile for a span without a profile context'); continue; } if (!profile_id) { - DEBUG_BUILD && logger.log('[Profiling] cannot find profile for a transaction without a profile context'); + DEBUG_BUILD && logger.log('[Profiling] cannot find profile for a span without a profile context'); continue; } - // Remove the profile from the transaction context before sending, relay will take care of the rest. + // Remove the profile from the span context before sending, relay will take care of the rest. if (context && context['profile']) { delete context.profile; } const profile = takeProfileFromGlobalCache(profile_id); if (!profile) { - DEBUG_BUILD && logger.log(`[Profiling] Could not retrieve profile for transaction: ${profile_id}`); + DEBUG_BUILD && logger.log(`[Profiling] Could not retrieve profile for span: ${profile_id}`); continue; } diff --git a/packages/browser/src/profiling/startProfileForTransaction.ts b/packages/browser/src/profiling/startProfileForSpan.ts similarity index 54% rename from packages/browser/src/profiling/startProfileForTransaction.ts rename to packages/browser/src/profiling/startProfileForSpan.ts index 7d62cd8b1c46..9dcdb81a9e8e 100644 --- a/packages/browser/src/profiling/startProfileForTransaction.ts +++ b/packages/browser/src/profiling/startProfileForSpan.ts @@ -1,140 +1,132 @@ -import { spanToJSON } from '@sentry/core'; -import type { Transaction } from '@sentry/types'; +import { getCurrentScope, spanToJSON } from '@sentry/core'; +import type { Span } from '@sentry/types'; import { logger, timestampInSeconds, uuid4 } from '@sentry/utils'; import { DEBUG_BUILD } from '../debug-build'; import { WINDOW } from '../helpers'; import type { JSSelfProfile } from './jsSelfProfiling'; -import { - MAX_PROFILE_DURATION_MS, - addProfileToGlobalCache, - isAutomatedPageLoadTransaction, - startJSSelfProfile, -} from './utils'; +import { MAX_PROFILE_DURATION_MS, addProfileToGlobalCache, isAutomatedPageLoadSpan, startJSSelfProfile } from './utils'; /** * Wraps startTransaction and stopTransaction with profiling related logic. * startProfileForTransaction 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. */ -export function startProfileForTransaction(transaction: Transaction): Transaction { +export function startProfileForSpan(span: Span): void { // Start the profiler and get the profiler instance. let startTimestamp: number | undefined; - if (isAutomatedPageLoadTransaction(transaction)) { + if (isAutomatedPageLoadSpan(span)) { startTimestamp = timestampInSeconds() * 1000; } const profiler = startJSSelfProfile(); - // We failed to construct the profiler, fallback to original transaction. + // We failed to construct the profiler, so we skip. // No need to log anything as this has already been logged in startProfile. if (!profiler) { - return transaction; + return; } if (DEBUG_BUILD) { - logger.log(`[Profiling] started profiling transaction: ${spanToJSON(transaction).description}`); + logger.log(`[Profiling] started profiling span: ${spanToJSON(span).description}`); } - // We create "unique" transaction names to avoid concurrent transactions with same names - // from being ignored by the profiler. From here on, only this transaction name should be used when + // We create "unique" span names to avoid concurrent spans with same names + // from being ignored by the profiler. From here on, only this span name should be used when // calling the profiler methods. Note: we log the original name to the user to avoid confusion. const profileId = uuid4(); // A couple of important things to note here: // `CpuProfilerBindings.stopProfiling` will be scheduled to run in 30seconds in order to exceed max profile duration. - // Whichever of the two (transaction.finish/timeout) is first to run, the profiling will be stopped and the gathered profile - // will be processed when the original transaction is finished. Since onProfileHandler can be invoked multiple times in the - // event of an error or user mistake (calling transaction.finish multiple times), it is important that the behavior of onProfileHandler + // Whichever of the two (span.finish/timeout) is first to run, the profiling will be stopped and the gathered profile + // will be processed when the original span is finished. Since onProfileHandler can be invoked multiple times in the + // event of an error or user mistake (calling span.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. const processedProfile: JSSelfProfile | null = null; + getCurrentScope().setContext('profile', { + profile_id: profileId, + start_timestamp: startTimestamp, + }); + /** * Idempotent handler for profile stop */ - async function onProfileHandler(): Promise { - // 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 null; + async function onProfileHandler(): Promise { + // Check if the profile exists and return it the behavior has to be idempotent as users may call span.finish multiple times. + if (!span) { + return; } // Satisfy the type checker, but profiler will always be defined here. if (!profiler) { - return null; + return; } if (processedProfile) { if (DEBUG_BUILD) { - logger.log('[Profiling] profile for:', spanToJSON(transaction).description, 'already exists, returning early'); + logger.log('[Profiling] profile for:', spanToJSON(span).description, 'already exists, returning early'); } - return null; + return; } return profiler .stop() - .then((profile: JSSelfProfile): null => { + .then((profile: JSSelfProfile): void => { if (maxDurationTimeoutID) { WINDOW.clearTimeout(maxDurationTimeoutID); maxDurationTimeoutID = undefined; } if (DEBUG_BUILD) { - logger.log(`[Profiling] stopped profiling of transaction: ${spanToJSON(transaction).description}`); + logger.log(`[Profiling] stopped profiling of span: ${spanToJSON(span).description}`); } - // In case of an overlapping transaction, stopProfiling may return null and silently ignore the overlapping profile. + // In case of an overlapping span, stopProfiling may return null and silently ignore the overlapping profile. if (!profile) { if (DEBUG_BUILD) { logger.log( - `[Profiling] profiler returned null profile for: ${spanToJSON(transaction).description}`, - 'this may indicate an overlapping transaction or a call to stopProfiling with a profile title that was never started', + `[Profiling] profiler returned null profile for: ${spanToJSON(span).description}`, + 'this may indicate an overlapping span or a call to stopProfiling with a profile title that was never started', ); } - return null; + return; } addProfileToGlobalCache(profileId, profile); - return null; }) .catch(error => { if (DEBUG_BUILD) { logger.log('[Profiling] error while stopping profiler:', error); } - return null; }); } // Enqueue a timeout to prevent profiles from running over max duration. let maxDurationTimeoutID: number | undefined = WINDOW.setTimeout(() => { if (DEBUG_BUILD) { - logger.log( - '[Profiling] max profile duration elapsed, stopping profiling for:', - spanToJSON(transaction).description, - ); + logger.log('[Profiling] max profile duration elapsed, stopping profiling for:', spanToJSON(span).description); } - // If the timeout exceeds, we want to stop profiling, but not finish the transaction + // If the timeout exceeds, we want to stop profiling, but not finish the span // eslint-disable-next-line @typescript-eslint/no-floating-promises onProfileHandler(); }, MAX_PROFILE_DURATION_MS); // We need to reference the original end call to avoid creating an infinite loop - const originalEnd = transaction.end.bind(transaction); + const originalEnd = span.end.bind(span); /** - * Wraps startTransaction and stopTransaction with profiling related logic. - * 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. + * Wraps span `end()` with profiling related logic. + * startProfiling is called after the call to spanStart in order to avoid our own code from + * being profiled. Because of that same reason, stopProfiling is called before the call to spanEnd. */ - function profilingWrappedTransactionEnd(): Transaction { - if (!transaction) { + function profilingWrappedSpanEnd(): Span { + if (!span) { return originalEnd(); } // 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. void onProfileHandler().then( () => { - // TODO: Can we rewrite this to use attributes? - // eslint-disable-next-line deprecation/deprecation - transaction.setContext('profile', { profile_id: profileId, start_timestamp: startTimestamp }); originalEnd(); }, () => { @@ -143,9 +135,8 @@ export function startProfileForTransaction(transaction: Transaction): Transactio }, ); - return transaction; + return span; } - transaction.end = profilingWrappedTransactionEnd; - return transaction; + span.end = profilingWrappedSpanEnd; } diff --git a/packages/browser/src/profiling/utils.ts b/packages/browser/src/profiling/utils.ts index f4f700a2ab6f..46ae0c07442a 100644 --- a/packages/browser/src/profiling/utils.ts +++ b/packages/browser/src/profiling/utils.ts @@ -1,7 +1,7 @@ /* eslint-disable max-lines */ import { DEFAULT_ENVIRONMENT, getClient, spanToJSON } from '@sentry/core'; -import type { DebugImage, Envelope, Event, EventEnvelope, StackFrame, StackParser, Transaction } from '@sentry/types'; +import type { DebugImage, Envelope, Event, EventEnvelope, Span, StackFrame, StackParser } from '@sentry/types'; import type { Profile, ThreadCpuProfile } from '@sentry/types/src/profiling'; import { GLOBAL_OBJ, browserPerformanceTimeOrigin, forEachEnvelopeItem, logger, uuid4 } from '@sentry/utils'; @@ -201,8 +201,8 @@ export function isProfiledTransactionEvent(event: Event): event is ProfiledEvent /** * */ -export function isAutomatedPageLoadTransaction(transaction: Transaction): boolean { - return spanToJSON(transaction).op === 'pageload'; +export function isAutomatedPageLoadSpan(span: Span): boolean { + return spanToJSON(span).op === 'pageload'; } /** @@ -506,7 +506,7 @@ export function startJSSelfProfile(): JSSelfProfiler | undefined { /** * Determine if a profile should be profiled. */ -export function shouldProfileTransaction(transaction: Transaction): boolean { +export function shouldProfileSpan(span: Span): boolean { // If constructor failed once, it will always fail, so we can early return. if (PROFILING_CONSTRUCTOR_FAILED) { if (DEBUG_BUILD) { @@ -515,7 +515,7 @@ export function shouldProfileTransaction(transaction: Transaction): boolean { return false; } - if (!transaction.isRecording()) { + if (!span.isRecording()) { if (DEBUG_BUILD) { logger.log('[Profiling] Discarding profile because transaction was not sampled.'); }