-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(browserprofiling): Add manual mode and deprecate old profiling
#18189
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
size-limit report 📦
|
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._sessionSampled = sessionSampled; | ||
| this._lifecycleMode = lifecycleMode; | ||
|
|
||
| client.on('spanStart', span => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The client.on calls (spanStart, spanEnd) are now added in the _setupTraceLifecycleListeners function.
# Conflicts: # packages/browser/src/profiling/UIProfiler.ts
dev-packages/browser-integration-tests/suites/profiling/manualMode/test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is similar to the Node profiling one: packages/core/src/profiling.ts (can be seen at the end of this diff page)
dev-packages/browser-integration-tests/suites/profiling/manualMode/test.ts
Outdated
Show resolved
Hide resolved
| // Trace: Profile context is kept as long as there is an active root span | ||
| if (this._lifecycleMode === 'manual') { | ||
| getGlobalScope().setContext('profile', {}); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Profiling state leaks after trace completion.
In trace mode, when _endProfiling is called after the last root span ends, the profile context is not cleared. The condition if (this._lifecycleMode === 'manual') prevents clearing the context in trace mode, but when there are no active root spans (which is when _endProfiling is called from spanEnd), the context should be cleared to prevent subsequent transactions from being incorrectly marked as profiled. The profile context persists even though profiling has stopped, causing spans created after profiling ends to incorrectly appear as profiled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The profiler context will stay the same for the whole profiling session as the profiler cannot be just stopped like in manual profiling, where there might be spans that are not profiled anymore.
Lms24
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! Had one thought about the public API <> integration interaction but otherwise LGTM!
| // In manual mode we start and stop once -> expect exactly one chunk | ||
| const profileChunkEnvelopes = await getMultipleSentryEnvelopeRequests<ProfileChunkEnvelope>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l: the code suggests we get two envelopes/chunks. Should we update/remove the comment?
| const _browserProfilingIntegration = (() => { | ||
| return { | ||
| name: INTEGRATION_NAME, | ||
| _profiler: new UIProfiler(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l/m: In case the CDN bundle tests fail, this is likely the reason why: _profiler is mangled by our terser config (makeTerserPlugin()) because it is not excluded from the ignore list of private properties.
We can either opt out of mangling this field, make it "public" (i.e. remove the leading underscore), or add client hooks instead of exposing any additional field. I'd personally prefer using client hooks because it makes the binding very loose and also removes the need for the isProfilingIntegrationWithProfiler type predicate. Happy to leave this up to you though!
Adds the
manualmode for profiling and browser integration tests.Closes #17279