feat(profiling): Drop profiler ids from spans no profile covers - #6015
feat(profiling): Drop profiler ids from spans no profile covers#6015markushi wants to merge 5 commits into
Conversation
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
📲 Install BuildsAndroid
|
|
@sentry review |
0xadam-brown
left a comment
There was a problem hiding this comment.
Nice! Much easier to follow, IMO. A few initial comments. Haven't checked for any threading issues. But the APIs are looking clean 💯
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.
Reviewed by Cursor Bugbot for commit 07e9b69. Configure here.
| stopInternal(false); | ||
| isClosed.set(true); | ||
| // sendChunk drops everything once isClosed is set, so the chunk that just ended is lost | ||
| markLastChunkNotRecordedIfUnknown(); |
There was a problem hiding this comment.
Close marks recorded chunks that never send
Medium Severity
close(true) calls stopInternal before isClosed is set. When the OS result is already available, endAndCollect runs onChunkCollected inline, sees isClosed as false, and marks a valid trace as RECORDED. sendChunk is then submitted off-thread; after isClosed flips, that task drops the chunk. markLastChunkNotRecordedIfUnknown does not touch a RECORDED record, so finishing transactions keep a profiler_id for a profile that is never sent.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 07e9b69. Configure here.
There was a problem hiding this comment.
Looks like this one's worth fixing.
PerfettoContinuousProfiler.close(true) has two jobs: stop the current profiling chunk and mark the profiler as closed so no more chunks are sent. But those two decisions are ordered in a way that lets one callback observe the old state and another step observe the new state, meaning a chunk can be marked as recorded in history even though shutdown guarantees it will never be sent.
0xadam-brown
left a comment
There was a problem hiding this comment.
A few minor comments + worth addressing the SentryBot issue; otherwise lgtm
| return startTimestamp; | ||
| } | ||
|
|
||
| public boolean overlaps(final @NotNull SentryDate startTime, final @NotNull SentryDate endTime) { |
There was a problem hiding this comment.
Worth a quick Javadoc reminding folks that this logic should be kept in sync with the rules we use on the backend to decide which chunks to display for a given transaction – at least until our hoped-for tombstone protocol shows up 😄.
| if (endTime.isBefore(startTimestamp)) { | ||
| return false; | ||
| } | ||
| // A chunk that is still running has no end yet, and covers everything from its start on |
There was a problem hiding this comment.
| // A chunk that is still running has no end yet, and covers everything from its start on | |
| // A chunk that is still running has no end yet, and covers everything from its start onward |
There was a problem hiding this comment.
(Fwiw, at first I thought the sentence had been truncated.)
| stopInternal(false); | ||
| isClosed.set(true); | ||
| // sendChunk drops everything once isClosed is set, so the chunk that just ended is lost | ||
| markLastChunkNotRecordedIfUnknown(); |
There was a problem hiding this comment.
Looks like this one's worth fixing.
PerfettoContinuousProfiler.close(true) has two jobs: stop the current profiling chunk and mark the profiler as closed so no more chunks are sent. But those two decisions are ordered in a way that lets one callback observe the old state and another step observe the new state, meaning a chunk can be marked as recorded in history even though shutdown guarantees it will never be sent.
| final @NotNull SentryId profilerId, | ||
| final @NotNull SentryDate startTime, | ||
| final @NotNull SentryDate endTime) { | ||
| return ProfileRecordingState.UNKNOWN; |
There was a problem hiding this comment.
Thoughts about replacing with a default method on IConinuousProfiler that returns UNKNOWN (same throughout)?


📜 Description
Transactions and spans are tagged with the continuous profiler's
profiler_idas soon as they start, but the OS only tells us later whether a Perfetto profile really exists. This PR lets anything tagged with an id that leads nowhere drop the reference before it is sent.IContinuousProfilergets one new method:SentryTracer.finish()asks it once for the root span window and once per child span window, and removes theProfileContextplus theprofiler_idspan data only onNOT_RECORDED.RECORDEDandUNKNOWNchange nothing, so an outcome we do not know never costs a valid link.PerfettoContinuousProfileranswers from a bounded history of chunk records, guarded by a lock of its own so that a finishing transaction never waits for a chunk start, a chunk stop or an OS callback. The answer for a window follows one policy:UNKNOWNRECORDEDUNKNOWNNOT_RECORDEDThe outcome of a chunk has a single owner on each side.
PerfettoProfileronly ever reports a failure — an OS error code, a missing or empty trace file, or the result timeout — as soon as it knows, which is what makes the common rate-limit case cheap.PerfettoContinuousProfileralone declares a chunk recorded, from the trace file it actually received, since the OS can report success and still leave nothing usable behind.NOT_RECORDEDis final, so a result the OS delivers late cannot revive a chunk that was already given up on.NoOpContinuousProfiler,AndroidContinuousProfilerandJavaContinuousProfileranswerUNKNOWN, which leaves every non-Perfetto path exactly as it was.This is an alternative to #5993, which solved the same problem with callbacks from the profiler into the tracer. Pulling the state at send time removes the listener registration, the unregistration on finish, and the tracers that stay registered because they never finish.
💡 Motivation and Context
Rate limiting is the common case on API 35+, and the OS reports it roughly 1 ms after the request. Without this, a rate-limited session produces transactions that link to profiles the backend never receives, which shows up in the UI as dangling profile references. There is no backend logic that removes such references.
💚 How did you test it?
Unit tests, on three levels:
ChunkRecordTest— the state machine (NOT_RECORDEDis final), whether a chunk has ended, and the window arithmetic, including the exact boundaries and a chunk that is still running.PerfettoProfilerTest— the record outcome for an OS error, a null result path, a missing trace file, the result timeout, and a result that arrives after the timeout, plus the two cases where a successful result leaves the record untouched.PerfettoContinuousProfilerTest— the answer for a running chunk, a chunk that is still being collected, a collected chunk, a failed chunk, a failed and an unknown chunk under one window, a window after the last chunk, a window that starts before the profiler did, per-chunk judgement across a failed and a recorded chunk, eviction, and close.SentryTracerTest— dropped, kept and unknown outcomes, per-span judgement, a span that never finished, and aprofiler_idthe SDK did not write.📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps
Known limits, left out deliberately:
PerfettoProfiler.start(long)was public and is now package-private, since it returns the internal chunk record. The class is@ApiStatus.Internaland@RequiresApi(35), but the method was on the published surface in 8.51.0–8.53.0.SentryTracerdrops the id. OpenTelemetry spans take another path and are not covered. This has no effect today, as only the Android Perfetto profiler ever answers anything butUNKNOWN.Sentry.initreplaced reads asNOT_RECORDED. Keeping such ids alive belongs to whatever owns the profiler lifecycle across inits, not to the chunk history.