Uh oh!
There was an error while loading. Please reload this page.
ref(browser): Move trace lifecycle listeners to class function - #18231
Conversation
| this._client = client; | ||
| this._sessionSampled = sessionSampled; | ||
| client.on('spanStart', span => { | ||
| 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; | ||
| } | ||
| // Matching root spans with profiles | ||
| getGlobalScope().setContext('profile', { | ||
| profiler_id: this._profilerId, | ||
| }); | ||
| const spanId = span.spanContext().spanId; | ||
| if (!spanId) { | ||
| return; | ||
| } | ||
| if (this._activeRootSpanIds.has(spanId)) { | ||
| return; | ||
| } | ||
| this._activeRootSpanIds.add(spanId); | ||
| const rootSpanCount = this._activeRootSpanIds.size; | ||
| const timeout = setTimeout(() => { | ||
| this._onRootSpanTimeout(spanId); | ||
| }, MAX_ROOT_SPAN_PROFILE_MS); | ||
| this._rootSpanTimeouts.set(spanId, timeout); | ||
| 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}).`, | ||
| ); | ||
| this.start(); | ||
| } | ||
| }); | ||
| client.on('spanEnd', span => { | ||
| if (!this._sessionSampled) { | ||
| return; | ||
| } | ||
| const spanId = span.spanContext().spanId; | ||
| if (!spanId || !this._activeRootSpanIds.has(spanId)) { | ||
| return; | ||
| } | ||
| this._activeRootSpanIds.delete(spanId); | ||
| const rootSpanCount = this._activeRootSpanIds.size; | ||
| DEBUG_BUILD && | ||
| debug.log( | ||
| `[Profiling] Root span with ID ${spanId} ended. Will continue profiling for as long as there are active root spans (currently: ${rootSpanCount}).`, | ||
| ); | ||
| if (rootSpanCount === 0) { | ||
| this._collectCurrentChunk().catch(e => { | ||
| DEBUG_BUILD && debug.error('[Profiling] Failed to collect current profile chunk on `spanEnd`:', e); | ||
| }); | ||
| this.stop(); | ||
| } | ||
| }); | ||
| this._setupTraceLifecycleListeners(client); | ||
| } | ||
| /** |
There was a problem hiding this comment.
Bug:notifyRootSpanActive() fails to register a safeguard timeout for already-active root spans, leading to indefinite profiling if spanEnd is not called.
Severity: CRITICAL | Confidence: 0.95
🔍 Detailed Analysis
The notifyRootSpanActive() method, used for already-active root spans during profiler initialization, adds the span ID to _activeRootSpanIds but fails to call _registerTraceRootSpan(). This omission means no safeguard timeout is registered for these spans. If such a pre-existing root span never fires its spanEnd event, the profiler will continue running indefinitely, wasting resources by continuously collecting and sending profile chunks every 60 seconds, contradicting the intended design of MAX_ROOT_SPAN_PROFILE_MS.
💡 Suggested Fix
Modify notifyRootSpanActive() to call _registerTraceRootSpan(spanId) instead of directly adding spanId to _activeRootSpanIds. This ensures that all active root spans, regardless of how they are initialized, have the necessary safeguard timeout registered.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: packages/browser/src/profiling/UIProfiler.ts#L62-L68
Potential issue: The `notifyRootSpanActive()` method, used for already-active root spans
during profiler initialization, adds the span ID to `_activeRootSpanIds` but fails to
call `_registerTraceRootSpan()`. This omission means no safeguard timeout is registered
for these spans. If such a pre-existing root span never fires its `spanEnd` event, the
profiler will continue running indefinitely, wasting resources by continuously
collecting and sending profile chunks every 60 seconds, contradicting the intended
design of `MAX_ROOT_SPAN_PROFILE_MS`.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference_id: 2726774
There was a problem hiding this comment.
This is done in another function - no problem here
It's kept track of it in _rootSpanTimeouts
| DEBUG_BUILD && debug.log('[Profiling] Started profiling with profile ID:', this._profilerId); | ||
| // Expose profiler_id to match root spans with profiles | ||
| getGlobalScope().setContext('profile', { profiler_id: this._profilerId }); |
There was a problem hiding this comment.
Bug: Profiler Context: Spans Adrift
Moving the global scope context setting from the spanStart listener to start() breaks profile-span association when the profiler fails to initialize. If the first root span fails to start the profiler, _resetProfilerInfo() clears the context and sets _isRunning to false. Subsequent root spans won't call start() (since rootSpanCount !== 1), leaving them without the profiler_id context needed to match spans with profiles.
There was a problem hiding this comment.
Works as intended, this is still attached at the correct time.
size-limit report 📦
|
Uh oh!
There was an error while loading. Please reload this page.
This PR was factored out of another PR to make reviewing easier. The other PR: #18189
Moved the
spanStartandspanEndlisteners into an extra function (_setupTraceLifecycleListeners) to be able to only call it depending on the lifecycle (used in another PR).Part of #17279