diff --git a/CHANGELOG.md b/CHANGELOG.md index 9eb80a6a29..13fbfa748a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### Breaking Changes +- Default trace context status to `ok` when an active span has no explicit status ([#6418](https://github.com/getsentry/sentry-java/pull/6418)) - Remove the deprecated user feedback APIs ([#6337](https://github.com/getsentry/sentry-java/pull/6337)) - `UserFeedback` and `Sentry.captureUserFeedback(...)`: use `Sentry.feedback().capture(...)` with `io.sentry.protocol.Feedback` instead - `Sentry.captureFeedback(...)`: use `Sentry.feedback().capture(...)` instead diff --git a/sentry/src/main/java/io/sentry/Scope.java b/sentry/src/main/java/io/sentry/Scope.java index ec98cc5fca..61e6826421 100644 --- a/sentry/src/main/java/io/sentry/Scope.java +++ b/sentry/src/main/java/io/sentry/Scope.java @@ -300,7 +300,7 @@ public void setTransaction(final @Nullable ITransaction transaction) { for (final IScopeObserver observer : options.getScopeObservers()) { if (transaction != null) { observer.setTransaction(transaction.getName()); - observer.setTrace(transaction.getSpanContext(), this); + observer.setTrace(transaction.getSpanContext().withDefaultStatus(), this); } else { observer.setTransaction(null); observer.setTrace(null, this); @@ -1246,7 +1246,7 @@ public void assignTraceContext(final @NotNull SentryEvent event) { if (event.getContexts().getTrace() == null && spanWeakRef != null) { final ISpan span = spanWeakRef.get(); if (span != null) { - event.getContexts().setTrace(span.getSpanContext()); + event.getContexts().setTrace(span.getSpanContext().withDefaultStatus()); } } final String transactionName = pair.getSecond(); diff --git a/sentry/src/main/java/io/sentry/SentryClient.java b/sentry/src/main/java/io/sentry/SentryClient.java index 9c019df1f5..1b8bef31ac 100644 --- a/sentry/src/main/java/io/sentry/SentryClient.java +++ b/sentry/src/main/java/io/sentry/SentryClient.java @@ -1442,7 +1442,7 @@ public void captureBatchedMetricsEvents(final @NotNull SentryMetricsEvents metri .getContexts() .setTrace(TransactionContext.fromPropagationContext(scope.getPropagationContext())); } else { - event.getContexts().setTrace(span.getSpanContext()); + event.getContexts().setTrace(span.getSpanContext().withDefaultStatus()); } } @@ -1487,7 +1487,7 @@ public void captureBatchedMetricsEvents(final @NotNull SentryMetricsEvents metri .getContexts() .setTrace(TransactionContext.fromPropagationContext(scope.getPropagationContext())); } else { - event.getContexts().setTrace(span.getSpanContext()); + event.getContexts().setTrace(span.getSpanContext().withDefaultStatus()); } } @@ -1505,7 +1505,7 @@ public void captureBatchedMetricsEvents(final @NotNull SentryMetricsEvents metri .getContexts() .setTrace(TransactionContext.fromPropagationContext(scope.getPropagationContext())); } else { - checkIn.getContexts().setTrace(span.getSpanContext()); + checkIn.getContexts().setTrace(span.getSpanContext().withDefaultStatus()); } } } @@ -1546,7 +1546,7 @@ public void captureBatchedMetricsEvents(final @NotNull SentryMetricsEvents metri .getContexts() .setTrace(TransactionContext.fromPropagationContext(scope.getPropagationContext())); } else { - replayEvent.getContexts().setTrace(span.getSpanContext()); + replayEvent.getContexts().setTrace(span.getSpanContext().withDefaultStatus()); } } } diff --git a/sentry/src/main/java/io/sentry/SpanContext.java b/sentry/src/main/java/io/sentry/SpanContext.java index e260062147..53f3d57f4e 100644 --- a/sentry/src/main/java/io/sentry/SpanContext.java +++ b/sentry/src/main/java/io/sentry/SpanContext.java @@ -194,6 +194,16 @@ public SpanId getParentSpanId() { return status; } + @NotNull + SpanContext withDefaultStatus() { + final @NotNull SpanContext traceContext = new SpanContext(this); + traceContext.setOrigin(getOrigin()); + if (traceContext.getStatus() == null) { + traceContext.setStatus(SpanStatus.OK); + } + return traceContext; + } + public @NotNull Map getTags() { return tags; } diff --git a/sentry/src/test/java/io/sentry/ScopeTest.kt b/sentry/src/test/java/io/sentry/ScopeTest.kt index 69f19b6d42..119455914f 100644 --- a/sentry/src/test/java/io/sentry/ScopeTest.kt +++ b/sentry/src/test/java/io/sentry/ScopeTest.kt @@ -849,13 +849,19 @@ class ScopeTest { val observer = mock() val options = SentryOptions().apply { addScopeObserver(observer) } val scope = Scope(options) + val spanContext = SpanContext("ui.load") scope.transaction = mock { whenever(mock.name).thenReturn("main") - whenever(mock.spanContext).thenReturn(SpanContext("ui.load")) + whenever(mock.spanContext).thenReturn(spanContext) } verify(observer).setTransaction(eq("main")) - verify(observer).setTrace(argThat { operation == "ui.load" }, eq(scope)) + verify(observer) + .setTrace( + argThat { operation == "ui.load" && status == SpanStatus.OK }, + eq(scope), + ) + assertThat(spanContext.status).isNull() } @Test diff --git a/sentry/src/test/java/io/sentry/ScopesTest.kt b/sentry/src/test/java/io/sentry/ScopesTest.kt index cdf9919a27..fd6ef367bf 100644 --- a/sentry/src/test/java/io/sentry/ScopesTest.kt +++ b/sentry/src/test/java/io/sentry/ScopesTest.kt @@ -415,7 +415,7 @@ class ScopesTest { val hints = HintUtils.createWithTypeCheckHint({}) sut.captureEvent(event, hints) - assertEquals(span.spanContext, event.contexts.trace) + assertTraceContextDefaultsToOk(span.spanContext, event.contexts.trace) verify(mockClient).captureEvent(eq(event), any(), eq(hints)) } @@ -431,7 +431,7 @@ class ScopesTest { val hints = HintUtils.createWithTypeCheckHint({}) sut.captureEvent(event, hints) - assertEquals(span.spanContext, event.contexts.trace) + assertTraceContextDefaultsToOk(span.spanContext, event.contexts.trace) verify(mockClient).captureEvent(eq(event), any(), eq(hints)) } @@ -448,7 +448,7 @@ class ScopesTest { val hints = HintUtils.createWithTypeCheckHint({}) sut.captureEvent(event, hints) - assertEquals(span.spanContext, event.contexts.trace) + assertTraceContextDefaultsToOk(span.spanContext, event.contexts.trace) verify(mockClient).captureEvent(eq(event), any(), eq(hints)) } @@ -729,7 +729,7 @@ class ScopesTest { verify(mockClient) .captureEvent( check { - assertEquals(span.spanContext, it.contexts.trace) + assertTraceContextDefaultsToOk(span.spanContext, it.contexts.trace) assertEquals("tx-name", it.transaction) }, any(), @@ -2153,7 +2153,7 @@ class ScopesTest { verify(mockClient) .captureEvent( - check { assertEquals(span.spanContext, it.contexts.trace) }, + check { assertTraceContextDefaultsToOk(span.spanContext, it.contexts.trace) }, anyOrNull(), anyOrNull(), ) @@ -4442,6 +4442,17 @@ class ScopesTest { return fileHashFolder.absolutePath } + private fun assertTraceContextDefaultsToOk( + expectedContext: SpanContext, + actualContext: SpanContext?, + ) { + assertThat(actualContext).isNotNull() + assertThat(actualContext!!.traceId).isEqualTo(expectedContext.traceId) + assertThat(actualContext.spanId).isEqualTo(expectedContext.spanId) + assertThat(actualContext.status).isEqualTo(SpanStatus.OK) + assertThat(expectedContext.status).isNull() + } + private fun equalTraceContext(expectedContext: TraceContext?): TraceContext? { expectedContext ?: return eq(null) diff --git a/sentry/src/test/java/io/sentry/SentryClientTest.kt b/sentry/src/test/java/io/sentry/SentryClientTest.kt index 926cf2d288..cc8ccf1b78 100644 --- a/sentry/src/test/java/io/sentry/SentryClientTest.kt +++ b/sentry/src/test/java/io/sentry/SentryClientTest.kt @@ -2239,7 +2239,10 @@ class SentryClientTest { transaction.finish() sut.captureEvent(event, scope) assertNotNull(event.contexts.trace) - assertEquals(transaction.root.spanContext, event.contexts.trace) + assertThat(event.contexts.trace!!.traceId).isEqualTo(transaction.root.spanContext.traceId) + assertThat(event.contexts.trace!!.spanId).isEqualTo(transaction.root.spanContext.spanId) + assertThat(event.contexts.trace!!.status).isEqualTo(SpanStatus.OK) + assertThat(transaction.root.spanContext.status).isNull() } @Test @@ -2252,7 +2255,10 @@ class SentryClientTest { val span = transaction.startChild("op") sut.captureEvent(event, scope) assertNotNull(event.contexts.trace) - assertEquals(span.spanContext, event.contexts.trace) + assertThat(event.contexts.trace!!.traceId).isEqualTo(span.spanContext.traceId) + assertThat(event.contexts.trace!!.spanId).isEqualTo(span.spanContext.spanId) + assertThat(event.contexts.trace!!.status).isEqualTo(SpanStatus.OK) + assertThat(span.spanContext.status).isNull() } @Test @@ -3288,6 +3294,8 @@ class SentryClientTest { assertEquals(spanContext.traceId, sentryEvent.contexts.trace!!.traceId) assertEquals(spanContext.spanId, sentryEvent.contexts.trace!!.spanId) + assertThat(sentryEvent.contexts.trace!!.status).isEqualTo(SpanStatus.OK) + assertThat(spanContext.status).isNull() assertNotEquals(scopePropagationContext.traceId, sentryEvent.contexts.trace!!.traceId) assertNotEquals(scopePropagationContext.spanId, sentryEvent.contexts.trace!!.spanId) } diff --git a/sentry/src/test/java/io/sentry/SpanContextTest.kt b/sentry/src/test/java/io/sentry/SpanContextTest.kt index aa941135e8..7ebc7f7015 100644 --- a/sentry/src/test/java/io/sentry/SpanContextTest.kt +++ b/sentry/src/test/java/io/sentry/SpanContextTest.kt @@ -1,5 +1,6 @@ package io.sentry +import com.google.common.truth.Truth.assertThat import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFalse @@ -21,6 +22,30 @@ class SpanContextTest { assertNotNull(trace.data[SpanDataConvention.THREAD_NAME]) } + @Test + fun `withDefaultStatus returns a copy with the default status without modifying the original`() { + val spanContext = SpanContext("op").apply { origin = "auto.test" } + + val traceContext = spanContext.withDefaultStatus() + + assertThat(traceContext).isNotSameInstanceAs(spanContext) + assertThat(traceContext.traceId).isEqualTo(spanContext.traceId) + assertThat(traceContext.spanId).isEqualTo(spanContext.spanId) + assertThat(traceContext.origin).isEqualTo(spanContext.origin) + assertThat(traceContext.status).isEqualTo(SpanStatus.OK) + assertThat(spanContext.status).isNull() + } + + @Test + fun `withDefaultStatus preserves an explicitly set status`() { + val spanContext = SpanContext("op").apply { status = SpanStatus.INTERNAL_ERROR } + + val traceContext = spanContext.withDefaultStatus() + + assertThat(traceContext).isNotSameInstanceAs(spanContext) + assertThat(traceContext.status).isEqualTo(SpanStatus.INTERNAL_ERROR) + } + @Test fun `sets tag`() { val trace = SpanContext("op")