Repository navigation
Conversation
Replace the PR number placeholder in the CHANGELOG entry with #6966.
Semver Impact of This PR⚪ None (no version bump detected) 📋 Changelog PreviewThis is how your changes will appear in the changelog.
🤖 This preview updates automatically when you update the PR. |
|
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 13c7c2d. Configure here.
antonis
added
ready-to-merge
antonis
left a comment
antonis
left a comment
There was a problem hiding this comment.
Thank you for your work on this @alwx 🚀
Overall LGTM. I've also run a few tests on an emulator and I think the result is the expected (Sentry link).
The CI looks stuck at this point. Merging the latest main or closing/opening the PR should unstuck it 🤞
Also looping in @markushi who has more context on the Perfetto Android implementation in case he has any feedback.
UI profiling on Android showed only native frames, so the profile of a transaction had no JavaScript functions. The integration now runs the Hermes sampler while the native profiler session runs. Every 60 seconds, and when the session stops, it sends the samples as a JavaScript profile_chunk with the native profiler_id. Sentry merges all chunks of a profiler_id into one profile. The feature is limited to Android API 35 and newer. Older devices use the legacy profiler, whose chunks vroom cannot merge with sample-format chunks (it returns HTTP 400). iOS needs the profiler id from sentry-cocoa first. The native Perfetto profiler samples the JS thread under its OS thread id too. Hermes uses the same id, so the Hermes samples get their own thread id (2^32) and spans point to it. Hermes timestamps use a monotonic clock, so each chunk is anchored to the Unix time when the sampler started. The sampler starts only after the native SDK returns a profiler id. It does not run when the session is not sampled or when transaction profiling (profilesSampleRate) uses the sampler. NATIVE.stopProfiling no longer warns about a missing native profile when only Hermes was started.
The close handler collected the current Hermes chunk but kept the profile session. If NATIVE.startProfiler() resolved after close, the profiler id passed the session check and started the Hermes sampler and its 60 second restart timer again. The close handler now clears the session, so a late profiler id is ignored.
The UI profiling integration turned off Hermes chunks whenever profilesSampleRate was set, also when it was 0. At rate 0 the transaction profiler never starts the Hermes sampler, and the React Native SDK does not pass profilesSampleRate to sentry-java, so the native UI profiler still runs. Hermes chunks are now turned off only for a positive profilesSampleRate.
@sentry/core emits the close hook after it disables the client, so sendEnvelope drops anything sent from there. The close handler built and sent the last Hermes chunk anyway, which only wasted a conversion. It now stops the Hermes sampler without building a chunk. Stopping and sending are split into stopHermesSampler and collectHermesChunk for this.
The last Hermes chunk was lost when the client closed, because @sentry/core disables the client before it emits close. Client.close() flushes first, and flush emits its hook while the client is still enabled and before the transport drains. The integration now sends the current chunk and restarts the sampler on flush, so close sends the last chunk, and Sentry.flush() and the fatal error handler send the JS samples collected so far. The close handler only stops the sampler.
7837007 to
39616cf
Compare
📲 Install BuildsAndroid
|
| client.on('close', () => { | ||
| clearStopTimeout(); | ||
| stopHermesSampler(); | ||
| session = undefined; | ||
| resolveAppStartSession?.(null); | ||
| }); |
There was a problem hiding this comment.
close clears session without stopping the native UI profiler
On close, stop the native profiler (e.g. call NATIVE.stopProfiler() / stopNativeProfiler()) before clearing session; otherwise Perfetto can keep running after SDK shutdown and later profiler.stopProfiler() cannot stop it.
Evidence
- The new
closehandler callsstopHermesSampler()and setssession = undefined, but never callsNATIVE.stopProfiler(). stopNativeProfiler()is the only helper that stops native (NATIVE.stopProfiler()), and it only runs whensyncNativeProfiler()seessession && !shouldRun().- After
sessionis cleared onclose,syncNativeProfiler()will not stop an in-flight native session even ifprofiler.stopProfiler()setsisManualRequested = false. - The close test asserts Hermes start/stop counts only, not
NATIVE.stopProfiler.
Identified by Warden · code-review · X9G-Q5G
| it('does not start the Hermes sampler when the client closes before the native start resolves', async () => { | ||
| const client = setupClient({ lifecycle: 'manual' }); | ||
|
|
||
| profiler.startProfiler(); | ||
| client.emit('close'); | ||
| await jest.advanceTimersByTimeAsync(0); | ||
|
|
||
| expect(NATIVE.startProfiling).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
Client close leaves the profiling integration able to start another session
The close handler stops the current Hermes sampler and clears session, but leaves the integration's run conditions and callbacks active. If afterInit fires later, the handler can call syncNativeProfiler() while isManualRequested is still true; with no session, that starts a new native session and can restart Hermes sampling after close. Make close terminal for profiling by clearing pending run state and guarding later callbacks from starting sessions.
Evidence
- The
closehandler clears the timer and sampler, setssessiontoundefined, and resolves the app-start promise, but leavesisManualRequestedand the resolver unchanged. - The
afterInithandler callssyncNativeProfiler()after resolving a pending app-start session; for manual lifecycle,shouldRun()still returns true after close. - With
sessioncleared,syncNativeProfiler()creates a new session and callsstartHermesChunks(). The existing close-before-start-resolution test does not cover a laterafterInitinvoking this path.
Identified by Warden · find-bugs · 84X-D2R
📢 Type of change
📜 Description
On Android 15 (API 35) and newer,
uiProfilingIntegrationnow runs the Hermes sampler during the native profiler session. Every 60 seconds, and when the session stops, it sends the samples as ajavascriptprofile chunk with the nativeprofiler_id, so Sentry merges JS and native frames into one profile. The Hermes samples use their own thread id (2^32), and spans point to it, because the native Perfetto profiler samples the JS thread under the same OS thread id.💡 Motivation and Context
Closes #6894. UI profiles on Android showed only native frames. The feature is limited to API 35+, because older devices send legacy Android chunks, and vroom cannot merge them with sample-format chunks. iOS needs the profiler id from sentry-cocoa first.
💚 How did you test it?
New Jest tests for the chunk builder, the integration, and the wrapper. I ran the sample app on an API 36 emulator with
profilingOptions(tracelifecycle). Perfetto and the Hermes sampler ran together, chunks were sent every 60 seconds, and both chunks passed Relay's sample v2 parse and normalize rules. The merged profile in the Sentry UI still needs a check.📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps
Add iOS when sentry-cocoa exposes the current profiler id.