feat(profiling): drop profiler context from transactions when profiler gets rate-limited - #5993
feat(profiling): drop profiler context from transactions when profiler gets rate-limited#5993markushi wants to merge 4 commits into
Conversation
…r gets rate-limited
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
📲 Install BuildsAndroid
|
markushi
added
the
deep-dive
![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 2 potential issues.
❌ 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 b1e0abe. Configure here.
| SentryLevel.ERROR, | ||
| "Failed to start Perfetto profiling. PerfettoProfiler.start() returned false."); | ||
| profilerId = SentryId.EMPTY_ID; | ||
| notifyProfilingCanceled(chunkProfilerId); |
There was a problem hiding this comment.
Sync cancel leaves profiler running
High Severity
When cancellation arrives while start() is still on the stack, notifyProfilingCanceled sees isRunning as false and skips teardown, so it never clears profilerId. startInternal then sets isRunning to true and keeps the already-invalidated id. Later transactions can pick up that dead id and register too late to be notified, which recreates the dangling profile references this change is meant to prevent. The chunk can also keep appearing to run until the timer fires.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit b1e0abe. Configure here.
| isClosed.set(true); | ||
| // sendChunk drops everything once isClosed is set, so the pending chunk is already lost. | ||
| notifyProfilingCanceled(closingProfilerId); | ||
| profilingCanceledCallbacks.clear(); |
There was a problem hiding this comment.
Callbacks run under profiler lock
Medium Severity
notifyProfilingCanceled copies listeners and is written to invoke them after releasing its own lock acquisition, but close(true) and the failure paths in startInternal call it while they still hold the profiler lock. That means every registered transaction callback can run under that lock, blocking other start/stop/register work and raising deadlock risk if a listener ever needs profiler state from another thread.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit b1e0abe. Configure here.
runningcode
left a comment
runningcode
left a comment
There was a problem hiding this comment.
gave it a first pass! looking good!
| transaction = | ||
| spanFactory.createTransaction( | ||
| transactionContext, this, transactionOptions, compositePerformanceCollector); | ||
| // new SentryTracer( |
| if (!continuousProfilerId.equals(SentryId.EMPTY_ID) && Boolean.TRUE.equals(isSampled())) { | ||
| this.contexts.setProfile(new ProfileContext(continuousProfilerId)); | ||
| contexts.setProfile(new ProfileContext(continuousProfilerId)); | ||
| scopes.getOptions().getContinuousProfiler().registerProfilingCanceledCallback(this); |
There was a problem hiding this comment.
registerProfilingCanceledCallback requires obtaining a lock which means this could stall the constructor. The constructor seems to be called from the main thread so this seems to have the potential to cause ANRs.
| } | ||
|
|
||
| override fun registerProfilingCanceledCallback(callback: IProfilingCanceledCallback) { | ||
| TODO("Not yet implemented") |
There was a problem hiding this comment.
I would use a different exception for a stub. this implies it will be implemented in the future.
| for (final @NotNull IProfilingCanceledCallback callback : callbacks) { | ||
| try { | ||
| callback.onProfilingCanceled(canceledProfilerId); | ||
| } catch (Throwable t) { |
There was a problem hiding this comment.
My main concern here is how would we know if this new feature is working or not working correctly? How would we get alerted in the future if a change causes this to break?
I'm not sure I understand the comment about this being an OS binder thread. My understanding is that an uncaught exception always ends the process. The onProfilingCanceled is our own callback so we should understand and control the code there.
| if (isRunning) { | ||
| stopInternal(false); | ||
| } | ||
| // Iterated from a copy, as a listener may unregister itself while being notified. |
There was a problem hiding this comment.
how can they unregister if they require a lock to unregister?
| } | ||
| }); | ||
| }); | ||
| scopes.getOptions().getContinuousProfiler().unregisterProfilingCanceledCallback(this); |
There was a problem hiding this comment.
This might not be called if the condition on line 211 isn't true root.isFinished() && (!transactionOptions.isWaitForChildren() || hasAllChildrenFinished(). would that cause a leak?
There was a problem hiding this comment.
Looking great, and thanks for this 💯
I had two bigger picture comments for your consideration (here and here). Up to you whether to incorporate those ideas now, later, or never.
I also included a few doc comments – but only because I bothered to deep dive into the implementation of our profiling and figured capturing some of that knowledge could be helpful. They're all optional, of course.
Note that I haven't carefully reviewed issues around thread-safety or call sequence in case we end up changing the approach. Happy to take a second pass as needed 👍
| } | ||
|
|
||
| /** | ||
| * Invoked once it is known that no profile chunk will be produced for the given profiler id, so |
There was a problem hiding this comment.
l: "no profile chunk" -> "no additional profile chunks"
| * Invoked once it is known that no profile chunk will be produced for the given profiler id, so | ||
| * that anything already tagged with it can drop the reference before being sent. | ||
| * | ||
| * <p>The id outlives a single chunk, so this may fire more than once for the same id, and it also |
There was a problem hiding this comment.
l: Fwiw, seems like most of this comment would be better suited for SentryTracer.onProfilingCanceled() (but up to you).
Also might be worth noting that transactions are bound to profiling sessions via profiler_id, and a single profiling session can produce 0 to N profile chunks, as that's what leads to the "Losing a valid profile link..." tradeoff you (helpfully!) mention.
| try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) { | ||
| // The OS can report the failure a few ms into a chunk that would otherwise run for another | ||
| // minute. Without tearing it down here, transactions started in the meantime would read the | ||
| // id that was just invalidated, and register too late to ever be told about it. |
There was a problem hiding this comment.
l: My vote would be to implement (and comment) PerfettoContinuousProfiler as if it has a policy that simply cancels the current profiling session whenever we receive a rate-limited response from the ProfilingManager. We could then include discussions about how that policy interacts with transactions higher up our stack, eg, in SentryTracer.
That'd^^ let us be more explicit about the contract in the profiler, while keeping comments about how that contract interacts with transactions in the abstraction that naturally has to care about both.
(Happy to defer to you, of course...)
| } | ||
|
|
||
| /** | ||
| * Sets the callback invoked once it is known that this session will not produce a trace file, |
There was a problem hiding this comment.
l: Consider: "this session" -> "the current request"
(although that's^^ assuming our docs use the term "session" the same way my comments have been, viz., a sequence of 0 to N requests made by PerfettoContinuousProfiler that are associated with the same profiler_id. Maybe we have a separate concept of a ProfilingManager session that exists for a single .pftrace request?? If so, nvm.)
| * Notified when the profiler learns that no profile will ever exist for a profiler id, so that | ||
| * anything already tagged with that id can drop the reference before being sent. | ||
| * | ||
| * <p>Implementations are invoked on whichever thread learns about the failure: an OS binder thread, |
|
|
||
| @ApiStatus.Internal | ||
| public final class SentryTracer implements ITransaction { | ||
| public final class SentryTracer implements ITransaction, IProfilingCanceledCallback { |
There was a problem hiding this comment.
m: Looks like we have two options:
- update SentryTracer in real time as the current profile session is updated (eg., via a callback); or
- have SentryTracer repair its provider_id and the provider_id bound to any of its children when someone calls SentryTracer.finish().
Off the bat, I'm inclined toward (2) because it lets avoid having to copy profiler state here (eg, profilingCanceled) and it makes for a more change-resistant IContinuousProfiler API (ie, a single IContinuousProfiler.getSessionData()-esque method that whose return type can be built out over time, rather than callbacks that we'll likely need to add to and document how they relate to each other). SentryTracer could then grab that session data, see whether it's status is CANCELED (etc.), and take appropriate action.
Thoughts? (And correct me if I'm being too naive!)
| } | ||
|
|
||
| @Override | ||
| public void onProfilingCanceled(final @NotNull SentryId profilerId) { |
There was a problem hiding this comment.
m: Thoughts about also passing info describing whether any ProfileChunks have been successfully produced by the current session + their start and end times? (in raw form or – probably preferably – in the form of a SessionData-type object that lets us query against it)
That'd^^ let us avoid declaring an entire profiling session invalid in cases where one or more ProfileChunks were successfully emitted while the current transaction was alive. And it'd have the nice property of allowing us to solve the more general problem of what we should do for transactions bound to profile sessions that don't produce any profiling data. (Solving the more general problem would mean taking action like we do here both when a session ends exceptionally – eg, rate-limiting – and normally – eg, someone manually calls stop() or no active transaction exists.)
Doesn't have to block this PR, but worth considering...
Background
Details
[I] Continuous profiling happens in sessions, which consist of 0 to N requests for Perfetto .pftrace files from the Android OS
Our PerfettoContinuousProfiler implements a profiling session model, where a session:
- is bound to a single
profiler_id; - consists of one or more sequential calls to Android's ProfilingManager, where each call either generates a .pftrace + ProfileChunk pair, or no-ops if we've been rate-limited by the OS.
Each .pftrace + ProfileChunk covers a 60 second interval.
The profiling session can end in one of two ways:
- naturally (ie, when someone manually calls stop() or no active transaction is present, depending on the profiling mode used); or
- when we're rate-limited by the Android OS.
So long as the session hasn't ended, PerfettoContinuousProfiler automatically calls Android's ProfilingManager again after each .pftrace + ProfileChunk pair is processed.
[II] Creating a transaction eagerly binds it to the currently active profiling session via the latter's profiler_id
...and if no profiling session is active, creating a transaction will start one. Each transaction is bound to the current profiling session before we know whether that session will actually produce profiling data (ie, .pftrace + ProfileChunk pairs).
Sentry's backend makes sure that only ProfileChunks i) associated with a transaction's bound profiler_id and ii) generated during a transaction's lifetime are displayed in the Sentry UI for that transaction (i.e., the backend filters out ProfileChunks generated by the profiling session that didn't occur during the transaction's lifetime).
[III] So what do we do if a transaction is bound to a profiling session that never produces any profiling data?
That, of course, is the question this PR addresses. But as @markushi points out in the PR description, it only addresses a reduced version, viz., what do we do if the Android OS rate-limits us partway through a profiling session?
Three options:
- deem the entire profiling session invalid, including any previously shipped ProfileChunks (the approach taken by this PR);
- deem any prior work done during the session valid, but invalidate the session going forward (what I propose above); or
- do nothing (ie, keep the status quo and the bad Sentry UI experience it produces).
| - Symbolicate tombstone native frames for libraries loaded directly from APKs ([#5992](https://github.com/getsentry/sentry-java/pull/5992)) | ||
| - Drop the `profiler_id` from in-flight transactions when Android's `ProfilingManager` rejects profiling requests ([#5993](https://github.com/getsentry/sentry-java/pull/5993)) | ||
| - Previously a rate-limited or failed Perfetto profiling request still left a `profiler_id` on transactions, pointing to non-existent profiles | ||
|
|
There was a problem hiding this comment.
m: Should we warn folks that we've added non-default methods to IContinuousProfiler, given that it can be vended by an SPI implemented in a different JAR (link)? (I get that the interface is @Internal, but it's a bit unusual given the runtime discovery.)
|
Closing in favor of #6015, as it seems a much more lightweight change and achieves the same goal. I'll try to transfer the learnings/feedback from here to the other PR, so not all is wasted. |
3 participants
📜 Description
Transactions are tagged with the continuous profiler's
profiler_idas soon as they start, but the OS only tells us whether a Perfetto profile actually exists later.This PR adds a callback path so anything already tagged with a dead id can drop it before being sent.
💡 Motivation and Context
Rate limiting is the common case on API 35+, and it is reported roughly 1 ms after the request. Without this, a rate-limited session produces transactions that link to profiles the backend never receives, which surfaces in the UI as dangling profile references.
Unfortunately we do not have any backend logic in place to remove dangling references.
💚 How did you test it?
Added unit tests.
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps
Known gaps left deliberately, happy to fold any of them in:
finish()unregisters unconditionally, and tracers that are never finished stay registered untilclose(true).