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

Add deadline timeout for automatic transactions #2865

Merged
merged 8 commits into from
Aug 8, 2023
Merged
Show file tree
Hide file tree
Changes from 2 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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ Breaking changes:
- Reduce flush timeout to 4s on Android to avoid ANRs ([#2858](https://github.com/getsentry/sentry-java/pull/2858))
- Set ip_address to {{auto}} by default, even if sendDefaultPII is disabled ([#2860](https://github.com/getsentry/sentry-java/pull/2860))
- Instead use the "Prevent Storing of IP Addresses" option in the "Security & Privacy" project settings on sentry.io
- Add deadline timeout for automatic transactions ([#2865](https://github.com/getsentry/sentry-java/pull/2865))
- This affects all automatically generated transactions on Android (UI, clicks), the default timeout is 30s

### Fixes

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,8 @@ private void startTracing(final @NotNull Activity activity) {

final TransactionOptions transactionOptions = new TransactionOptions();
if (options.isEnableActivityLifecycleTracingAutoFinish()) {
transactionOptions.setDeadlineTimeout(
TransactionOptions.SENTRY_AUTO_TRANSACTION_DEADLINE_MS);
markushi marked this conversation as resolved.
Show resolved Hide resolved
transactionOptions.setIdleTimeout(options.getIdleTimeout());
transactionOptions.setTrimEnd(true);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,7 @@ private void startTracing(final @NotNull UiElement target, final @NotNull String

final TransactionOptions transactionOptions = new TransactionOptions();
transactionOptions.setWaitForChildren(true);
transactionOptions.setDeadlineTimeout(TransactionOptions.SENTRY_AUTO_TRANSACTION_DEADLINE_MS);
transactionOptions.setIdleTimeout(options.getIdleTimeout());
transactionOptions.setTrimEnd(true);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,7 @@ class ActivityLifecycleIntegrationTest {
}

@Test
fun `Transaction op is ui_load`() {
fun `Transaction op is ui_load and idle+deadline timeouts are set`() {
val sut = fixture.getSut()
fixture.options.tracesSampleRate = 1.0
sut.register(fixture.hub, fixture.options)
Expand All @@ -365,11 +365,14 @@ class ActivityLifecycleIntegrationTest {
sut.onActivityCreated(activity, fixture.bundle)

verify(fixture.hub).startTransaction(
check {
check<TransactionContext> {
assertEquals("ui.load", it.operation)
assertEquals(TransactionNameSource.COMPONENT, it.transactionNameSource)
},
any<TransactionOptions>()
check<TransactionOptions> { transactionOptions ->
assertEquals(fixture.options.idleTimeout, transactionOptions.idleTimeout)
assertEquals(TransactionOptions.SENTRY_AUTO_TRANSACTION_DEADLINE_MS, transactionOptions.deadlineTimeout)
}
)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,24 @@ class SentryGestureListenerTracingTest {
)
}

@Test
fun `captures transaction and both idle+deadline timeouts are set`() {
val sut = fixture.getSut<View>()

sut.onSingleTapUp(fixture.event)

verify(fixture.hub).startTransaction(
any<TransactionContext>(),
check<TransactionOptions> { transactionOptions ->
assertEquals(fixture.options.idleTimeout, transactionOptions.idleTimeout)
assertEquals(
TransactionOptions.SENTRY_AUTO_TRANSACTION_DEADLINE_MS,
transactionOptions.deadlineTimeout
)
}
)
}

@Test
fun `captures transaction with interaction event type as op`() {
val sut = fixture.getSut<View>()
Expand Down
3 changes: 3 additions & 0 deletions sentry/api/sentry.api
Original file line number Diff line number Diff line change
Expand Up @@ -2297,15 +2297,18 @@ public abstract interface class io/sentry/TransactionFinishedCallback {
}

public final class io/sentry/TransactionOptions : io/sentry/SpanOptions {
public static final field SENTRY_AUTO_TRANSACTION_DEADLINE_MS J
public fun <init> ()V
public fun getCustomSamplingContext ()Lio/sentry/CustomSamplingContext;
public fun getDeadlineTimeout ()Ljava/lang/Long;
public fun getIdleTimeout ()Ljava/lang/Long;
public fun getStartTimestamp ()Lio/sentry/SentryDate;
public fun getTransactionFinishedCallback ()Lio/sentry/TransactionFinishedCallback;
public fun isBindToScope ()Z
public fun isWaitForChildren ()Z
public fun setBindToScope (Z)V
public fun setCustomSamplingContext (Lio/sentry/CustomSamplingContext;)V
public fun setDeadlineTimeout (Ljava/lang/Long;)V
public fun setIdleTimeout (Ljava/lang/Long;)V
public fun setStartTimestamp (Lio/sentry/SentryDate;)V
public fun setTransactionFinishedCallback (Lio/sentry/TransactionFinishedCallback;)V
Expand Down
132 changes: 99 additions & 33 deletions sentry/src/main/java/io/sentry/SentryTracer.java
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,14 @@ public final class SentryTracer implements ITransaction {
*/
private @NotNull FinishStatus finishStatus = FinishStatus.NOT_FINISHED;

private volatile @Nullable TimerTask timerTask;
private volatile @Nullable TimerTask idleTimeoutTask;
private volatile @Nullable TimerTask deadlineTimeoutTask;

private volatile @Nullable Timer timer = null;
private final @NotNull Object timerLock = new Object();
private final @NotNull AtomicBoolean isFinishTimerRunning = new AtomicBoolean(false);

private final @NotNull AtomicBoolean isIdleFinishTimerRunning = new AtomicBoolean(false);
private final @NotNull AtomicBoolean isDeadlineTimerRunning = new AtomicBoolean(false);

private final @NotNull Baggage baggage;
private @NotNull TransactionNameSource transactionNameSource;
Expand Down Expand Up @@ -92,7 +96,8 @@ public SentryTracer(
transactionPerformanceCollector.start(this);
}

if (transactionOptions.getIdleTimeout() != null) {
if (transactionOptions.getIdleTimeout() != null
|| transactionOptions.getDeadlineTimeout() != null) {
timer = new Timer(true);
scheduleFinish();
}
Expand All @@ -101,34 +106,71 @@ public SentryTracer(
@Override
public void scheduleFinish() {
synchronized (timerLock) {
cancelTimer();
cancelIdleTimer();
cancelDeadlineTimer();
markushi marked this conversation as resolved.
Show resolved Hide resolved

if (timer != null) {
isFinishTimerRunning.set(true);
timerTask =
new TimerTask() {
@Override
public void run() {
finishFromTimer();
}
};

try {
timer.schedule(timerTask, transactionOptions.getIdleTimeout());
} catch (Throwable e) {
hub.getOptions()
.getLogger()
.log(SentryLevel.WARNING, "Failed to schedule finish timer", e);
// if we failed to schedule the finish timer for some reason, we finish it here right away
finishFromTimer();
final @Nullable Long idleTimeout = transactionOptions.getIdleTimeout();
final @Nullable Long deadlineTimeOut = transactionOptions.getDeadlineTimeout();

if (idleTimeout != null) {
isIdleFinishTimerRunning.set(true);
idleTimeoutTask =
new TimerTask() {
@Override
public void run() {
onIdleTimeoutReached();
}
};

try {
timer.schedule(idleTimeoutTask, idleTimeout);
} catch (Throwable e) {
hub.getOptions()
.getLogger()
.log(SentryLevel.WARNING, "Failed to schedule finish timer", e);
// if we failed to schedule the finish timer for some reason, we finish it here right
// away
onIdleTimeoutReached();
}
}
if (deadlineTimeOut != null) {
isDeadlineTimerRunning.set(true);
deadlineTimeoutTask =
new TimerTask() {
@Override
public void run() {
onDeadlineTimeoutReached();
}
};

try {
timer.schedule(deadlineTimeoutTask, deadlineTimeOut);
} catch (Throwable e) {
hub.getOptions()
.getLogger()
.log(SentryLevel.WARNING, "Failed to schedule finish timer", e);
// if we failed to schedule the finish timer for some reason, we finish it here right
// away
onDeadlineTimeoutReached();
}
}
}
}
}

private void finishFromTimer() {
final SpanStatus status = getStatus();
private void onIdleTimeoutReached() {
final @Nullable SpanStatus status = getStatus();
finish((status != null) ? status : SpanStatus.OK);
isFinishTimerRunning.set(false);
isIdleFinishTimerRunning.set(false);
}

private void onDeadlineTimeoutReached() {
final @Nullable SpanStatus status = getStatus();
forceFinish(
(status != null) ? status : SpanStatus.DEADLINE_EXCEEDED,
transactionOptions.getIdleTimeout() != null);
isDeadlineTimerRunning.set(false);
}

@Override
Expand Down Expand Up @@ -222,6 +264,8 @@ public void finish(
if (timer != null) {
synchronized (timerLock) {
if (timer != null) {
cancelIdleTimer();
cancelDeadlineTimer();
timer.cancel();
timer = null;
}
Expand All @@ -244,12 +288,22 @@ public void finish(
}
}

private void cancelTimer() {
private void cancelIdleTimer() {
synchronized (timerLock) {
if (timerTask != null) {
timerTask.cancel();
isFinishTimerRunning.set(false);
timerTask = null;
if (idleTimeoutTask != null) {
idleTimeoutTask.cancel();
isIdleFinishTimerRunning.set(false);
idleTimeoutTask = null;
}
}
}

private void cancelDeadlineTimer() {
synchronized (timerLock) {
if (deadlineTimeoutTask != null) {
deadlineTimeoutTask.cancel();
isDeadlineTimerRunning.set(false);
deadlineTimeoutTask = null;
}
}
}
Expand Down Expand Up @@ -360,7 +414,7 @@ private ISpan createChild(

Objects.requireNonNull(parentSpanId, "parentSpanId is required");
Objects.requireNonNull(operation, "operation is required");
cancelTimer();
cancelIdleTimer();
final Span span =
new Span(
root.getTraceId(),
Expand Down Expand Up @@ -720,8 +774,14 @@ Span getRoot() {

@TestOnly
@Nullable
TimerTask getTimerTask() {
return timerTask;
TimerTask getIdleTimeoutTask() {
return idleTimeoutTask;
}

@TestOnly
@Nullable
TimerTask getDeadlineTimeoutTask() {
return deadlineTimeoutTask;
}

@TestOnly
Expand All @@ -733,7 +793,13 @@ Timer getTimer() {
@TestOnly
@NotNull
AtomicBoolean isFinishTimerRunning() {
return isFinishTimerRunning;
return isIdleFinishTimerRunning;
}

@TestOnly
@NotNull
AtomicBoolean isDeadlineTimerRunning() {
return isDeadlineTimerRunning;
}

@TestOnly
Expand Down
32 changes: 32 additions & 0 deletions sentry/src/main/java/io/sentry/TransactionOptions.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
/** Sentry Transaction options */
public final class TransactionOptions extends SpanOptions {

public static final long SENTRY_AUTO_TRANSACTION_DEADLINE_MS = 300000;

/**
* Arbitrary data used in {@link SamplingContext} to determine if transaction is going to be
* sampled.
Expand Down Expand Up @@ -34,6 +36,16 @@ public final class TransactionOptions extends SpanOptions {
*/
private @Nullable Long idleTimeout = null;

/**
* The deadline time, measured in ms, to wait until the transaction will be force-finished with
* deadline-exceeded status./
*
* <p>When set to {@code null} the transaction won't be forcefully finished.
*
* <p>The default is 30 seconds.
*/
private @Nullable Long deadlineTimeout = null;

/**
* When `waitForChildren` is set to `true` and this callback is set, it's called before the
* transaction is captured.
Expand Down Expand Up @@ -121,6 +133,26 @@ public void setWaitForChildren(boolean waitForChildren) {
return idleTimeout;
}

/**
* Sets the deadlineTimeout. If set, an transaction and it's child spans will be force-finished
* with status {@link SpanStatus#DEADLINE_EXCEEDED} in case the transaction isn't finished in
* time.
*
* @param deadlineTimeoutMs - the deadlineTimeout, in ms - or null if no deadline should be set
*/
public void setDeadlineTimeout(@Nullable Long deadlineTimeoutMs) {
markushi marked this conversation as resolved.
Show resolved Hide resolved
this.deadlineTimeout = deadlineTimeoutMs;
}

/**
* Gets the deadlineTimeout
*
* @return deadlineTimeout - the deadlineTimeout, in ms - or null if no deadline is set
*/
public @Nullable Long getDeadlineTimeout() {
return deadlineTimeout;
}

/**
* Sets the idleTimeout
*
Expand Down
Loading