chore(compose): Update SentryTraced to emit a span only for initial composition - #6051
chore(compose): Update SentryTraced to emit a span only for initial composition#60510xadam-brown wants to merge 1 commit into
Conversation
0f87b22 to
8b70bf7
Compare
📲 Install BuildsAndroid
|
8b70bf7 to
e048a34
Compare
| ?.apply { spanContext.origin = OP_TRACE_ORIGIN } | ||
| } | ||
|
|
||
| @Immutable internal class ImmutableHolder<T>(var item: T) |
There was a problem hiding this comment.
The @Immutable annotation wasn't doing anything here as ImmutableHolder isn't snapshot state and we never pass it as an argument to a composable function.
There was a problem hiding this comment.
cc @markushi in case I'm overlooking something^^
There was a problem hiding this comment.
just for my understanding, is snapshot state something that causes a re-render?
e048a34 to
a110425
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| val parentRenderingSpan = localSentryRenderingParentSpan.current | ||
|
|
||
| val alreadyComposed = remember(parentCompositionSpan) { MutableRef(false) } | ||
| val alreadyRendered = remember(parentRenderingSpan) { MutableRef(false) } |
There was a problem hiding this comment.
Keying on the parent span doesn't actually change anything yet (though the optics are right) because our implementation of the CompositionLocals above caches the transaction seen by the first SentryTraced to enter the composition in the app's process and never lets it go. That means both that:
- the parents never change (making our keying on the parent spans here an optics-only update for now), and
- once the root transaction finishes, all spans generated by all SentryTraced composables no-op for the remainder of the app process's life.
I'll be fixing (2) in the next PR in this sequence, which will make the parent span keys meaningful.
…omposition Commit updates SentryTraced to: - emit a span only for initial composition, rather than for every composition of the wrapped composable; and - add an origin to ui.render spans, just like we already do for composition spans. Commit also cleans up use of ImmutableHolder and renames it to MutableRef.
277216f to
b9706dc
Compare
runningcode
left a comment
runningcode
left a comment
There was a problem hiding this comment.
I'll give it a 👍 with the caveat that I'm not a compose expert.
2 participants
📜 Description
PR updates SentryTraced to:
ui.renderspans, just like we already do for composition spans.I've also cleaned up use of ImmutableHolder and renamed it to MutableRef in response to @runningcode's reasonable thoughts here.
💡 Motivation and Context
These are behavior changes to follow the fixes in #6049.
💚 How did you test it?
I had my clanker exercise SentryTraced composables in our sample app and verify the output.
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps
Fix the currently broken CompositionLocals that lead to the possibility of a stale parent transaction.