-
-
Notifications
You must be signed in to change notification settings - Fork 476
fix(compose): allow overriding the IScopes used by SentryTraced via LocalSentryScopes #5838
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
d3d38c7
d1e90a3
5a43f30
105fd6d
dfa46df
446749f
6702434
6bd3aa5
863894f
20dafa5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,11 +4,14 @@ import androidx.compose.foundation.layout.Box | |
| import androidx.compose.foundation.layout.BoxScope | ||
| import androidx.compose.runtime.Composable | ||
| import androidx.compose.runtime.Immutable | ||
| import androidx.compose.runtime.compositionLocalOf | ||
| import androidx.compose.runtime.ProvidableCompositionLocal | ||
| import androidx.compose.runtime.RememberObserver | ||
| import androidx.compose.runtime.remember | ||
| import androidx.compose.runtime.staticCompositionLocalOf | ||
| import androidx.compose.ui.ExperimentalComposeUiApi | ||
| import androidx.compose.ui.Modifier | ||
| import androidx.compose.ui.draw.drawWithContent | ||
| import io.sentry.IScopes | ||
| import io.sentry.ISpan | ||
| import io.sentry.Sentry | ||
| import io.sentry.SpanOptions | ||
|
|
@@ -24,15 +27,24 @@ private const val OP_TRACE_ORIGIN = "auto.ui.jetpack_compose" | |
|
|
||
| @Immutable private class ImmutableHolder<T>(var item: T) | ||
|
|
||
| private fun getRootSpan(): ISpan? { | ||
| // Defaults to null rather than eagerly resolving Sentry.getCurrentScopes(): a CompositionLocal's | ||
| // default value is computed at most once for the process lifetime, so a non-null default would | ||
| // permanently cache whatever scopes were current on first read (e.g. a NoOp scopes if read before | ||
| // Sentry.init()). SentryTraced instead falls back to Sentry.getCurrentScopes() on every call when | ||
| // no value has been explicitly provided, so it always reflects the current scopes. | ||
| public val LocalSentryScopes: ProvidableCompositionLocal<IScopes?> = staticCompositionLocalOf { | ||
| null | ||
| } | ||
|
|
||
| private fun getRootSpan(scopes: IScopes): ISpan? { | ||
| var rootSpan: ISpan? = null | ||
| Sentry.configureScope { rootSpan = it.transaction } | ||
| scopes.configureScope { rootSpan = it.transaction } | ||
| return rootSpan | ||
| } | ||
|
|
||
| private val localSentryCompositionParentSpan = compositionLocalOf { | ||
| private fun createCompositionParentSpan(scopes: IScopes): ImmutableHolder<ISpan?> = | ||
| ImmutableHolder( | ||
| getRootSpan() | ||
| getRootSpan(scopes) | ||
| ?.startChild( | ||
| OP_PARENT_COMPOSITION, | ||
| "Jetpack Compose Initial Composition", | ||
|
|
@@ -44,11 +56,10 @@ private val localSentryCompositionParentSpan = compositionLocalOf { | |
| ) | ||
| ?.apply { spanContext.origin = OP_TRACE_ORIGIN } | ||
| ) | ||
| } | ||
|
|
||
| private val localSentryRenderingParentSpan = compositionLocalOf { | ||
| private fun createRenderingParentSpan(scopes: IScopes): ImmutableHolder<ISpan?> = | ||
| ImmutableHolder( | ||
| getRootSpan() | ||
| getRootSpan(scopes) | ||
| ?.startChild( | ||
| OP_PARENT_RENDER, | ||
| "Jetpack Compose Initial Render", | ||
|
|
@@ -60,8 +71,78 @@ private val localSentryRenderingParentSpan = compositionLocalOf { | |
| ) | ||
| ?.apply { spanContext.origin = OP_TRACE_ORIGIN } | ||
| ) | ||
|
|
||
| private class RootSpans { | ||
| // Entries are cleaned up deterministically via reference counting (see retain/release) rather | ||
| // than relying on GC to reclaim a weakly-keyed map: a value here (ImmutableHolder -> Span) | ||
| // strongly references the IScopes that created it (Span keeps its Scopes alive), so a | ||
| // WeakHashMap keyed on IScopes would never actually evict its own key, and a value wrapped in a | ||
| // WeakReference could be collected between recompositions even while a SentryTraced call under | ||
| // that scopes is still in composition, silently breaking root span sharing. | ||
| val compositionSpans = HashMap<IScopes, ImmutableHolder<ISpan?>>() | ||
| val renderingSpans = HashMap<IScopes, ImmutableHolder<ISpan?>>() | ||
| private val refCounts = HashMap<IScopes, Int>() | ||
|
|
||
| fun retain(scopes: IScopes) { | ||
| refCounts[scopes] = (refCounts[scopes] ?: 0) + 1 | ||
| } | ||
|
|
||
| fun release(scopes: IScopes) { | ||
| val remaining = (refCounts[scopes] ?: 1) - 1 | ||
| if (remaining <= 0) { | ||
| refCounts.remove(scopes) | ||
| // These are idle spans: they only auto-finish when the whole transaction finishes, so | ||
| // evicting them from the cache without finishing them here would leave them open on the | ||
| // transaction. If the same scopes is used again later, a fresh pair would be created | ||
| // alongside the still-open, now-untracked originals, showing up as duplicate root spans. | ||
| compositionSpans.remove(scopes)?.item?.finish() | ||
| renderingSpans.remove(scopes)?.item?.finish() | ||
| } else { | ||
| refCounts[scopes] = remaining | ||
| } | ||
| } | ||
|
cursor[bot] marked this conversation as resolved.
|
||
| } | ||
|
|
||
| // Retains scopes in the constructor so it always runs synchronously during composition, before | ||
| // any effect-phase work for this frame (including another SentryTraced call's dispose): Compose | ||
| // runs the dispose of an outgoing node before the effects of an incoming one in the same | ||
| // recomposition, so retaining from an effect could let a shared entry's refcount hit zero (and | ||
| // get evicted) between an old and a new SentryTraced call sharing the same scopes. Releasing from | ||
| // both onForgotten and onAbandoned (rather than a DisposableEffect) also covers a composition | ||
| // that never commits (e.g. an exception elsewhere during composition), which would otherwise | ||
| // leave the retain in place with no matching release. | ||
| private class ScopesRetention(private val rootSpans: RootSpans, private val scopes: IScopes) : | ||
| RememberObserver { | ||
| init { | ||
| rootSpans.retain(scopes) | ||
| } | ||
|
|
||
| override fun onRemembered() {} | ||
|
|
||
| override fun onForgotten() { | ||
| rootSpans.release(scopes) | ||
| } | ||
|
|
||
| override fun onAbandoned() { | ||
| rootSpans.release(scopes) | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Retain skips reusable re-rememberMedium Severity
Reviewed by Cursor Bugbot for commit 20dafa5. Configure here. |
||
| } | ||
|
|
||
| private fun getOrCreateParentSpan( | ||
| map: MutableMap<IScopes, ImmutableHolder<ISpan?>>, | ||
| scopes: IScopes, | ||
| create: (IScopes) -> ImmutableHolder<ISpan?>, | ||
| ): ImmutableHolder<ISpan?> = | ||
| // Only cache the holder once it actually contains a span; a null result (no transaction bound | ||
| // to the scopes yet) is recomputed on the next call so a later transaction is still picked up. | ||
| map[scopes] ?: create(scopes).also { if (it.item != null) map[scopes] = it } | ||
|
|
||
| // Cached once per Composition and shared by every SentryTraced call within it, mirroring the | ||
| // old eagerly-computed `compositionLocalOf { ... }` default (which Compose resolves once and | ||
| // reuses for every `.current` read that has no ancestor Provider, sibling or not). Keyed per | ||
| // IScopes so distinct custom scopes each get their own root span instead of colliding. | ||
| private val LocalRootSpans = staticCompositionLocalOf { RootSpans() } | ||
|
sentry[bot] marked this conversation as resolved.
|
||
|
|
||
| @ExperimentalComposeUiApi | ||
| @Composable | ||
| public fun SentryTraced( | ||
|
|
@@ -70,8 +151,14 @@ public fun SentryTraced( | |
| enableUserInteractionTracing: Boolean = true, | ||
| content: @Composable BoxScope.() -> Unit, | ||
| ) { | ||
| val parentCompositionSpan = localSentryCompositionParentSpan.current | ||
| val parentRenderingSpan = localSentryRenderingParentSpan.current | ||
| val scopes = LocalSentryScopes.current ?: Sentry.getCurrentScopes() | ||
| val rootSpans = LocalRootSpans.current | ||
| remember(rootSpans, scopes) { ScopesRetention(rootSpans, scopes) } | ||
| val parentCompositionSpan = | ||
| getOrCreateParentSpan(rootSpans.compositionSpans, scopes, ::createCompositionParentSpan) | ||
| val parentRenderingSpan = | ||
| getOrCreateParentSpan(rootSpans.renderingSpans, scopes, ::createRenderingParentSpan) | ||
|
|
||
| val compositionSpan = | ||
| parentCompositionSpan.item?.startChild(OP_COMPOSE, tag)?.apply { | ||
| spanContext.origin = OP_TRACE_ORIGIN | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,173 @@ | ||
| package io.sentry.compose | ||
|
|
||
| import android.app.Application | ||
| import android.content.ComponentName | ||
| import androidx.activity.ComponentActivity | ||
| import androidx.compose.foundation.layout.Box | ||
| import androidx.compose.runtime.CompositionLocalProvider | ||
| import androidx.compose.runtime.getValue | ||
| import androidx.compose.runtime.key | ||
| import androidx.compose.runtime.mutableStateOf | ||
| import androidx.compose.runtime.setValue | ||
| import androidx.compose.ui.ExperimentalComposeUiApi | ||
| import androidx.compose.ui.test.junit4.createAndroidComposeRule | ||
| import androidx.test.core.app.ApplicationProvider | ||
| import androidx.test.ext.junit.runners.AndroidJUnit4 | ||
| import io.sentry.IScopes | ||
| import io.sentry.ITransaction | ||
| import io.sentry.Sentry | ||
| import io.sentry.SentryOptions | ||
| import io.sentry.TransactionOptions | ||
| import io.sentry.test.createTestScopes | ||
| import kotlin.test.assertEquals | ||
| import kotlin.test.assertTrue | ||
| import org.junit.After | ||
| import org.junit.Rule | ||
| import org.junit.Test | ||
| import org.junit.rules.TestWatcher | ||
| import org.junit.runner.Description | ||
| import org.junit.runner.RunWith | ||
| import org.robolectric.Shadows | ||
| import org.robolectric.annotation.Config | ||
|
|
||
| @OptIn(ExperimentalComposeUiApi::class) | ||
| @RunWith(AndroidJUnit4::class) | ||
| @Config(sdk = [30]) | ||
| class SentryTracedTest { | ||
| // workaround for robolectric tests with composeRule | ||
| // from https://github.com/robolectric/robolectric/pull/4736#issuecomment-1831034882 | ||
| @get:Rule(order = 1) | ||
| val addActivityToRobolectricRule = | ||
| object : TestWatcher() { | ||
| override fun starting(description: Description?) { | ||
| super.starting(description) | ||
| val appContext: Application = ApplicationProvider.getApplicationContext() | ||
| Shadows.shadowOf(appContext.packageManager) | ||
| .addActivityIfNotPresent( | ||
| ComponentName(appContext.packageName, ComponentActivity::class.java.name) | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| @get:Rule(order = 2) val rule = createAndroidComposeRule<ComponentActivity>() | ||
|
|
||
| @After | ||
| fun tearDown() { | ||
| Sentry.close() | ||
| } | ||
|
|
||
| private fun newTracingScopes(): IScopes = | ||
| createTestScopes( | ||
| SentryOptions().apply { | ||
| dsn = "https://key@sentry.io/proj" | ||
| tracesSampleRate = 1.0 | ||
| } | ||
| ) | ||
|
|
||
| private fun IScopes.startBoundTransaction(name: String): ITransaction = | ||
| startTransaction(name, "test", TransactionOptions().apply { isBindToScope = true }) | ||
|
|
||
| @Test | ||
| fun `SentryTraced creates its root span on the scopes provided via LocalSentryScopes`() { | ||
| val scopes = newTracingScopes() | ||
| val tx = scopes.startBoundTransaction("custom-scopes-tx") | ||
|
|
||
| rule.setContent { | ||
| CompositionLocalProvider(LocalSentryScopes provides scopes) { | ||
| SentryTraced(tag = "custom") { Box {} } | ||
| } | ||
| } | ||
| rule.waitForIdle() | ||
|
|
||
| assertEquals(1, tx.spans.count { it.operation == "ui.compose.composition" }) | ||
| assertEquals(1, tx.spans.count { it.operation == "ui.compose" }) | ||
| } | ||
|
|
||
| @Test | ||
| fun `sibling SentryTraced composables under the same scopes share one root span`() { | ||
| val scopes = newTracingScopes() | ||
| val tx = scopes.startBoundTransaction("custom-scopes-tx") | ||
|
|
||
| rule.setContent { | ||
| CompositionLocalProvider(LocalSentryScopes provides scopes) { | ||
| SentryTraced(tag = "first") { Box {} } | ||
| SentryTraced(tag = "second") { Box {} } | ||
| } | ||
| } | ||
| rule.waitForIdle() | ||
|
|
||
| assertEquals(1, tx.spans.count { it.operation == "ui.compose.composition" }) | ||
| assertEquals(2, tx.spans.count { it.operation == "ui.compose" }) | ||
| } | ||
|
|
||
| @Test | ||
| fun `SentryTraced composables under different scopes do not interfere with each other`() { | ||
| val scopesA = newTracingScopes() | ||
| val txA = scopesA.startBoundTransaction("scopes-a-tx") | ||
| val scopesB = newTracingScopes() | ||
| val txB = scopesB.startBoundTransaction("scopes-b-tx") | ||
|
|
||
| rule.setContent { | ||
| CompositionLocalProvider(LocalSentryScopes provides scopesA) { | ||
| SentryTraced(tag = "a") { Box {} } | ||
| } | ||
| CompositionLocalProvider(LocalSentryScopes provides scopesB) { | ||
| SentryTraced(tag = "b") { Box {} } | ||
| } | ||
| } | ||
| rule.waitForIdle() | ||
|
|
||
| assertEquals(1, txA.spans.count { it.operation == "ui.compose.composition" }) | ||
| assertEquals(1, txA.spans.count { it.operation == "ui.compose" }) | ||
| assertEquals(1, txB.spans.count { it.operation == "ui.compose.composition" }) | ||
| assertEquals(1, txB.spans.count { it.operation == "ui.compose" }) | ||
| } | ||
|
|
||
| @Test | ||
| fun `repeatedly replacing a traced composable under the same scopes keeps sharing the root span`() { | ||
| // Each swap disposes the outgoing keyed composable and mounts a new one under the same | ||
| // scopes within a single recomposition. Compose dispatches the outgoing composable's | ||
| // onForgotten before the incoming one's onRemembered, so a second swap is needed to surface | ||
| // a root span cache that was cleared out from under a still-live retain. | ||
| val scopes = newTracingScopes() | ||
| val tx = scopes.startBoundTransaction("custom-scopes-tx") | ||
| var step by mutableStateOf(0) | ||
|
|
||
| rule.setContent { | ||
| CompositionLocalProvider(LocalSentryScopes provides scopes) { | ||
| key(step) { SentryTraced(tag = "step-$step") { Box {} } } | ||
| } | ||
| } | ||
| rule.waitForIdle() | ||
|
|
||
| step = 1 | ||
| rule.waitForIdle() | ||
|
|
||
| step = 2 | ||
| rule.waitForIdle() | ||
|
|
||
| assertEquals(1, tx.spans.count { it.operation == "ui.compose.composition" }) | ||
| } | ||
|
|
||
| @Test | ||
| fun `evicting a root span cache entry finishes the underlying span`() { | ||
| val scopes = newTracingScopes() | ||
| val tx = scopes.startBoundTransaction("custom-scopes-tx") | ||
| var mounted by mutableStateOf(true) | ||
|
|
||
| rule.setContent { | ||
| if (mounted) { | ||
| CompositionLocalProvider(LocalSentryScopes provides scopes) { | ||
| SentryTraced(tag = "first") { Box {} } | ||
| } | ||
| } | ||
| } | ||
| rule.waitForIdle() | ||
|
|
||
| mounted = false | ||
| rule.waitForIdle() | ||
|
|
||
| val compositionRootSpan = tx.spans.first { it.operation == "ui.compose.composition" } | ||
| assertTrue(compositionRootSpan.isFinished) | ||
| } | ||
| } |


Uh oh!
There was an error while loading. Please reload this page.