fix(android): Stop attributing Nav3 data to Activities without a SentryNavEffect

CHANGELOG.md

@@ -4,6 +4,9 @@
 
 ### Fixes
 
+- Stop attributing errors and spans from an Activity or ViewPager2 page without a `SentryNavEffect` to the most recently visited Navigation 3 root ([#6457](https://github.com/getsentry/sentry-java/pull/6457))
+  - On API 29+, an Activity opened from a Navigation 3 root now gets its own `ui.load` transaction, as the first Activity does at app start. If the new Activity has a `SentryNavEffect`, `ui.load` replaces the navigation transaction for its first destination.
+- Fix missing Navigation 3 transactions for preloaded ViewPager2 pages and destinations reached while another Activity or Fragment owns navigation tracking ([#6400](https://github.com/getsentry/sentry-java/pull/6400))
 - Keep UI profiling running when `startProfiler` is called before a requested stop takes effect at the end of the current chunk ([#6291](https://github.com/getsentry/sentry-java/pull/6291))
   - Before, the start call was ignored and the profiler still stopped at the end of the chunk, even in `TRACE` mode while sampled spans were active. On API 34 and lower, every later profile also stopped after one chunk.
 

sentry-android-core/src/main/java/io/sentry/android/core/ActivityLifecycleIntegration.java

@@ -406,7 +406,11 @@ private void startTracing(final @NotNull Activity activity) {
           }
         }
 
-        // lets bind to the scope so other integrations can pick it up
+        // lets bind to the scope so other integrations can pick it up.
+        // Binding happens in onActivityCreated, after every callback's onActivityPreCreated, so
+        // that sentry-android-navigation3's NavLeaseCoordinator can unbind a covered Activity's nav
+        // transaction first and leave the slot free for this one. Moving it earlier changes
+        // which transaction the incoming Activity gets.
         scopes.configureScope(
             scope -> {
               applyScope(scope, transaction);

sentry-android-core/src/test/java/io/sentry/android/core/ActivityLifecycleIntegrationTest.kt

@@ -15,6 +15,7 @@ import android.view.View
 import android.view.ViewTreeObserver
 import androidx.test.core.app.ApplicationProvider
 import androidx.test.ext.junit.runners.AndroidJUnit4
+import com.google.common.truth.Truth.assertThat
 import io.sentry.DateUtils
 import io.sentry.FullyDisplayedReporter
 import io.sentry.IScope
@@ -889,6 +890,26 @@ class ActivityLifecycleIntegrationTest {
     sut.onActivityCreated(activity, fixture.bundle)
   }
 
+  @Test
+  fun `binds the activity's transaction to the scope when the activity is created, not before`() {
+    val sut = fixture.getSut()
+    fixture.options.tracesSampleRate = 1.0
+    sut.register(fixture.scopes, fixture.options)
+    val scope = Scope(fixture.options)
+    whenever(fixture.scopes.configureScope(any())).thenAnswer {
+      it.getArgument<ScopeCallback>(0).run(scope)
+    }
+    val activity = mock<Activity>()
+
+    sut.onActivityPreCreated(activity, fixture.bundle)
+
+    assertThat(scope.transaction).isNull()
+
+    sut.onActivityCreated(activity, fixture.bundle)
+
+    assertThat(scope.transaction).isSameInstanceAs(fixture.transaction)
+  }
+
   @Test
   fun `When transaction is created, do not overwrite transaction already bound to the Scope`() {
     val sut = fixture.getSut()

sentry-android-navigation3/build.gradle.kts

@@ -63,6 +63,7 @@ dependencies {
   implementation(libs.androidx.lifecycle.runtime.compose)
 
   compileOnly(libs.androidx.compose.runtime)
+  compileOnly(libs.androidx.compose.ui)
 
   testImplementation(libs.androidx.compose.ui.test.junit4)
   testImplementation(libs.androidx.fragment.ktx)

sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/BackStackConverter.kt

@@ -450,7 +450,7 @@ internal data class NormalizedSentryBackStackEntry(
 internal fun List<NormalizedSentryBackStackEntry>.serialize(): List<Map<String, Any?>> =
   map(NormalizedSentryBackStackEntry::serialize)
 
-/** Why host-provided arguments were unavailable in emitted navigation data. */
+/** Why host-provided arguments were unavailable in emitted nav state. */
 internal enum class ArgumentDropReason(
   val serializedValue: String,
   val exhaustsUpdateBudget: Boolean = false,

sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/BackStackEntryMapper.kt

@@ -78,7 +78,7 @@ public class SentryBackStackEntry(
  *
  * If [map] throws, returns `null`, or returns a blank [name][SentryBackStackEntry.name], Sentry
  * records the destination as "/unknown". Doing so signals that name extraction needs to be fixed
- * while avoiding misleading gaps in navigation data.
+ * while avoiding misleading gaps in nav state.
  *
  * For instance, if a user navigates from `/home -> /detail -> /settings`, but the mapper for
  * `/detail` throws, the back stack record will be `/home -> /unknown -> /settings` rather than

sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/BackStackObserver.kt

@@ -2,12 +2,9 @@ package io.sentry.compose.navigation3
 
 import io.sentry.Breadcrumb
 import io.sentry.Hint
-import io.sentry.ITransaction
 import io.sentry.SentryIntegrationPackageStorage
-import io.sentry.SentryLevel.DEBUG
 import io.sentry.SentryLevel.INFO
 import io.sentry.SentryOptions
-import io.sentry.SpanStatus
 import io.sentry.TransactionContext
 import io.sentry.TransactionOptions
 import io.sentry.compose.navigation3.BackStackConverter.RetentionPolicy
@@ -16,6 +13,7 @@ import io.sentry.compose.navigation3.PreparedChange.BackStackHasSameTop
 import io.sentry.compose.navigation3.PreparedChange.BackStackIsEmpty
 import io.sentry.compose.navigation3.lifecycle.INavScope
 import io.sentry.compose.navigation3.lifecycle.INavScopes
+import io.sentry.compose.navigation3.lifecycle.INavTransaction
 import io.sentry.compose.navigation3.lifecycle.NAVIGATION_CONTEXT_KEY
 import io.sentry.protocol.App
 import io.sentry.protocol.TransactionNameSource
@@ -59,7 +57,7 @@ internal class BackStackObserver<T : Any>(
 
   private val backStackConverter = BackStackConverter(entryMapper, scopes.options.logger)
 
-  private val navTransaction = NavTransaction(scopes)
+  private val navTransactions = NavTransactions(scopes)
   private val navBreadcrumbs = NavBreadcrumbs(scopes)
   private val navScreen = NavScreen()
   private val navContext = NavContext(scopes, options)
@@ -83,7 +81,7 @@ internal class BackStackObserver<T : Any>(
   }
 
   /**
-   * Updates Sentry nav data based on the provided [backStack].
+   * Updates Sentry nav state based on the provided [backStack].
    *
    * **Data generated**
    *
@@ -113,7 +111,7 @@ internal class BackStackObserver<T : Any>(
   }
 
   /**
-   * Clears the nav data this observer recorded.
+   * Clears the nav state this observer recorded.
    *
    * Call it when observation ends so that no stale screen, nav context, or nav transaction outlives
    * it.
@@ -194,10 +192,10 @@ internal class BackStackObserver<T : Any>(
       navBreadcrumbs.emit(fromEntry = previousTop, toEntry = currentBackStack.topEntryNormalized)
     }
 
-    navTransaction.stop(scope)
+    navTransactions.stop(scope)
 
     if (areNavigationTransactionsEnabled) {
-      navTransaction
+      navTransactions
         .start(
           scope,
           currentTop.name,
@@ -231,7 +229,7 @@ internal class BackStackObserver<T : Any>(
   }
 
   private fun tearDown(scope: INavScope) {
-    navTransaction.stop(scope)
+    navTransactions.stop(scope)
     navContext.clear(scope)
     navScreen.clear(scope)
     clearPreviousTop()
@@ -277,32 +275,22 @@ private data class BackStackData<T>(
 )
 
 /** A helper class for managing nav transactions. */
-private class NavTransaction(private val scopes: INavScopes) {
+private class NavTransactions(private val scopes: INavScopes) {
 
   private companion object {
     private const val TRANSACTION_ORIGIN = "auto.navigation.nav3"
   }
 
-  private var activeNavTransaction: ITransaction? = null
-
-  /** Starts an idle navigation transaction, or no-ops if another transaction is already active. */
+  /**
+   * Starts an idle navigation transaction, or no-ops if another transaction is already active.
+   *
+   * [Stop][stop] the current navigation transaction first. See [INavScope.startNavTransaction].
+   */
   fun start(
     scope: INavScope,
     name: String,
     arguments: Map<String, Any?>,
-  ): ITransaction? {
-    scope.clearFinishedTransaction()
-
-    if (scope.transaction != null) {
-      scopes.options.logger.log(
-        DEBUG,
-        "Nav3 transaction for route %s won't be created because another transaction is active.",
-        name,
-      )
-
-      return null
-    }
-
+  ): INavTransaction? {
     val transactionOptions =
       TransactionOptions().also {
         it.isWaitForChildren = true
@@ -314,36 +302,25 @@ private class NavTransaction(private val scopes: INavScopes) {
       }
 
     val transaction =
-      scopes.startTransaction(
+      scope.startNavTransaction(
         TransactionContext(name, TransactionNameSource.ROUTE, NAVIGATION_OP),
         transactionOptions,
-      )
+      ) ?: return null
 
-    // Not an error: INavScopes.startTransaction may decline to start a transaction by design.
-    if (transaction.isNoOp) {
-      return null
+    if (arguments.isNotEmpty()) {
+      transaction.setData("arguments", arguments)
     }
 
-    activeNavTransaction = transaction
-
-    transaction.apply {
-      if (arguments.isNotEmpty()) {
-        setData("arguments", arguments)
-      }
-    }
-
-    scope.bindTransaction(transaction)
-
     return transaction
   }
 
-  /** Finishes and unsets the active navigation transaction, if one exists. */
+  /**
+   * Finishes and unsets the active navigation transaction, if one exists.
+   *
+   * See [INavScope.finishNavTransaction].
+   */
   fun stop(scope: INavScope) {
-    val transaction = activeNavTransaction ?: return
-    val status = transaction.status ?: SpanStatus.OK
-    transaction.finish(status)
-    scope.unbindTransaction(transaction)
-    activeNavTransaction = null
+    scope.finishNavTransaction()
   }
 }
 
@@ -377,15 +354,15 @@ private class NavContext(private val scopes: INavScopes, private val options: Se
    * proper values to the transaction context instead.
    */
   fun <T : Any> updateTransaction(
-    transaction: ITransaction,
+    transaction: INavTransaction,
     scope: INavScope,
     backStack: BackStackData<T>,
   ) {
     if (scopes.options.isEnableScreenTracking) {
-      val appContext = transaction.contexts.app ?: scope.app ?: App()
+      val appContext = scope.app ?: App()
 
       appContext.viewNames = listOf(backStack.topEntryNormalized.name)
-      transaction.contexts.setApp(appContext)
+      transaction.setApp(appContext)
     }
 
     if (options.captureBackStack && backStack.capturedEntriesNormalized.isNotEmpty()) {

sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/CoordinatedBackStackObserver.kt

@@ -1,10 +1,10 @@
 package io.sentry.compose.navigation3
 
-import androidx.lifecycle.Lifecycle
 import androidx.lifecycle.LifecycleEventObserver
 import io.sentry.IScope
 import io.sentry.IScopes
 import io.sentry.compose.navigation3.lifecycle.NavLeaseCoordinator
+import io.sentry.compose.navigation3.lifecycle.NavRoot
 import io.sentry.compose.navigation3.lifecycle.NavScopes
 
 /**
@@ -22,7 +22,8 @@ import io.sentry.compose.navigation3.lifecycle.NavScopes
  * **Thread safety**
  *
  * This class is ***not*** thread-safe. Invocations should be confined to the same thread as the
- * [LifecycleEventObserver] callbacks implemented by [NavScopes].
+ * [LifecycleEventObserver] callbacks through which the [NavLeaseCoordinator] observes the attached
+ * lifecycle.
  */
 internal class CoordinatedBackStackObserver<T : Any>(
   scopes: IScopes,
@@ -34,8 +35,8 @@ internal class CoordinatedBackStackObserver<T : Any>(
   private val navScopes = NavScopes(scopes, coordinator)
   private val observer = BackStackObserver(navScopes, options, entryMapper)
 
-  fun attach(lifecycle: Lifecycle) {
-    navScopes.attach(lifecycle)
+  fun attach(root: NavRoot) {
+    navScopes.attach(root)
   }
 
   fun onBackStackChanged(backStack: List<T>) {

sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/SentryNavEffect.kt

@@ -4,11 +4,14 @@ import androidx.compose.runtime.Composable
 import androidx.compose.runtime.DisposableEffect
 import androidx.compose.runtime.remember
 import androidx.compose.runtime.rememberUpdatedState
+import androidx.compose.ui.platform.LocalContext
 import androidx.lifecycle.compose.LocalLifecycleOwner
 import io.sentry.IScopes
 import io.sentry.ScopesAdapter
 import io.sentry.SentryOptions
 import io.sentry.compose.navigation3.lifecycle.NavLeaseCoordinator
+import io.sentry.compose.navigation3.lifecycle.NavRoot
+import io.sentry.compose.navigation3.lifecycle.findActivity
 import org.jetbrains.annotations.ApiStatus
 
 /**
@@ -105,6 +108,7 @@ internal fun <T : Any> SentryNavEffect(
 ) {
   val currentBackStackEntryMapper = rememberUpdatedState(backStackEntryMapper)
   val lifecycle = LocalLifecycleOwner.current.lifecycle
+  val context = LocalContext.current
 
   val observer =
     remember(scopes, options, lifecycle, coordinator) {
@@ -116,9 +120,13 @@ internal fun <T : Any> SentryNavEffect(
       )
     }
 
-  // Register first so the initial prepared back stack update can acquire navigation ownership.
+  // Intentionally don't key on context. It's only used to find the Activity that displays the nav
+  // root, which can't change without the lifecycle changing too, and that replaces the observer.
+  // The context's identity can change more often (e.g., if the host app provides a new
+  // ContextThemeWrapper on every recomposition), and re-attaching for those changes would
+  // needlessly finish the nav transaction.
   DisposableEffect(observer) {
-    observer.attach(lifecycle)
+    observer.attach(NavRoot(lifecycle, context.findActivity()))
     onDispose { observer.dispose() }
   }