From 473c6654576ba30d7c664293b29f5e4214f4d739 Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Thu, 20 Jul 2023 08:50:24 +0200 Subject: [PATCH 1/8] Return transaction for Sentry.getSpan when global hub mode is enabled --- CHANGELOG.md | 1 + sentry/api/sentry.api | 4 ++ sentry/src/main/java/io/sentry/Hub.java | 16 ++++++++ .../src/main/java/io/sentry/HubAdapter.java | 6 +++ sentry/src/main/java/io/sentry/IHub.java | 9 ++++ sentry/src/main/java/io/sentry/NoOpHub.java | 5 +++ sentry/src/main/java/io/sentry/Sentry.java | 10 ++++- sentry/src/test/java/io/sentry/HubTest.kt | 18 ++++++-- sentry/src/test/java/io/sentry/SentryTest.kt | 41 +++++++++++++++++++ 9 files changed, 104 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ec184b6abb..f2a81317d6e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ Breaking changes: - Capture failed HTTP requests by default ([#2794](https://github.com/getsentry/sentry-java/pull/2794)) +- If global hub mode is enabled (default on Android), Sentry.getSpan() returns the root span instead of the latest span ([#2853](https://github.com/getsentry/sentry-java/pull/2853)) ### Fixes diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index 2470987c352..92df404fe8b 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -358,6 +358,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 @@ -405,6 +406,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 @@ -477,6 +479,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 @@ -839,6 +842,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 index 3171c67a95b..90baf3ed87a 100644 --- 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 index e6ec2208746..a33fe4e8885 100644 --- 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 index 97583f66e8c..bf4223576e8 100644 --- 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 index aa5d8469751..6ad44552bb0 100644 --- 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 index 254c1fffd50..882d867d695 100644 --- a/sentry/src/main/java/io/sentry/Sentry.java +++ b/sentry/src/main/java/io/sentry/Sentry.java @@ -918,10 +918,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) { + return getCurrentHub().getTransaction(); + } else { + return getCurrentHub().getSpan(); + } } /** diff --git a/sentry/src/test/java/io/sentry/HubTest.kt b/sentry/src/test/java/io/sentry/HubTest.kt index 96a44462d49..8ec6f8039ee 100644 --- a/sentry/src/test/java/io/sentry/HubTest.kt +++ b/sentry/src/test/java/io/sentry/HubTest.kt @@ -1607,15 +1607,23 @@ 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 @@ -1625,6 +1633,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 index c19063e8c46..ded94cebee4 100644 --- a/sentry/src/test/java/io/sentry/SentryTest.kt +++ b/sentry/src/test/java/io/sentry/SentryTest.kt @@ -723,6 +723,47 @@ class SentryTest { assertFalse(previousSessionFile.exists()) } + @Test + fun `getSpan calls hub getSpan`() { + val hub = mock() + Sentry.init({ + it.dsn = dsn + }, false) + Sentry.setCurrentHub(hub) + Sentry.getSpan() + verify(hub).span + } + + @Test + fun `getSpan calls returns root span if globalhub mode is enabled`() { + Sentry.init({ + it.dsn = dsn + it.enableTracing = true + it.sampleRate = 1.0 + }, true) + + val transaction = Sentry.startTransaction("name", "op-root", true) + transaction.startChild("op-child") + + val span = Sentry.getSpan()!! + assertEquals("op-root", span.operation) + } + + @Test + fun `getSpan calls returns child span if globalhub mode is disabled`() { + Sentry.init({ + it.dsn = dsn + it.enableTracing = true + it.sampleRate = 1.0 + }, false) + + val transaction = Sentry.startTransaction("name", "op-root", true) + transaction.startChild("op-child") + + val span = Sentry.getSpan()!! + assertEquals("op-child", span.operation) + } + private class InMemoryOptionsObserver : IOptionsObserver { var release: String? = null private set From c4df37e7a6b1bd5212f4ad7c503d57f663a4b7b6 Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Thu, 20 Jul 2023 09:13:12 +0200 Subject: [PATCH 2/8] Fix changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4ff40daf695..1c3a5bbcb3d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ Breaking changes: - Capture failed HTTP requests by default ([#2794](https://github.com/getsentry/sentry-java/pull/2794)) -- If global hub mode is enabled (default on Android), Sentry.getSpan() returns the root span instead of the latest span ([#2853](https://github.com/getsentry/sentry-java/pull/2853)) +- If global hub mode is enabled (default on Android), Sentry.getSpan() returns the root span instead of the latest span ([#2855](https://github.com/getsentry/sentry-java/pull/2855)) ### Fixes From c934c6f4c2f4153ccd3644625aee0730a67b412a Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Fri, 21 Jul 2023 07:30:33 +0200 Subject: [PATCH 3/8] Add missing tests --- sentry/src/test/java/io/sentry/HubAdapterTest.kt | 5 +++++ sentry/src/test/java/io/sentry/HubTest.kt | 9 +++++++++ 2 files changed, 14 insertions(+) diff --git a/sentry/src/test/java/io/sentry/HubAdapterTest.kt b/sentry/src/test/java/io/sentry/HubAdapterTest.kt index 6fec20ca7da..f906c5e3c8f 100644 --- 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 index 8ec6f8039ee..da18dd3d82d 100644 --- a/sentry/src/test/java/io/sentry/HubTest.kt +++ b/sentry/src/test/java/io/sentry/HubTest.kt @@ -1626,6 +1626,15 @@ class HubTest { 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 fun `when there is active span within a transaction bound to the scope, getSpan returns active span`() { val hub = generateHub() From 348fb561d85290391f8c22db1bc94ee264d31e01 Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Mon, 7 Aug 2023 13:02:09 +0200 Subject: [PATCH 4/8] Use root span for OkHttp+FileIo integrations --- .../src/main/java/io/sentry/android/okhttp/SentryOkHttpEvent.kt | 2 +- .../java/io/sentry/android/okhttp/SentryOkHttpInterceptor.kt | 2 +- .../java/io/sentry/instrumentation/file/FileIOSpanManager.java | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) 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 index 5727eb7a838..f57f143ee99 100644 --- 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 @@ -37,7 +37,7 @@ 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") + callRootSpan = hub.transaction?.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 index a8e65352129..395a212008d 100644 --- 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 @@ -78,7 +78,7 @@ class SentryOkHttpInterceptor( isFromEventListener = true } else { // read the span from the bound scope - span = hub.span?.startChild("http.client", "$method $url") + span = hub.transaction?.startChild("http.client", "$method $url") isFromEventListener = false } diff --git a/sentry/src/main/java/io/sentry/instrumentation/file/FileIOSpanManager.java b/sentry/src/main/java/io/sentry/instrumentation/file/FileIOSpanManager.java index accee3b0510..9ca837a6aa7 100644 --- 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 = hub.getTransaction(); return parent != null ? parent.startChild(op) : null; } From 909262a95ebf5474cda23973f62f2860257b60e8 Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Fri, 11 Aug 2023 09:04:39 +0200 Subject: [PATCH 5/8] return root span only on Android --- sentry/src/main/java/io/sentry/Sentry.java | 3 ++- .../main/java/io/sentry/util/Platform.java | 2 +- sentry/src/test/java/io/sentry/SentryTest.kt | 21 ++++++++++++++++++- .../io/sentry/util/PlatformTestManipulator.kt | 4 ++++ 4 files changed, 27 insertions(+), 3 deletions(-) diff --git a/sentry/src/main/java/io/sentry/Sentry.java b/sentry/src/main/java/io/sentry/Sentry.java index 882d867d695..a7d7330d3f7 100644 --- 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; @@ -923,7 +924,7 @@ public static void endSession() { * span. */ public static @Nullable ISpan getSpan() { - if (globalHubMode) { + if (globalHubMode && Platform.isAndroid()) { return getCurrentHub().getTransaction(); } else { return getCurrentHub().getSpan(); diff --git a/sentry/src/main/java/io/sentry/util/Platform.java b/sentry/src/main/java/io/sentry/util/Platform.java index 0a5d06cec51..b08b6e584fb 100644 --- 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/SentryTest.kt b/sentry/src/test/java/io/sentry/SentryTest.kt index ded94cebee4..dd9e6fc9756 100644 --- a/sentry/src/test/java/io/sentry/SentryTest.kt +++ b/sentry/src/test/java/io/sentry/SentryTest.kt @@ -9,6 +9,7 @@ import io.sentry.internal.modules.IModulesLoader import io.sentry.protocol.SdkVersion import io.sentry.protocol.SentryId import io.sentry.test.ImmediateExecutorService +import io.sentry.util.PlatformTestManipulator import io.sentry.util.thread.IMainThreadChecker import io.sentry.util.thread.MainThreadChecker import org.awaitility.kotlin.await @@ -735,7 +736,8 @@ class SentryTest { } @Test - fun `getSpan calls returns root span if globalhub mode is enabled`() { + fun `getSpan calls returns root span if globalhub mode is enabled on Android`() { + PlatformTestManipulator.pretendIsAndroid(true) Sentry.init({ it.dsn = dsn it.enableTracing = true @@ -747,6 +749,23 @@ class SentryTest { val span = Sentry.getSpan()!! assertEquals("op-root", span.operation) + PlatformTestManipulator.pretendIsAndroid(false) + } + + @Test + fun `getSpan calls returns child span if globalhub mode is enabled, but the platform is not Android`() { + PlatformTestManipulator.pretendIsAndroid(false) + Sentry.init({ + it.dsn = dsn + it.enableTracing = true + it.sampleRate = 1.0 + }, false) + + val transaction = Sentry.startTransaction("name", "op-root", true) + transaction.startChild("op-child") + + val span = Sentry.getSpan()!! + assertEquals("op-child", span.operation) } @Test diff --git a/sentry/src/test/java/io/sentry/util/PlatformTestManipulator.kt b/sentry/src/test/java/io/sentry/util/PlatformTestManipulator.kt index a849cf3d6dc..3eb4662f7ce 100644 --- a/sentry/src/test/java/io/sentry/util/PlatformTestManipulator.kt +++ b/sentry/src/test/java/io/sentry/util/PlatformTestManipulator.kt @@ -6,5 +6,9 @@ class PlatformTestManipulator { fun pretendJavaNinePlus(isJavaNinePlus: Boolean) { Platform.isJavaNinePlus = isJavaNinePlus } + + fun pretendIsAndroid(isAndroid: Boolean) { + Platform.isAndroid = isAndroid + } } } From a1fc5ac455eeccfa733eb5ec6bc3d9d136724dd9 Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Tue, 29 Aug 2023 09:07:48 +0200 Subject: [PATCH 6/8] Use hub.transaction on Android for FileIO/OkHttp --- .../android/okhttp/SentryOkHttpEvent.kt | 4 +- .../android/okhttp/SentryOkHttpInterceptor.kt | 4 +- .../file/FileIOSpanManager.java | 2 +- .../file/FileIOSpanManagerTest.kt | 37 +++++++++++++++++++ 4 files changed, 44 insertions(+), 3 deletions(-) create mode 100644 sentry/src/test/java/io/sentry/instrumentation/file/FileIOSpanManagerTest.kt 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 index f57f143ee99..eaad6484a67 100644 --- 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.transaction?.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 index 395a212008d..7b2099739d4 100644 --- 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.transaction?.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/src/main/java/io/sentry/instrumentation/file/FileIOSpanManager.java b/sentry/src/main/java/io/sentry/instrumentation/file/FileIOSpanManager.java index 9ca837a6aa7..956996ce04b 100644 --- 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.getTransaction(); + final ISpan parent = Platform.isAndroid() ? hub.getTransaction() : hub.getSpan(); return parent != null ? parent.startChild(op) : null; } diff --git a/sentry/src/test/java/io/sentry/instrumentation/file/FileIOSpanManagerTest.kt b/sentry/src/test/java/io/sentry/instrumentation/file/FileIOSpanManagerTest.kt new file mode 100644 index 00000000000..ea04bc40e58 --- /dev/null +++ b/sentry/src/test/java/io/sentry/instrumentation/file/FileIOSpanManagerTest.kt @@ -0,0 +1,37 @@ +package io.sentry.instrumentation.file + +import io.sentry.IHub +import io.sentry.ISpan +import io.sentry.ITransaction +import io.sentry.util.PlatformTestManipulator +import org.mockito.kotlin.any +import org.mockito.kotlin.mock +import org.mockito.kotlin.verify +import org.mockito.kotlin.whenever +import kotlin.test.Test + +class FileIOSpanManagerTest { + @Test + fun `startSpan uses transaction on Android platform`() { + val hub = mock() + val transaction = mock() + whenever(hub.transaction).thenReturn(transaction) + + PlatformTestManipulator.pretendIsAndroid(true) + + FileIOSpanManager.startSpan(hub, "op.read") + verify(transaction).startChild(any()) + } + + @Test + fun `startSpan uses last span on non-Android platforms`() { + val hub = mock() + val span = mock() + whenever(hub.span).thenReturn(span) + + PlatformTestManipulator.pretendIsAndroid(false) + + FileIOSpanManager.startSpan(hub, "op.read") + verify(span).startChild(any()) + } +} From aa6e1afe2df01432c1e54bc453396c281d38c226 Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Wed, 30 Aug 2023 09:50:27 +0200 Subject: [PATCH 7/8] Fix flaky tests --- .../sentry/instrumentation/file/FileIOSpanManagerTest.kt | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/sentry/src/test/java/io/sentry/instrumentation/file/FileIOSpanManagerTest.kt b/sentry/src/test/java/io/sentry/instrumentation/file/FileIOSpanManagerTest.kt index ea04bc40e58..00c89f27bea 100644 --- a/sentry/src/test/java/io/sentry/instrumentation/file/FileIOSpanManagerTest.kt +++ b/sentry/src/test/java/io/sentry/instrumentation/file/FileIOSpanManagerTest.kt @@ -4,6 +4,7 @@ import io.sentry.IHub import io.sentry.ISpan import io.sentry.ITransaction import io.sentry.util.PlatformTestManipulator +import org.junit.After import org.mockito.kotlin.any import org.mockito.kotlin.mock import org.mockito.kotlin.verify @@ -11,6 +12,12 @@ import org.mockito.kotlin.whenever import kotlin.test.Test class FileIOSpanManagerTest { + + @After + fun cleanup() { + PlatformTestManipulator.pretendIsAndroid(false) + } + @Test fun `startSpan uses transaction on Android platform`() { val hub = mock() From a1914e9cf14d25a71da2a9a702863b317739f850 Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Wed, 30 Aug 2023 12:10:29 +0200 Subject: [PATCH 8/8] Use root transaction for Apollo spans on Android --- .../apollo3/SentryApollo3HttpInterceptor.kt | 3 ++- .../apollo3/SentryApollo3InterceptorTest.kt | 22 +++++++++++++++++++ .../util/Apollo3PlatformTestManipulator.kt | 8 +++++++ .../sentry/apollo/SentryApolloInterceptor.kt | 2 +- .../apollo/SentryApolloInterceptorTest.kt | 22 +++++++++++++++++++ .../util/ApolloPlatformTestManipulator.kt | 8 +++++++ 6 files changed, 63 insertions(+), 2 deletions(-) create mode 100644 sentry-apollo-3/src/test/java/io/sentry/util/Apollo3PlatformTestManipulator.kt create mode 100644 sentry-apollo/src/test/java/io/sentry/util/ApolloPlatformTestManipulator.kt 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 index 906af7c8c67..1a8eb4213b5 100644 --- 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 index d197eeae4a0..1c65d07ee95 100644 --- 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 new file mode 100644 index 00000000000..b639cab40e4 --- /dev/null +++ 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 index 9cdd005a956..b2d9b4943e8 100644 --- 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 index a68edab92b7..7ca5d1b82f4 100644 --- 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 new file mode 100644 index 00000000000..219b95d08a0 --- /dev/null +++ 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 + } +}