| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
…ocalSentryScopes SDKs that report their own telemetry through a separate IScopes/Hub instance (distinct from the host app's Sentry setup) previously had no way to make SentryTraced trace against that instance, since it always read from the global Sentry.getCurrentScopes(). LocalSentryScopes can now be overridden via CompositionLocalProvider to scope tracing to a specific IScopes, while sibling SentryTraced calls under the same scopes still share one root composition/render span. Fixes getsentry#2668
…an caching Key scopes weakly in RootSpans so custom IScopes instances (and their cached parent spans) can be garbage collected once nothing else holds a reference, instead of being pinned for the lifetime of the root Composition. Also stop permanently caching a null parent span when no transaction is bound yet, so a later transaction on the same scopes is still picked up. Addresses review feedback from sentry-review-bot and cursor bugbot on getsentry#5838. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…an caching Key scopes weakly in RootSpans so custom IScopes instances (and their cached parent spans) can be garbage collected once nothing else holds a reference, instead of being pinned for the lifetime of the root Composition. Also stop permanently caching a null parent span when no transaction is bound yet, so a later transaction on the same scopes is still picked up. Addresses review feedback from sentry-review-bot and cursor bugbot on getsentry#5838.
|
Thanks for the reviews! Addressed everything in 105fd6d:
Existing SentryTracedTest suite (sibling sharing + per-scopes isolation) still passes. |
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM, great contribution!
Sorry, something went wrong.
Missing apiDump entry for the public LocalSentryScopes CompositionLocal was failing the apiCheck CI job. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The cached ImmutableHolder<ISpan?> values strongly referenced their own IScopes key (via Span's internal scopes field), which kept every scopes instance permanently reachable through RootSpans and defeated the WeakHashMap keying entirely. Wrap the cached holder in a WeakReference so the map no longer holds a strong path back to its own key. LocalSentryScopes now defaults to null instead of eagerly resolving Sentry.getCurrentScopes(): a CompositionLocal's default factory runs at most once per process, so the previous default would permanently cache whatever scopes were current on first read (e.g. NoOp scopes if read before Sentry.init()). SentryTraced now falls back to Sentry.getCurrentScopes() on every call when nothing was explicitly provided. Also adds missing Sentry.close() teardown to SentryTracedTest, and fixes a typo in the changelog example. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…r root spans Wrapping the cached root span holder in a WeakReference fixed the prior leak but let GC collect it between recompositions even while a SentryTraced call under that scopes was still mounted, silently breaking the "sibling calls share one root span" guarantee. RootSpans now uses plain HashMaps with reference counting: each SentryTraced call retains its scopes entry via a DisposableEffect and releases it on dispose. An entry is only removed once the last SentryTraced call using that IScopes actually leaves composition, so cleanup no longer depends on GC timing.
… dispose/remember race Retaining inside a DisposableEffect deferred the retain to the effect-application phase, but Compose dispatches an outgoing composable's onDispose before an incoming composable's DisposableEffect in the same recomposition. Replacing a SentryTraced call under a given scopes (e.g. during navigation) could therefore have the outgoing call's release clear the cache before the incoming call's retain ran, so a later SentryTraced call under the same scopes found nothing cached and created a duplicate root span. Retain now happens inside remember instead, which runs synchronously during composition, before any effect-phase work for that frame - including another SentryTraced call's dispose. Release stays in DisposableEffect's onDispose since its timing no longer matters. Adds a regression test that swaps which keyed SentryTraced composable is mounted under the same scopes twice in a row, which reproduces the duplicate span without this fix.
retain ran synchronously as a side effect of remember, while release only ran from DisposableEffect's onDispose. If the composition that retained never actually committed (e.g. an exception thrown elsewhere during the same composition), onDispose never fired, so the refcount was never decremented and the IScopes entry stayed pinned in RootSpans for the process lifetime. Replaced the plain remember + DisposableEffect pair with a single RememberObserver: retain happens in its constructor (still synchronous, so it keeps running before any effect-phase work for that frame), and release happens from either onForgotten (normal dispose) or onAbandoned (composition never committed), whichever Compose actually calls.
isIdle spans only auto-finish when the whole transaction finishes, so evicting a cached root span from RootSpans without finishing it left it open on the transaction. If the same scopes was used again later, a fresh root span was created alongside the still-open, now-untracked original, showing up as a duplicate root span once the transaction finished. release now finishes the evicted composition/rendering spans before dropping them from the cache.
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 20dafa5. Configure here.
Sorry, something went wrong.
…activation Finishing spans when a RootSpans cache entry is evicted turned out to be unsafe: Compose can deactivate a composable's remembered state (onForgotten) and later reactivate the same instance (onRemembered) without re-running remember, e.g. for LazyColumn item reuse. A refcount reaching zero during a brief deactivation doesn't reliably mean the scopes is done for good, so eagerly finishing there risked silently dropping compose/render spans for content that was still in use, just temporarily deactivated. release no longer finishes evicted spans; they still get finished automatically once the whole transaction finishes, same as before this cache existed. The trade-off is a possible extra root span if the same scopes is genuinely reused much later. ScopesRetention also now tracks whether it currently holds a retain and re-retains from onRemembered when reactivated without the constructor re-running, instead of only retaining once at construction time. Without this, a reactivated call's claim on a shared cache entry was silently lost.
|
Sorry, something went wrong.
|
@Tabishahmad thanks for the ping, sorry for the lack of activity on this PR. Let me take of getting this PR merged. |
Sorry, something went wrong.
|
Thanks @markushi , really appreciate you taking care of it! |
Sorry, something went wrong.
|
Hi @Tabishahmad – as you may have seen, we've overhauled SentryTraced over the past week or so in connection with our Nav3 work. It should now be considerably better behaved, esp w/r/t no longer keeping hold expired transactions. Fyi, we landed on an approach that centers on a new LocalSentrySpan (see #6112). It lets host apps inject their preferred parent ISpan into SentryTraced via a composition local. That's one level below what you proposed here, but – like your approach – it gives contexts the ability to construct span hierarchies as they wish. Nicely, receiving composables are encouraged to remain ignorant about those details. Let us know if you think LocalSentrySpan will fit your use case (eg, fetch your IScopes instance in a composable above SentryTraced and use that instance to provide a LocalSentrySpan parent). And thanks again for the contribution 🙌 (Fyi, I've added you as a co-author of #6112, given that it was inspired by your work here. Closing the current PR in light of the above, but feel free to provide any feedback here or in #6112.) |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
SDKs that report their own telemetry through a separate IScopes/Hub instance (distinct from the host app's Sentry setup) previously had no way to make SentryTraced trace against that instance, since it always read from the global Sentry.getCurrentScopes(). LocalSentryScopes can now be overridden via CompositionLocalProvider to scope tracing to a specific IScopes, while sibling SentryTraced calls under the same scopes still share one root composition/render span.
📜 Description
Adds a public LocalSentryScopes CompositionLocal to sentry-compose, defaulting to Sentry.getCurrentScopes() and overridable via CompositionLocalProvider(LocalSentryScopes provides myScopes) { ... }.
SentryTraced now resolves the current IScopes from LocalSentryScopes instead of always reading the global default. The root "Initial Composition" / "Initial Render" spans are cached per IScopes in a shared holder so that sibling SentryTraced calls under the same scopes still share one root span (matching current behavior), while calls under different scopes each get their own independent root span.
💡 Motivation and Context
Enables SDKs/libraries that report their own telemetry through a separate IScopes/Hub instance to use SentryTraced against that instance, independently of the host app's own Sentry setup. Direction confirmed with @markushi and @romtsn on the issue.
💚 How did you test it?
Added SentryTracedTest covering:
Existing ComposeIntegrationTests still pass.
📝 Checklist
🔮 Next steps
Update public API docs for sentry-compose once this lands, to document LocalSentryScopes.