Skip to content
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

Return transaction for Sentry.getSpan when global hub mode is enabled #2855

Merged
merged 14 commits into from
Aug 31, 2023
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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: 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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
4 changes: 4 additions & 0 deletions sentry/api/sentry.api
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
16 changes: 16 additions & 0 deletions sentry/src/main/java/io/sentry/Hub.java
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
6 changes: 6 additions & 0 deletions sentry/src/main/java/io/sentry/HubAdapter.java
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
9 changes: 9 additions & 0 deletions sentry/src/main/java/io/sentry/IHub.java
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down
5 changes: 5 additions & 0 deletions sentry/src/main/java/io/sentry/NoOpHub.java
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,11 @@ public void setSpanContext(
return null;
}

@Override
public @Nullable ITransaction getTransaction() {
return null;
}

@Override
public @NotNull SentryOptions getOptions() {
return emptyOptions;
Expand Down
11 changes: 9 additions & 2 deletions sentry/src/main/java/io/sentry/Sentry.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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()) {
romtsn marked this conversation as resolved.
Show resolved Hide resolved
return getCurrentHub().getTransaction();
} else {
return getCurrentHub().getSpan();
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
2 changes: 1 addition & 1 deletion sentry/src/main/java/io/sentry/util/Platform.java
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

@ApiStatus.Internal
public final class Platform {
private static boolean isAndroid;
static boolean isAndroid;
static boolean isJavaNinePlus;

static {
Expand Down
5 changes: 5 additions & 0 deletions sentry/src/test/java/io/sentry/HubAdapterTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
27 changes: 23 additions & 4 deletions sentry/src/test/java/io/sentry/HubTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
60 changes: 60 additions & 0 deletions sentry/src/test/java/io/sentry/SentryTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -723,6 +724,65 @@ class SentryTest {
assertFalse(previousSessionFile.exists())
}

@Test
fun `getSpan calls hub getSpan`() {
val hub = mock<IHub>()
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 on Android`() {
PlatformTestManipulator.pretendIsAndroid(true)
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)
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
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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,9 @@ class PlatformTestManipulator {
fun pretendJavaNinePlus(isJavaNinePlus: Boolean) {
Platform.isJavaNinePlus = isJavaNinePlus
}

fun pretendIsAndroid(isAndroid: Boolean) {
Platform.isAndroid = isAndroid
}
}
}