(Android) Use root transaction instead of last active span (#2855)

markushi · web-flow · commit cd268a300fe3 · 2023-08-31T14:15:00.000+02:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -15,6 +15,7 @@ Breaking changes:
 - Apollo v2 BeforeSpanCallback now allows returning null ([#2890](https://github.com/getsentry/sentry-java/pull/2890))
 - Automatic user interaction tracking: every click now starts a new automatic transaction ([#2891](https://github.com/getsentry/sentry-java/pull/2891))
   - Previously performing a click on the same UI widget twice would keep the existing transaction running, the new behavior now better aligns with other SDKs
+- Android only: If global hub mode is enabled, Sentry.getSpan() returns the root span instead of the latest span ([#2855](https://github.com/getsentry/sentry-java/pull/2855))
 
 ### Fixes
 
diff --git a/sentry-android-okhttp/src/main/java/io/sentry/android/okhttp/SentryOkHttpEvent.kt b/sentry-android-okhttp/src/main/java/io/sentry/android/okhttp/SentryOkHttpEvent.kt
@@ -14,6 +14,7 @@ import io.sentry.android.okhttp.SentryOkHttpEventListener.Companion.REQUEST_HEAD
 import io.sentry.android.okhttp.SentryOkHttpEventListener.Companion.RESPONSE_BODY_EVENT
 import io.sentry.android.okhttp.SentryOkHttpEventListener.Companion.RESPONSE_HEADERS_EVENT
 import io.sentry.android.okhttp.SentryOkHttpEventListener.Companion.SECURE_CONNECT_EVENT
+import io.sentry.util.Platform
 import io.sentry.util.UrlUtils
 import okhttp3.Request
 import okhttp3.Response
@@ -37,7 +38,8 @@ internal class SentryOkHttpEvent(private val hub: IHub, private val request: Req
         val method: String = request.method
 
         // We start the call span that will contain all the others
-        callRootSpan = hub.span?.startChild("http.client", "$method $url")
+        val parentSpan = if (Platform.isAndroid()) hub.transaction else hub.span
+        callRootSpan = parentSpan?.startChild("http.client", "$method $url")
         callRootSpan?.spanContext?.origin = TRACE_ORIGIN
         urlDetails.applyToSpan(callRootSpan)
 
diff --git a/sentry-android-okhttp/src/main/java/io/sentry/android/okhttp/SentryOkHttpInterceptor.kt b/sentry-android-okhttp/src/main/java/io/sentry/android/okhttp/SentryOkHttpInterceptor.kt
@@ -19,6 +19,7 @@ import io.sentry.exception.ExceptionMechanismException
 import io.sentry.exception.SentryHttpClientException
 import io.sentry.protocol.Mechanism
 import io.sentry.util.HttpUtils
+import io.sentry.util.Platform
 import io.sentry.util.PropagationTargetsUtils
 import io.sentry.util.TracingUtils
 import io.sentry.util.UrlUtils
@@ -78,7 +79,8 @@ class SentryOkHttpInterceptor(
             isFromEventListener = true
         } else {
             // read the span from the bound scope
-            span = hub.span?.startChild("http.client", "$method $url")
+            val parentSpan = if (Platform.isAndroid()) hub.transaction else hub.span
+            span = parentSpan?.startChild("http.client", "$method $url")
             isFromEventListener = false
         }
 
diff --git a/sentry-apollo-3/src/main/java/io/sentry/apollo3/SentryApollo3HttpInterceptor.kt b/sentry-apollo-3/src/main/java/io/sentry/apollo3/SentryApollo3HttpInterceptor.kt
@@ -32,6 +32,7 @@ import io.sentry.util.PropagationTargetsUtils
 import io.sentry.util.TracingUtils
 import io.sentry.util.UrlUtils
 import io.sentry.vendor.Base64
+import okhttp3.internal.platform.Platform
 import okio.Buffer
 import org.jetbrains.annotations.ApiStatus
 
@@ -62,7 +63,7 @@ class SentryApollo3HttpInterceptor @JvmOverloads constructor(
         request: HttpRequest,
         chain: HttpInterceptorChain
     ): HttpResponse {
-        val activeSpan = hub.span
+        val activeSpan = if (io.sentry.util.Platform.isAndroid()) hub.transaction else hub.span
 
         val operationName = getHeader(HEADER_APOLLO_OPERATION_NAME, request.headers)
         val operationType = decodeHeaderValue(request, SENTRY_APOLLO_3_OPERATION_TYPE)
diff --git a/sentry-apollo-3/src/test/java/io/sentry/apollo3/SentryApollo3InterceptorTest.kt b/sentry-apollo-3/src/test/java/io/sentry/apollo3/SentryApollo3InterceptorTest.kt
@@ -25,11 +25,13 @@ import io.sentry.TransactionContext
 import io.sentry.apollo3.SentryApollo3HttpInterceptor.BeforeSpanCallback
 import io.sentry.protocol.SdkVersion
 import io.sentry.protocol.SentryTransaction
+import io.sentry.util.Apollo3PlatformTestManipulator
 import kotlinx.coroutines.launch
 import kotlinx.coroutines.runBlocking
 import okhttp3.mockwebserver.MockResponse
 import okhttp3.mockwebserver.MockWebServer
 import okhttp3.mockwebserver.SocketPolicy
+import org.junit.Before
 import org.mockito.kotlin.any
 import org.mockito.kotlin.anyOrNull
 import org.mockito.kotlin.check
@@ -112,6 +114,11 @@ class SentryApollo3InterceptorTest {
 
     private val fixture = Fixture()
 
+    @Before
+    fun setup() {
+        Apollo3PlatformTestManipulator.pretendIsAndroid(false)
+    }
+
     @Test
     fun `creates a span around the successful request`() {
         executeQuery()
@@ -307,6 +314,20 @@ class SentryApollo3InterceptorTest {
         assert(packageInfo.version == BuildConfig.VERSION_NAME)
     }
 
+    @Test
+    fun `attaches to root transaction on Android`() {
+        Apollo3PlatformTestManipulator.pretendIsAndroid(true)
+        executeQuery(fixture.getSut())
+        verify(fixture.hub).transaction
+    }
+
+    @Test
+    fun `attaches to child span on non-Android`() {
+        Apollo3PlatformTestManipulator.pretendIsAndroid(false)
+        executeQuery(fixture.getSut())
+        verify(fixture.hub).span
+    }
+
     private fun assertTransactionDetails(it: SentryTransaction, httpStatusCode: Int? = 200, contentLength: Long? = 0L) {
         assertEquals(1, it.spans.size)
         val httpClientSpan = it.spans.first()
@@ -328,6 +349,7 @@ class SentryApollo3InterceptorTest {
         var tx: ITransaction? = null
         if (isSpanActive) {
             tx = SentryTracer(TransactionContext("op", "desc", TracesSamplingDecision(true)), fixture.hub)
+            whenever(fixture.hub.transaction).thenReturn(tx)
             whenever(fixture.hub.span).thenReturn(tx)
         }
 
diff --git a/sentry-apollo-3/src/test/java/io/sentry/util/Apollo3PlatformTestManipulator.kt b/sentry-apollo-3/src/test/java/io/sentry/util/Apollo3PlatformTestManipulator.kt
@@ -0,0 +1,8 @@
+package io.sentry.util
+
+object Apollo3PlatformTestManipulator {
+
+    fun pretendIsAndroid(isAndroid: Boolean) {
+        Platform.isAndroid = isAndroid
+    }
+}
diff --git a/sentry-apollo/src/main/java/io/sentry/apollo/SentryApolloInterceptor.kt b/sentry-apollo/src/main/java/io/sentry/apollo/SentryApolloInterceptor.kt
@@ -44,7 +44,7 @@ class SentryApolloInterceptor(
     }
 
     override fun interceptAsync(request: InterceptorRequest, chain: ApolloInterceptorChain, dispatcher: Executor, callBack: CallBack) {
-        val activeSpan = hub.span
+        val activeSpan = if (io.sentry.util.Platform.isAndroid()) hub.transaction else hub.span
         if (activeSpan == null) {
             val headers = addTracingHeaders(request, null)
             val modifiedRequest = request.toBuilder().requestHeaders(headers).build()
diff --git a/sentry-apollo/src/test/java/io/sentry/apollo/SentryApolloInterceptorTest.kt b/sentry-apollo/src/test/java/io/sentry/apollo/SentryApolloInterceptorTest.kt
@@ -19,11 +19,13 @@ import io.sentry.TracesSamplingDecision
 import io.sentry.TransactionContext
 import io.sentry.protocol.SdkVersion
 import io.sentry.protocol.SentryTransaction
+import io.sentry.util.ApolloPlatformTestManipulator
 import kotlinx.coroutines.launch
 import kotlinx.coroutines.runBlocking
 import okhttp3.mockwebserver.MockResponse
 import okhttp3.mockwebserver.MockWebServer
 import okhttp3.mockwebserver.SocketPolicy
+import org.junit.Before
 import org.mockito.kotlin.any
 import org.mockito.kotlin.anyOrNull
 import org.mockito.kotlin.check
@@ -93,6 +95,11 @@ class SentryApolloInterceptorTest {
 
     private val fixture = Fixture()
 
+    @Before
+    fun setup() {
+        ApolloPlatformTestManipulator.pretendIsAndroid(false)
+    }
+
     @Test
     fun `creates a span around the successful request`() {
         executeQuery()
@@ -234,6 +241,20 @@ class SentryApolloInterceptorTest {
         assert(packageInfo.version == BuildConfig.VERSION_NAME)
     }
 
+    @Test
+    fun `attaches to root transaction on Android`() {
+        ApolloPlatformTestManipulator.pretendIsAndroid(true)
+        executeQuery(fixture.getSut())
+        verify(fixture.hub).transaction
+    }
+
+    @Test
+    fun `attaches to child span on non-Android`() {
+        ApolloPlatformTestManipulator.pretendIsAndroid(false)
+        executeQuery(fixture.getSut())
+        verify(fixture.hub).span
+    }
+
     private fun assertTransactionDetails(it: SentryTransaction) {
         assertEquals(1, it.spans.size)
         val httpClientSpan = it.spans.first()
@@ -250,6 +271,7 @@ class SentryApolloInterceptorTest {
         var tx: ITransaction? = null
         if (isSpanActive) {
             tx = SentryTracer(TransactionContext("op", "desc", TracesSamplingDecision(true)), fixture.hub)
+            whenever(fixture.hub.transaction).thenReturn(tx)
             whenever(fixture.hub.span).thenReturn(tx)
         }
 
diff --git a/sentry-apollo/src/test/java/io/sentry/util/ApolloPlatformTestManipulator.kt b/sentry-apollo/src/test/java/io/sentry/util/ApolloPlatformTestManipulator.kt
@@ -0,0 +1,8 @@
+package io.sentry.util
+
+object ApolloPlatformTestManipulator {
+
+    fun pretendIsAndroid(isAndroid: Boolean) {
+        Platform.isAndroid = isAndroid
+    }
+}
diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api
@@ -364,6 +364,7 @@ public final class io/sentry/Hub : io/sentry/IHub {
 	public fun getOptions ()Lio/sentry/SentryOptions;
 	public fun getSpan ()Lio/sentry/ISpan;
 	public fun getTraceparent ()Lio/sentry/SentryTraceHeader;
+	public fun getTransaction ()Lio/sentry/ITransaction;
 	public fun isCrashedLastRun ()Ljava/lang/Boolean;
 	public fun isEnabled ()Z
 	public fun popScope ()V
@@ -411,6 +412,7 @@ public final class io/sentry/HubAdapter : io/sentry/IHub {
 	public fun getOptions ()Lio/sentry/SentryOptions;
 	public fun getSpan ()Lio/sentry/ISpan;
 	public fun getTraceparent ()Lio/sentry/SentryTraceHeader;
+	public fun getTransaction ()Lio/sentry/ITransaction;
 	public fun isCrashedLastRun ()Ljava/lang/Boolean;
 	public fun isEnabled ()Z
 	public fun popScope ()V
@@ -483,6 +485,7 @@ public abstract interface class io/sentry/IHub {
 	public abstract fun getOptions ()Lio/sentry/SentryOptions;
 	public abstract fun getSpan ()Lio/sentry/ISpan;
 	public abstract fun getTraceparent ()Lio/sentry/SentryTraceHeader;
+	public abstract fun getTransaction ()Lio/sentry/ITransaction;
 	public abstract fun isCrashedLastRun ()Ljava/lang/Boolean;
 	public abstract fun isEnabled ()Z
 	public abstract fun popScope ()V
@@ -869,6 +872,7 @@ public final class io/sentry/NoOpHub : io/sentry/IHub {
 	public fun getOptions ()Lio/sentry/SentryOptions;
 	public fun getSpan ()Lio/sentry/ISpan;
 	public fun getTraceparent ()Lio/sentry/SentryTraceHeader;
+	public fun getTransaction ()Lio/sentry/ITransaction;
 	public fun isCrashedLastRun ()Ljava/lang/Boolean;
 	public fun isEnabled ()Z
 	public fun popScope ()V
diff --git a/sentry/src/main/java/io/sentry/Hub.java b/sentry/src/main/java/io/sentry/Hub.java
@@ -762,6 +762,22 @@ public void flush(long timeoutMillis) {
     return span;
   }
 
+  @Override
+  @ApiStatus.Internal
+  public @Nullable ITransaction getTransaction() {
+    ITransaction span = null;
+    if (!isEnabled()) {
+      options
+          .getLogger()
+          .log(
+              SentryLevel.WARNING,
+              "Instance is disabled and this 'getTransaction' call is a no-op.");
+    } else {
+      span = stack.peek().getScope().getTransaction();
+    }
+    return span;
+  }
+
   @Override
   @ApiStatus.Internal
   public void setSpanContext(
diff --git a/sentry/src/main/java/io/sentry/HubAdapter.java b/sentry/src/main/java/io/sentry/HubAdapter.java
@@ -221,6 +221,12 @@ public void setSpanContext(
     return Sentry.getCurrentHub().getSpan();
   }
 
+  @Override
+  @ApiStatus.Internal
+  public @Nullable ITransaction getTransaction() {
+    return Sentry.getCurrentHub().getTransaction();
+  }
+
   @Override
   public @NotNull SentryOptions getOptions() {
     return Sentry.getCurrentHub().getOptions();
diff --git a/sentry/src/main/java/io/sentry/IHub.java b/sentry/src/main/java/io/sentry/IHub.java
@@ -554,6 +554,15 @@ void setSpanContext(
   @Nullable
   ISpan getSpan();
 
+  /**
+   * Returns the transaction.
+   *
+   * @return the transaction or null when no active transaction is running.
+   */
+  @ApiStatus.Internal
+  @Nullable
+  ITransaction getTransaction();
+
   /**
    * Gets the {@link SentryOptions} attached to current scope.
    *
diff --git a/sentry/src/main/java/io/sentry/NoOpHub.java b/sentry/src/main/java/io/sentry/NoOpHub.java
@@ -179,6 +179,11 @@ public void setSpanContext(
     return null;
   }
 
+  @Override
+  public @Nullable ITransaction getTransaction() {
+    return null;
+  }
+
   @Override
   public @NotNull SentryOptions getOptions() {
     return emptyOptions;
diff --git a/sentry/src/main/java/io/sentry/Sentry.java b/sentry/src/main/java/io/sentry/Sentry.java
@@ -15,6 +15,7 @@
 import io.sentry.transport.NoOpEnvelopeCache;
 import io.sentry.util.DebugMetaPropertiesApplier;
 import io.sentry.util.FileUtils;
+import io.sentry.util.Platform;
 import io.sentry.util.thread.IMainThreadChecker;
 import io.sentry.util.thread.MainThreadChecker;
 import io.sentry.util.thread.NoOpMainThreadChecker;
@@ -918,10 +919,16 @@ public static void endSession() {
   /**
    * Gets the current active transaction or span.
    *
-   * @return the active span or null when no active transaction is running
+   * @return the active span or null when no active transaction is running. In case of
+   *     globalHubMode=true, always the active transaction is returned, rather than the last active
+   *     span.
    */
   public static @Nullable ISpan getSpan() {
-    return getCurrentHub().getSpan();
+    if (globalHubMode && Platform.isAndroid()) {
+      return getCurrentHub().getTransaction();
+    } else {
+      return getCurrentHub().getSpan();
+    }
   }
 
   /**
diff --git a/sentry/src/main/java/io/sentry/instrumentation/file/FileIOSpanManager.java b/sentry/src/main/java/io/sentry/instrumentation/file/FileIOSpanManager.java
@@ -29,7 +29,7 @@ final class FileIOSpanManager {
   private final @NotNull SentryStackTraceFactory stackTraceFactory;
 
   static @Nullable ISpan startSpan(final @NotNull IHub hub, final @NotNull String op) {
-    final ISpan parent = hub.getSpan();
+    final ISpan parent = Platform.isAndroid() ? hub.getTransaction() : hub.getSpan();
     return parent != null ? parent.startChild(op) : null;
   }
 
diff --git a/sentry/src/main/java/io/sentry/util/Platform.java b/sentry/src/main/java/io/sentry/util/Platform.java
@@ -6,7 +6,7 @@
 
 @ApiStatus.Internal
 public final class Platform {
-  private static boolean isAndroid;
+  static boolean isAndroid;
   static boolean isJavaNinePlus;
 
   static {
diff --git a/sentry/src/test/java/io/sentry/HubAdapterTest.kt b/sentry/src/test/java/io/sentry/HubAdapterTest.kt
@@ -226,6 +226,11 @@ class HubAdapterTest {
         verify(hub).span
     }
 
+    @Test fun `getTransaction calls Hub`() {
+        HubAdapter.getInstance().transaction
+        verify(hub).transaction
+    }
+
     @Test fun `getOptions calls Hub`() {
         HubAdapter.getInstance().options
         verify(hub).options
diff --git a/sentry/src/test/java/io/sentry/HubTest.kt b/sentry/src/test/java/io/sentry/HubTest.kt
@@ -1607,15 +1607,32 @@ class HubTest {
     @Test
     fun `when there is no active transaction, getSpan returns null`() {
         val hub = generateHub()
-        assertNull(hub.getSpan())
+        assertNull(hub.span)
     }
 
     @Test
-    fun `when there is active transaction bound to the scope, getSpan returns active transaction`() {
+    fun `when there is no active transaction, getTransaction returns null`() {
+        val hub = generateHub()
+        assertNull(hub.transaction)
+    }
+
+    @Test
+    fun `when there is active transaction bound to the scope, getTransaction and getSpan return active transaction`() {
         val hub = generateHub()
         val tx = hub.startTransaction("aTransaction", "op")
-        hub.configureScope { it.setTransaction(tx) }
-        assertEquals(tx, hub.getSpan())
+        hub.configureScope { it.transaction = tx }
+
+        assertEquals(tx, hub.transaction)
+        assertEquals(tx, hub.span)
+    }
+
+    @Test
+    fun `when there is a transaction but the hub is closed, getTransaction returns null`() {
+        val hub = generateHub()
+        hub.startTransaction("name", "op")
+        hub.close()
+
+        assertNull(hub.transaction)
     }
 
     @Test
@@ -1625,6 +1642,8 @@ class HubTest {
         hub.configureScope { it.setTransaction(tx) }
         hub.configureScope { it.setTransaction(tx) }
         val span = tx.startChild("op")
+
+        assertEquals(tx, hub.transaction)
         assertEquals(span, hub.span)
     }
     // endregion
diff --git a/sentry/src/test/java/io/sentry/SentryTest.kt b/sentry/src/test/java/io/sentry/SentryTest.kt
diff --git a/sentry/src/test/java/io/sentry/instrumentation/file/FileIOSpanManagerTest.kt b/sentry/src/test/java/io/sentry/instrumentation/file/FileIOSpanManagerTest.kt
diff --git a/sentry/src/test/java/io/sentry/util/PlatformTestManipulator.kt b/sentry/src/test/java/io/sentry/util/PlatformTestManipulator.kt

Original file line number	Diff line number	Diff line change
`@@ -44,7 +44,7 @@ class SentryApolloInterceptor(`
`44`	`44`	`}`
`45`	`45`
`46`	`46`	`override fun interceptAsync(request: InterceptorRequest, chain: ApolloInterceptorChain, dispatcher: Executor, callBack: CallBack) {`
`47`		`- val activeSpan = hub.span`
	`47`	`+ val activeSpan = if (io.sentry.util.Platform.isAndroid()) hub.transaction else hub.span`
`48`	`48`	`if (activeSpan == null) {`
`49`	`49`	`val headers = addTracingHeaders(request, null)`
`50`	`50`	`val modifiedRequest = request.toBuilder().requestHeaders(headers).build()`