fix(compose): Stop SentryTraced from reusing stale parent spans - #6057
fix(compose): Stop SentryTraced from reusing stale parent spans#60570xadam-brown wants to merge 3 commits into
Conversation
Fix the stale parent problem that causes spans from all SentryTraced instances for the entire app process to be dropped once the initial active transaction finishes. Prior to this commit, SentryTraced used process-wide composition locals to bind the transaction active whenever the first SentryTraced for an app process entered the composition, and to reuse it for all SentryTraced instances thereafter. That meant all SentryTraced spans for the entire app would be dropped for the lifetime of the app process once the initial transaction finished. Oof. This commit sets things right by having each SentryTraced composable request the current active transaction and update the generation of spans accordingly. Because we're no longer relying on a single transaction + parent span pair, SentryTraced now needs to manage the creation of possibly multiple parent span pairs, as the owning transaction updates. That logic lives in the new ParentSpans class.
302c95b to
e508334
Compare
| getByName("androidUnitTest") { | ||
| dependencies { | ||
| implementation(libs.androidx.compose.foundation) | ||
| implementation(libs.androidx.compose.foundation.layout) |
There was a problem hiding this comment.
Fyi, new deps are test-only.
foundation and foundation-layout are needed to keep the Robolectric Compose test runtime from picking up navigation-compose's obsolete transitive foundation artifacts, which led to a BoxKt.maybeCachedBoxMeasurePolicy(...) linkage failure (given the Box used by SentryTraced).
There was a problem hiding this comment.
do you have a build-scan with this issue? I would be curious to examine the dependency tree when this happened to figure out where it came from.
could this happen to our customers?
📲 Install BuildsAndroid
|
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
|
|
||
| private const val OP_TRACE_ORIGIN = "auto.ui.jetpack_compose" | ||
|
|
||
| private val localSentryCompositionParentSpan = compositionLocalOf { |
There was a problem hiding this comment.
These composition locals were the cause of the stale parent bug.
The lambda here is executed when localSentryCompositionParentSpan.current is first read (ie, by the first SentryTraced in the app process to be invoked). CompositionLocal then caches that default value and returns it every time .current is called. Once the original transaction returned by getRootSpan() expired, all remaining spans generated by SentryTraced through the lifetime of the app process were dropped.
There was a problem hiding this comment.
Nice find and explanation!
![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 e508334. Configure here.
Pass the captured start timestamp through the timestamp-aware span overload when creating shared SentryTraced parent spans. This keeps parent spans from starting after their child composition or render spans. Add regression coverage for parent span start ordering. Co-Authored-By: OpenAI GPT-5.5 <noreply@openai.com>
0xadam-brown
added
the
deep-dive
runningcode
left a comment
runningcode
left a comment
There was a problem hiding this comment.
Looks good! I'll note that this is not a deep dive since I am not a compose expert. I added some nits and happy to discuss those if needed.
For my understanding, what are the units the benchmark you performed? and was it a macrobenchmark?
|
|
||
| private const val OP_TRACE_ORIGIN = "auto.ui.jetpack_compose" | ||
|
|
||
| private val localSentryCompositionParentSpan = compositionLocalOf { |
There was a problem hiding this comment.
Nice find and explanation!
| getByName("androidUnitTest") { | ||
| dependencies { | ||
| implementation(libs.androidx.compose.foundation) | ||
| implementation(libs.androidx.compose.foundation.layout) |
There was a problem hiding this comment.
do you have a build-scan with this issue? I would be curious to examine the dependency tree when this happened to figure out where it came from.
could this happen to our customers?
| * | ||
| * **Not threadsafe:** Access must be confined to Compose UI-thread callbacks. | ||
| */ | ||
| private class ParentSpans { |
There was a problem hiding this comment.
I got confused between Owner and Parent. If I understand correctly the Owner is still the Parent of the Parent spans which is why it gets confusing. Can we call these buckets or something else? This way we have the CompositionBucket and the RenderBucket ?
| */ | ||
| private class ParentSpans { | ||
|
|
||
| private var compositionParentSpan: WeakReference<ISpan>? = null |
There was a problem hiding this comment.
it seems these are WeakReferences in order to avoid a circular reference back to the key.
example:
ownerSpanToParentSpans has a ParentSpans value which has a compositionParentSpan which holds a SentryTracer which in this case is the "owner span" which is also the map's key.
the code works as intended. there's no bug here. i'm just saying we should comment why it is a WeakReference here otherwise it isn't immediately obvious why these are WeakReferences and someone might try to change them to be strong references which means it'll never be cleaned up
| * The end result looks something like this: | ||
| * ``` | ||
| * Root span | ||
| * Owner span |
There was a problem hiding this comment.
I don't have a strong opinion on this change, but others might. I wonder if Root span is a Sentry-wide lingo that we should keep.
| return rootSpan | ||
| } | ||
|
|
||
| private fun recordCompositionSpan( |
There was a problem hiding this comment.
| private fun recordCompositionSpan( | |
| private fun recordCompositionSpanForOwner( |
just a thought since the param is named ownerSpan but you often don't check method parameter names.
2 participants
📜 Description
PR fixes the stale parent problem that causes spans to be dropped from all SentryTraced instances for the entire app process after the initial active transaction finishes.
Prior to this PR, SentryTraced used process-wide composition locals to bind the transaction active whenever the first SentryTraced for an app process entered the composition, and to reuse it for all SentryTraced instances thereafter. That meant all SentryTraced spans for the entire app would be dropped for the lifetime of the app process once the initial transaction finished.
This PR sets things right by having each SentryTraced composable request the current active transaction and update the generation of spans accordingly.
Because we're no longer relying on a single transaction + parent span pair, SentryTraced now needs to manage the creation of possibly multiple parent span pairs, as the owning transaction updates. That logic lives in the new ParentSpans class.
💡 Motivation and Context
(See above.)
Measuring performance
Performance of SentryTraced is essentially unchanged, if not slightly better with the introduction of this PR. Expand below to see the details.
Details
I had my clanker run 8 time trials, comparing
mainagainst this PR.Key Timing Results
ui.composechild countComposeActivitychild set in the stock multi-screen flowui.composechild countComposeActivitybuttons_pageui.composeavg msComposeActivitybutton_nav_githubui.composeavg msComposeActivitybutton_dialog#2ui.composeavg msComposeActivitybuttons_pageui.composeavg msComposeActivitybutton_nav_githubui.composeavg msComposeActivitybutton_dialog#2ui.composeavg msComposeActivitybuttons_pageui.renderavg msComposeActivitynavhostui.renderavg msExecutive Summary
ui.composechild countComposeActivitychild spans in the stock flowbuttons_pageui.composeavg msbutton_nav_githubui.composeavg ms💚 How did you test it?
New unit tests + I had my clanker run manual tests for both correctness and performance against the Compose and Nav2 sample apps.
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps