diff --git a/CHANGELOG.md b/CHANGELOG.md index 98ba6a48a0..5e605b1ee0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,6 +47,10 @@ - Remove deprecated OpenTelemetry integration classes and the legacy `SentrySpanStorage` ([#6243](https://github.com/getsentry/sentry-java/pull/6243)) - Decide whether foregrounding the app starts a new session on a monotonic clock instead of the wall clock, so that a device time change no longer starts a session that should have been resumed, or resumes one that should have ended. This affects session counts ([#6096](https://github.com/getsentry/sentry-java/pull/6096)) +### Fixes + +- Prevent changes to reused tracing options from affecting spans and transactions after creation ([#6417](https://github.com/getsentry/sentry-java/pull/6417)) + ## 8.62.0 ### Improvements diff --git a/sentry-opentelemetry/sentry-opentelemetry-core/src/main/java/io/sentry/opentelemetry/OtelSpanWrapper.java b/sentry-opentelemetry/sentry-opentelemetry-core/src/main/java/io/sentry/opentelemetry/OtelSpanWrapper.java index df9f7d8583..ec560066db 100644 --- a/sentry-opentelemetry/sentry-opentelemetry-core/src/main/java/io/sentry/opentelemetry/OtelSpanWrapper.java +++ b/sentry-opentelemetry/sentry-opentelemetry-core/src/main/java/io/sentry/opentelemetry/OtelSpanWrapper.java @@ -152,11 +152,9 @@ public OtelSpanWrapper( @Nullable String description, @Nullable SentryDate timestamp, @NotNull SpanOptions spanOptions) { - if (timestamp != null) { - spanOptions.setStartTimestamp(timestamp); - } - - return startChild(operation, description, spanOptions); + final @NotNull SpanOptions spanOptionsSnapshot = new SpanOptions(spanOptions); + spanOptionsSnapshot.setStartTimestamp(timestamp); + return startChild(operation, description, spanOptionsSnapshot); } @Override diff --git a/sentry-opentelemetry/sentry-opentelemetry-core/src/test/kotlin/OtelSpanWrapperTest.kt b/sentry-opentelemetry/sentry-opentelemetry-core/src/test/kotlin/OtelSpanWrapperTest.kt index 1b5ed360d9..8432aeff9e 100644 --- a/sentry-opentelemetry/sentry-opentelemetry-core/src/test/kotlin/OtelSpanWrapperTest.kt +++ b/sentry-opentelemetry/sentry-opentelemetry-core/src/test/kotlin/OtelSpanWrapperTest.kt @@ -16,12 +16,13 @@ import org.mockito.kotlin.any import org.mockito.kotlin.argumentCaptor import org.mockito.kotlin.eq import org.mockito.kotlin.mock +import org.mockito.kotlin.times import org.mockito.kotlin.verify import org.mockito.kotlin.whenever class OtelSpanWrapperTest { @Test - fun `startChild with timestamp and options forwards both`() { + fun `startChild with timestamp and options applies timestamp to copy`() { val otelSpan = mock() whenever(otelSpan.spanContext) .thenReturn( @@ -42,19 +43,28 @@ class OtelSpanWrapperTest { val parent = OtelSpanWrapper(otelSpan, scopes, SentryLongDate(0), null, null, null, null) val expectedChild = mock() whenever(spanFactory.createSpan(eq(scopes), any(), any(), eq(parent))).thenReturn(expectedChild) - val timestamp = SentryLongDate(1234) - val suppliedSpanOptions = SpanOptions() + val originalTimestamp = SentryLongDate(1234) + val timestamp = SentryLongDate(5678) + val suppliedSpanOptions = SpanOptions().apply { startTimestamp = originalTimestamp } val child = parent.startChild("child-op", "description", timestamp, suppliedSpanOptions) + val childWithoutTimestamp = + parent.startChild("second-child-op", "second-description", null, suppliedSpanOptions) val spanOptions = argumentCaptor() val spanContext = argumentCaptor() - verify(spanFactory) + verify(spanFactory, times(2)) .createSpan(eq(scopes), spanOptions.capture(), spanContext.capture(), eq(parent)) assertThat(child).isSameInstanceAs(expectedChild) - assertThat(spanOptions.firstValue).isSameInstanceAs(suppliedSpanOptions) - assertThat(spanOptions.firstValue.startTimestamp).isSameInstanceAs(timestamp) - assertThat(spanContext.firstValue.operation).isEqualTo("child-op") - assertThat(spanContext.firstValue.description).isEqualTo("description") + assertThat(childWithoutTimestamp).isSameInstanceAs(expectedChild) + assertThat(spanOptions.allValues[0]).isNotSameInstanceAs(suppliedSpanOptions) + assertThat(spanOptions.allValues[0].startTimestamp).isSameInstanceAs(timestamp) + assertThat(spanOptions.allValues[1]).isNotSameInstanceAs(suppliedSpanOptions) + assertThat(spanOptions.allValues[1].startTimestamp).isNull() + assertThat(suppliedSpanOptions.startTimestamp).isSameInstanceAs(originalTimestamp) + assertThat(spanContext.allValues[0].operation).isEqualTo("child-op") + assertThat(spanContext.allValues[0].description).isEqualTo("description") + assertThat(spanContext.allValues[1].operation).isEqualTo("second-child-op") + assertThat(spanContext.allValues[1].description).isEqualTo("second-description") } } diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index 9affbccd01..2cb4ba5ac5 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -4371,6 +4371,7 @@ public final class io/sentry/SpanId$Deserializer : io/sentry/JsonDeserializer { public class io/sentry/SpanOptions { protected field origin Ljava/lang/String; public fun ()V + public fun (Lio/sentry/SpanOptions;)V public fun getOrigin ()Ljava/lang/String; public fun getScopeBindingMode ()Lio/sentry/ScopeBindingMode; public fun getStartTimestamp ()Lio/sentry/SentryDate; diff --git a/sentry/src/main/java/io/sentry/Scopes.java b/sentry/src/main/java/io/sentry/Scopes.java index 3af7268d7c..bcef21553d 100644 --- a/sentry/src/main/java/io/sentry/Scopes.java +++ b/sentry/src/main/java/io/sentry/Scopes.java @@ -867,7 +867,7 @@ public void flush(long timeoutMillis) { public @NotNull ITransaction startTransaction( final @NotNull TransactionContext transactionContext, final @NotNull TransactionOptions transactionOptions) { - return createTransaction(transactionContext, transactionOptions); + return createTransaction(transactionContext, new TransactionOptions(transactionOptions)); } private @NotNull ITransaction createTransaction( diff --git a/sentry/src/main/java/io/sentry/SentryTracer.java b/sentry/src/main/java/io/sentry/SentryTracer.java index 85c6532a71..eaa0553774 100644 --- a/sentry/src/main/java/io/sentry/SentryTracer.java +++ b/sentry/src/main/java/io/sentry/SentryTracer.java @@ -52,7 +52,10 @@ public final class SentryTracer implements ITransaction { private @NotNull TransactionNameSource transactionNameSource; private final @NotNull Contexts contexts = new Contexts(); private final @Nullable CompositePerformanceCollector compositePerformanceCollector; - private final @NotNull TransactionOptions transactionOptions; + private final boolean waitForChildren; + private final @Nullable Long idleTimeout; + private final @Nullable Long deadlineTimeout; + private final @Nullable TransactionFinishedCallback transactionFinishedCallback; public SentryTracer(final @NotNull TransactionContext context, final @NotNull IScopes scopes) { this(context, scopes, new TransactionOptions(), null); @@ -81,7 +84,10 @@ public SentryTracer( this.compositePerformanceCollector = Boolean.TRUE.equals(isSampled()) ? compositePerformanceCollector : null; this.transactionNameSource = context.getTransactionNameSource(); - this.transactionOptions = transactionOptions; + this.waitForChildren = transactionOptions.isWaitForChildren(); + this.idleTimeout = transactionOptions.getIdleTimeout(); + this.deadlineTimeout = transactionOptions.getDeadlineTimeout(); + this.transactionFinishedCallback = transactionOptions.getTransactionFinishedCallback(); setDefaultSpanData(root); @@ -97,8 +103,7 @@ public SentryTracer( this.compositePerformanceCollector.start(this); } - if (transactionOptions.getIdleTimeout() != null - || transactionOptions.getDeadlineTimeout() != null) { + if (idleTimeout != null || deadlineTimeout != null) { timersEnabled = true; scheduleDeadlineTimeout(); @@ -110,8 +115,6 @@ public SentryTracer( public void scheduleFinish() { try (final @NotNull ISentryLifecycleToken ignored = timerLock.acquire()) { if (timersEnabled) { - final @Nullable Long idleTimeout = transactionOptions.getIdleTimeout(); - if (idleTimeout != null) { cancelIdleTimer(); isIdleFinishTimerRunning.set(true); @@ -145,9 +148,7 @@ private void onIdleTimeoutReached() { private void onDeadlineTimeoutReached() { final @Nullable SpanStatus status = getStatus(); forceFinish( - (status != null) ? status : SpanStatus.DEADLINE_EXCEEDED, - transactionOptions.getIdleTimeout() != null, - null); + (status != null) ? status : SpanStatus.DEADLINE_EXCEEDED, idleTimeout != null, null); isDeadlineTimerRunning.set(false); } @@ -195,14 +196,13 @@ public void finish( // auto-finish any idle spans first for (final Span span : children) { - if (span.getOptions().isIdle()) { + if (span.isIdle()) { span.finish((status != null) ? status : getSpanContext().status, finishTimestamp); } } this.finishStatus = FinishStatus.finishing(status); - if (!root.isFinished() - && (!transactionOptions.isWaitForChildren() || hasAllChildrenFinished())) { + if (!root.isFinished() && (!waitForChildren || hasAllChildrenFinished())) { final @NotNull AtomicReference> performanceCollectionData = new AtomicReference<>(); @@ -218,7 +218,7 @@ public void finish( // Let's call the finishCallback here, when the root span has a finished date but it's // not finished, yet final @Nullable TransactionFinishedCallback finishedCallback = - transactionOptions.getTransactionFinishedCallback(); + transactionFinishedCallback; if (finishedCallback != null) { finishedCallback.execute(this); } @@ -274,7 +274,7 @@ public void finish( } } - if (dropIfNoChildren && children.isEmpty() && transactionOptions.getIdleTimeout() != null) { + if (dropIfNoChildren && children.isEmpty() && idleTimeout != null) { // if it's an idle transaction which has no children, we drop it to save user's quota scopes .getOptions() @@ -302,7 +302,7 @@ private void cancelIdleTimer() { } private void scheduleDeadlineTimeout() { - final @Nullable Long deadlineTimeOut = transactionOptions.getDeadlineTimeout(); + final @Nullable Long deadlineTimeOut = deadlineTimeout; if (deadlineTimeOut != null) { try (final @NotNull ISentryLifecycleToken ignored = timerLock.acquire()) { if (timersEnabled) { @@ -393,8 +393,9 @@ ISpan startChild( final @Nullable String description, final @Nullable SentryDate timestamp, final @NotNull SpanOptions spanOptions) { - spanOptions.setStartTimestamp(timestamp); - return createChild(parentSpanId, operation, description, spanOptions); + final @NotNull SpanOptions spanOptionsSnapshot = new SpanOptions(spanOptions); + spanOptionsSnapshot.setStartTimestamp(timestamp); + return createChild(parentSpanId, operation, description, spanOptionsSnapshot); } /** @@ -449,11 +450,11 @@ private ISpan createChild( compositePerformanceCollector.onSpanFinished(finishingSpan); } final FinishStatus finishStatus = this.finishStatus; - if (transactionOptions.getIdleTimeout() != null) { + if (idleTimeout != null) { // if it's an idle transaction, no matter the status, we'll reset the timeout here // so the transaction will either idle and finish itself, or a new child will be // added and we'll wait for it again - if (!transactionOptions.isWaitForChildren() || hasAllChildrenFinished()) { + if (!waitForChildren || hasAllChildrenFinished()) { scheduleFinish(); } } else if (finishStatus.isFinishing) { @@ -475,13 +476,13 @@ private ISpan createChild( // compositePerformanceCollector.onSpanFinished(finishingSpan); // } // final FinishStatus finishStatus = this.finishStatus; - // if (transactionOptions.getIdleTimeout() != null) { + // if (idleTimeout != null) { // // if it's an idle transaction, no matter the status, we'll reset the // timeout here // // so the transaction will either idle and finish itself, or a new child // will be // // added and we'll wait for it again - // if (!transactionOptions.isWaitForChildren() || hasAllChildrenFinished()) { + // if (!waitForChildren || hasAllChildrenFinished()) { // scheduleFinish(); // } // } else if (finishStatus.isFinishing) { diff --git a/sentry/src/main/java/io/sentry/Span.java b/sentry/src/main/java/io/sentry/Span.java index e9b1f5987c..e181653f8f 100644 --- a/sentry/src/main/java/io/sentry/Span.java +++ b/sentry/src/main/java/io/sentry/Span.java @@ -40,7 +40,9 @@ public final class Span implements ISpan { private final @NotNull AtomicBoolean isFinishing = new AtomicBoolean(false); - private final @NotNull SpanOptions options; + private final boolean trimStart; + private final boolean trimEnd; + private final boolean idle; private @Nullable SpanFinishedCallback spanFinishedCallback; @@ -59,7 +61,9 @@ public final class Span implements ISpan { this.context.setOrigin(options.getOrigin()); this.transaction = Objects.requireNonNull(transaction, "transaction is required"); this.scopes = Objects.requireNonNull(scopes, "Scopes are required"); - this.options = options; + this.trimStart = options.isTrimStart(); + this.trimEnd = options.isTrimEnd(); + this.idle = options.isIdle(); this.spanFinishedCallback = spanFinishedCallback; final @Nullable SentryDate startTimestamp = options.getStartTimestamp(); if (startTimestamp != null) { @@ -78,6 +82,9 @@ public Span( this.context.setOrigin(options.getOrigin()); this.transaction = Objects.requireNonNull(sentryTracer, "sentryTracer is required"); this.scopes = Objects.requireNonNull(scopes, "scopes are required"); + this.trimStart = options.isTrimStart(); + this.trimEnd = options.isTrimEnd(); + this.idle = options.isIdle(); this.spanFinishedCallback = null; final @Nullable SentryDate startTimestamp = options.getStartTimestamp(); if (startTimestamp != null) { @@ -85,7 +92,6 @@ public Span( } else { this.startTimestamp = scopes.getOptions().getDateProvider().now(); } - this.options = options; } @Override @@ -188,7 +194,7 @@ public void finish(final @Nullable SpanStatus status, final @Nullable SentryDate this.context.setStatus(status); this.timestamp = timestamp == null ? scopes.getOptions().getDateProvider().now() : timestamp; - if (options.isTrimStart() || options.isTrimEnd()) { + if (trimStart || trimEnd) { @Nullable SentryDate minChildStart = null; @Nullable SentryDate maxChildEnd = null; @@ -207,12 +213,10 @@ public void finish(final @Nullable SpanStatus status, final @Nullable SentryDate maxChildEnd = child.getFinishDate(); } } - if (options.isTrimStart() - && minChildStart != null - && startTimestamp.isBefore(minChildStart)) { + if (trimStart && minChildStart != null && startTimestamp.isBefore(minChildStart)) { updateStartDate(minChildStart); } - if (options.isTrimEnd() + if (trimEnd && maxChildEnd != null && (this.timestamp == null || this.timestamp.isAfter(maxChildEnd))) { updateEndDate(maxChildEnd); @@ -431,9 +435,8 @@ private void updateStartDate(@NotNull SentryDate date) { this.startTimestamp = date; } - @NotNull - SpanOptions getOptions() { - return options; + boolean isIdle() { + return idle; } @NotNull diff --git a/sentry/src/main/java/io/sentry/SpanOptions.java b/sentry/src/main/java/io/sentry/SpanOptions.java index 69f93e00a9..5e134ed70a 100644 --- a/sentry/src/main/java/io/sentry/SpanOptions.java +++ b/sentry/src/main/java/io/sentry/SpanOptions.java @@ -3,12 +3,25 @@ import static io.sentry.SpanContext.DEFAULT_ORIGIN; import com.jakewharton.nopen.annotation.Open; +import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @Open public class SpanOptions { + public SpanOptions() {} + + @ApiStatus.Internal + public SpanOptions(final @NotNull SpanOptions options) { + startTimestamp = options.getStartTimestamp(); + scopeBindingMode = options.getScopeBindingMode(); + trimStart = options.isTrimStart(); + trimEnd = options.isTrimEnd(); + isIdle = options.isIdle(); + origin = options.getOrigin(); + } + /** The start timestamp of the transaction */ private @Nullable SentryDate startTimestamp = null; diff --git a/sentry/src/main/java/io/sentry/TransactionOptions.java b/sentry/src/main/java/io/sentry/TransactionOptions.java index 0da3cf611c..d56c3fb58c 100644 --- a/sentry/src/main/java/io/sentry/TransactionOptions.java +++ b/sentry/src/main/java/io/sentry/TransactionOptions.java @@ -9,6 +9,19 @@ public final class TransactionOptions extends SpanOptions { @ApiStatus.Internal public static final long DEFAULT_DEADLINE_TIMEOUT_AUTO_TRANSACTION = 30000; + public TransactionOptions() {} + + TransactionOptions(final @NotNull TransactionOptions options) { + super(options); + customSamplingContext = options.getCustomSamplingContext(); + isAppStartTransaction = options.isAppStartTransaction(); + waitForChildren = options.isWaitForChildren(); + idleTimeout = options.getIdleTimeout(); + deadlineTimeout = options.getDeadlineTimeout(); + transactionFinishedCallback = options.getTransactionFinishedCallback(); + spanFactory = options.getSpanFactory(); + } + /** * Arbitrary data used in {@link SamplingContext} to determine if transaction is going to be * sampled. diff --git a/sentry/src/test/java/io/sentry/ScopesTest.kt b/sentry/src/test/java/io/sentry/ScopesTest.kt index cdf9919a27..771a0da4fb 100644 --- a/sentry/src/test/java/io/sentry/ScopesTest.kt +++ b/sentry/src/test/java/io/sentry/ScopesTest.kt @@ -1870,6 +1870,28 @@ class ScopesTest { assertEquals(contexts, transaction.root.spanContext) } + @Test + fun `when startTransaction, passes a snapshot of transaction options to span factory`() { + val scopes = generateScopes() + val spanFactory = mock() + val transaction = mock() + whenever(spanFactory.createTransaction(any(), any(), any(), anyOrNull())) + .thenReturn(transaction) + val transactionOptions = + TransactionOptions().apply { + isWaitForChildren = true + setSpanFactory(spanFactory) + } + + scopes.startTransaction(TransactionContext("name", "op"), transactionOptions) + transactionOptions.isWaitForChildren = false + + val optionsCaptor = argumentCaptor() + verify(spanFactory).createTransaction(any(), eq(scopes), optionsCaptor.capture(), anyOrNull()) + assertThat(optionsCaptor.firstValue).isNotSameInstanceAs(transactionOptions) + assertThat(optionsCaptor.firstValue.isWaitForChildren).isTrue() + } + @Test fun `when startTransaction with bindToScope set to false, transaction is not attached to the scope`() { val scopes = generateScopes() diff --git a/sentry/src/test/java/io/sentry/SentryTracerTest.kt b/sentry/src/test/java/io/sentry/SentryTracerTest.kt index 95f1934424..c57e3d56bf 100644 --- a/sentry/src/test/java/io/sentry/SentryTracerTest.kt +++ b/sentry/src/test/java/io/sentry/SentryTracerTest.kt @@ -90,6 +90,79 @@ class SentryTracerTest { assertEquals("new-origin", transaction.spanContext.origin) } + @Test + fun `transaction options copy snapshots all values`() { + val startTimestamp = SentryLongDate(1234) + val customSamplingContext = CustomSamplingContext() + val finishedCallback = mock() + val spanFactory = mock() + val original = + TransactionOptions().apply { + this.startTimestamp = startTimestamp + scopeBindingMode = ScopeBindingMode.ON + isTrimStart = true + isTrimEnd = true + isIdle = true + origin = "origin" + this.customSamplingContext = customSamplingContext + isAppStartTransaction = true + isWaitForChildren = true + idleTimeout = 1000 + deadlineTimeout = 2000 + transactionFinishedCallback = finishedCallback + setSpanFactory(spanFactory) + } + + val copy = TransactionOptions(original) + + assertThat(copy.startTimestamp).isSameInstanceAs(startTimestamp) + assertThat(copy.scopeBindingMode).isEqualTo(ScopeBindingMode.ON) + assertThat(copy.isTrimStart).isTrue() + assertThat(copy.isTrimEnd).isTrue() + assertThat(copy.isIdle).isTrue() + assertThat(copy.origin).isEqualTo("origin") + assertThat(copy.customSamplingContext).isSameInstanceAs(customSamplingContext) + assertThat(copy.isAppStartTransaction).isTrue() + assertThat(copy.isWaitForChildren).isTrue() + assertThat(copy.idleTimeout).isEqualTo(1000) + assertThat(copy.deadlineTimeout).isEqualTo(2000) + assertThat(copy.transactionFinishedCallback).isSameInstanceAs(finishedCallback) + assertThat(copy.spanFactory).isSameInstanceAs(spanFactory) + } + + @Test + fun `modifying transaction options does not affect running transaction`() { + val originalCallback = mock() + val replacementCallback = mock() + val transactionOptions = + TransactionOptions().apply { + isWaitForChildren = true + transactionFinishedCallback = originalCallback + } + val transaction = + SentryTracer( + TransactionContext("name", "op", TracesSamplingDecision(true)), + fixture.scopes, + transactionOptions, + null, + ) + val child = transaction.startChild("child") + + transactionOptions.isWaitForChildren = false + transactionOptions.transactionFinishedCallback = replacementCallback + transaction.finish() + + assertThat(transaction.isFinished).isFalse() + verify(originalCallback, never()).execute(any()) + verify(replacementCallback, never()).execute(any()) + + child.finish() + + assertThat(transaction.isFinished).isTrue() + verify(originalCallback).execute(transaction) + verify(replacementCallback, never()).execute(any()) + } + @Test fun `root span has thread name and thread id in the data`() { val tracer = fixture.getSut() diff --git a/sentry/src/test/java/io/sentry/SpanTest.kt b/sentry/src/test/java/io/sentry/SpanTest.kt index 9c4491d2de..519300734b 100644 --- a/sentry/src/test/java/io/sentry/SpanTest.kt +++ b/sentry/src/test/java/io/sentry/SpanTest.kt @@ -108,6 +108,50 @@ class SpanTest { assertThat(child.startDate).isSameInstanceAs(timestamp) } + @Test + fun `starting a child with timestamp does not modify supplied options`() { + val parent: ISpan = fixture.getSut() + val originalTimestamp = SentryLongDate(1234) + val timestamp = SentryLongDate(5678) + val spanOptions = SpanOptions().apply { startTimestamp = originalTimestamp } + + val child = parent.startChild("child-op", "description", timestamp, spanOptions) as Span + + assertThat(child.startDate).isSameInstanceAs(timestamp) + assertThat(spanOptions.startTimestamp).isSameInstanceAs(originalTimestamp) + } + + @Test + fun `modifying options after span creation does not change trimming behavior`() { + val originalStart = SentryLongDate(1000) + val childStart = SentryLongDate(2000) + val childEnd = SentryLongDate(3000) + val originalEnd = SentryLongDate(4000) + val spanOptions = SpanOptions().apply { startTimestamp = originalStart } + val span = fixture.getSut(spanOptions) + val child = span.startChild("child-op", "description", childStart) as Span + child.finish(SpanStatus.OK, childEnd) + + spanOptions.isTrimStart = true + spanOptions.isTrimEnd = true + span.finish(SpanStatus.OK, originalEnd) + + assertThat(span.startDate).isSameInstanceAs(originalStart) + assertThat(span.finishDate).isSameInstanceAs(originalEnd) + } + + @Test + fun `modifying options after child creation does not make child idle`() { + val transaction = getTransaction() + val spanOptions = SpanOptions() + val child = transaction.startChild("child-op", "description", spanOptions) + + spanOptions.isIdle = true + transaction.finish() + + assertThat(child.isFinished).isFalse() + } + @Test fun `converts to Sentry trace header`() { val traceId = SentryId()