feat(browserProfiling): Add trace lifecycle mode for UI profiling
#17619
Conversation
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
This reverts commit ec0fd27.
There was a problem hiding this comment.
Nice! I had some comments and remarks but most of them really are just nits about defensiveness in type checking. Feel free to apply them as you see fit. My general thinking here is that we don't have to be too defensive on types we defined in the SDK (like options, span ids, etc), as discussed in the call today.
| } | ||
|
|
||
| // Adding client hook to attach profiles to transaction events before they are sent. | ||
| client.on('beforeSendEvent', attachProfiledThreadToEvent); |
There was a problem hiding this comment.
l: is there a specific reason why we use beforeSendEvent rather than e.g. an event processor? My thinking is that by using beforeSendEvent, users cannot filter on the added data in beforeSend (because it's only added to the event after beforeSend). But no need to change if there's a reason!
There was a problem hiding this comment.
Initially, I added this because profiling-node is using the same approach:
But I can also add this via processEvent on the integration.
| const startTimeSec = (profile1.samples[0] as any).timestamp as number; | ||
| const endTimeSec = (profile1.samples[profile1.samples.length - 1] as any).timestamp as number; | ||
| const durationSec = endTimeSec - startTimeSec; | ||
|
|
||
| // Should be at least 20ms based on our setTimeout(21) in the test | ||
| expect(durationSec).toBeGreaterThan(0.2); |
There was a problem hiding this comment.
Nit: instead of relying on an inferred metric, you could just check if sample count > 1
There was a problem hiding this comment.
Same as other comment:
I'm checking the samples count: expect(profile1.samples.length).toBeGreaterThanOrEqual(2);
However, there are more than two samples because the fibonacci function is called a couple of times. So I added this additional check for the duration.
...ckages/browser-integration-tests/suites/profiling/traceLifecycleMode_multiple-chunks/test.ts
Outdated
Show resolved
Hide resolved
...ges/browser-integration-tests/suites/profiling/traceLifecycleMode_multiple-chunks/subject.js
Show resolved
Hide resolved
| console.log('child span'); | ||
| }); | ||
|
|
||
| // Timeout to prevent flaky tests. Integration samples every 20ms, if function is too fast it might not get sampled |
There was a problem hiding this comment.
I think my comment about relying on sample count above might mitigate this
There was a problem hiding this comment.
I'm checking the samples count: expect(profile1.samples.length).toBeGreaterThanOrEqual(2);
However, there are more than two samples because the fibonacci function is called a couple of times. So I added this additional check for the duration.
...s/browser-integration-tests/suites/profiling/traceLifecycleMode_overlapping-spans/subject.js
Outdated
Show resolved
Hide resolved
packages/browser/src/profiling/lifecycleMode/traceLifecycleProfiler.ts
Outdated
Show resolved
Hide resolved
| `[Profiling] Root span ${spanJSON.description} ended. Active root spans: ${this._activeRootSpanCount}`, | ||
| ); | ||
| if (this._activeRootSpanCount === 0) { | ||
| this._collectCurrentChunk().catch(() => /* no catch */ {}); |
There was a problem hiding this comment.
Good point, I added logs in case it catches :)
| } | ||
|
|
||
| this._activeRootSpanIds.delete(spanId); | ||
| this._activeRootSpanCount = Math.max(0, this._activeRootSpanCount - 1); |
There was a problem hiding this comment.
I would add a debugLog here where you check that
if debug && activeRootSpanCount - 1 < 0
we have an SDK bug, span tracking is broken
There was a problem hiding this comment.
I deleted _activeRootSpanCount now to just use this._activeRootSpanIds.size. I figured, I don't need the extra tracking.
packages/browser/src/profiling/lifecycleMode/traceLifecycleProfiler.ts
Outdated
Show resolved
Hide resolved
| }; | ||
|
|
||
| export type BrowserClientProfilingOptions = { | ||
| // todo: add deprecation warning for profilesSampleRate: @deprecated Use `profileSessionSampleRate` and `profileLifecycle` instead. |
There was a problem hiding this comment.
Yes, I would add the deprecation note after "manual" mode is working as well.
size-limit report 📦
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| * - Presence of samples, stacks, frames | ||
| * - Required metadata fields | ||
| */ | ||
| export function validateProfileChunk(chunk: ProfileChunk): { valid: boolean; reason?: string } { |
There was a problem hiding this comment.
bundle size suggestion (feel free to ignore): we can probably save some more bytes here by only returning { valid: true } or {reason: string} (i.e. avoid returning valid: false a bunch of times).
# Conflicts: # packages/browser/src/profiling/utils.ts
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
|
||
| this.stop(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Bug: Root Span Timeout Not Cleared
When a root span ends, its associated timeout is not cleared from _rootSpanTimeouts. This leaves the timeout active, potentially leading to memory or resource leaks and unnecessary callbacks.
There was a problem hiding this comment.
the timeouts are cleared in stop
3 participants
Adds the
tracelifecycle mode and sendsprofile_chunkenvelopes. Also adds test for either overlapping root spans (one chunk) or single root spans (multiple chunks).The "manual" mode comes in another PR to keep this from growing too large.
Browser trace-lifecycle profiler (v2):
Profiles are emitted as standalone
profile_chunkenvelopes either when:Handling never-ending root spans
In the trace lifecycle, profiling continues as long as a root span is active. To prevent profiling endlessly, each root span has its own profile timeout and is terminated if it is too long (5 minutes). If another root span is still active, profiling will continue regardless.
part of #17279
Note
Adds UI profiling trace lifecycle mode that samples sessions, streams profile_chunk envelopes, and attaches thread data, with accompanying tests and type options.
profileLifecycle: 'trace'with session sampling viaprofileSessionSampleRate; defaults lifecycle tomanualwhen unspecified.profile_chunkenvelopes; periodic chunking (60s) and 5‑min root-span timeout.BrowserTraceLifecycleProfilermanages start/stop across root spans and chunk sending.hasLegacyProfiling,shouldProfileSpanLegacy,shouldProfileSession.processEventto attach thread data.startProfileForSpan(processed profile handling).BrowserClientProfilingOptionswithprofileSessionSampleRateandprofileLifecycle; refine Node types docs.@sentry/browserincl. Tracing, Profiling (48 KB).Written by Cursor Bugbot for commit 765f89d. This will update automatically on new commits. Configure here.