diff --git a/CHANGELOG.md b/CHANGELOG.md index 4af90e40ee0..309d16238ce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -127,6 +127,8 @@ - Report attached profiles dropped by transaction callback errors as `callback_error` in client reports and `OnDiscardCallback` ([#6166](https://github.com/getsentry/sentry-java/pull/6166)) - Drop telemetry and record `callback_error` when a customer event processor throws instead of continuing with a potentially partially processed item. SDK-owned processor failures are logged and processing continues without a `callback_error` client report. - Drop breadcrumbs when `beforeBreadcrumb` throws instead of storing exception details on the breadcrumb. + - Skip Android screenshot or view hierarchy capture when its capture callback throws, while retaining the error event ([#6167](https://github.com/getsentry/sentry-java/pull/6167)) + - Drop spans when `beforeSpan` throws in OkHttp, OpenFeign, GraphQL, Ktor, or Apollo, without disrupting the request. Report lost sampled spans as `callback_error` in client reports and `OnDiscardCallback` ([#6167](https://github.com/getsentry/sentry-java/pull/6167)) - Skip replay capture when `beforeErrorSampling` throws, while still sending the error event ([#6165](https://github.com/getsentry/sentry-java/pull/6165)) - When `tracesSampler` throws, drop the transaction and record `callback_error` instead of inheriting the parent sampling decision or falling back to `tracesSampleRate` ([#6163](https://github.com/getsentry/sentry-java/pull/6163)) - When `profilesSampler` throws, disable profiling instead of falling back to `profilesSampleRate` or inheriting the parent's profiling decision. Trace sampling is unchanged ([#6164](https://github.com/getsentry/sentry-java/pull/6164)) diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/ScreenshotEventProcessor.java b/sentry-android-core/src/main/java/io/sentry/android/core/ScreenshotEventProcessor.java index 7783955f42c..6932b6be3f6 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/ScreenshotEventProcessor.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/ScreenshotEventProcessor.java @@ -112,7 +112,17 @@ private boolean isMaskingEnabled() { final @Nullable SentryAndroidOptions.BeforeCaptureCallback beforeCaptureCallback = options.getBeforeScreenshotCaptureCallback(); if (beforeCaptureCallback != null) { - if (!beforeCaptureCallback.execute(event, hint, shouldDebounce)) { + try { + if (!beforeCaptureCallback.execute(event, hint, shouldDebounce)) { + return event; + } + } catch (Exception e) { + options + .getLogger() + .log( + SentryLevel.ERROR, + "The beforeScreenshotCapture callback threw an exception. Skipping screenshot capture.", + e); return event; } } else if (shouldDebounce) { diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/ViewHierarchyEventProcessor.java b/sentry-android-core/src/main/java/io/sentry/android/core/ViewHierarchyEventProcessor.java index 77e3ad917ea..00753ee4c90 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/ViewHierarchyEventProcessor.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/ViewHierarchyEventProcessor.java @@ -91,7 +91,17 @@ public ViewHierarchyEventProcessor(final @NotNull SentryAndroidOptions options) final @Nullable SentryAndroidOptions.BeforeCaptureCallback beforeCaptureCallback = options.getBeforeViewHierarchyCaptureCallback(); if (beforeCaptureCallback != null) { - if (!beforeCaptureCallback.execute(event, hint, shouldDebounce)) { + try { + if (!beforeCaptureCallback.execute(event, hint, shouldDebounce)) { + return event; + } + } catch (Exception e) { + options + .getLogger() + .log( + SentryLevel.ERROR, + "The beforeViewHierarchyCapture callback threw an exception. Skipping view hierarchy capture.", + e); return event; } } else if (shouldDebounce) { diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/ScreenshotEventProcessorTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/ScreenshotEventProcessorTest.kt index b8e223f08e9..635d7a308c5 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/ScreenshotEventProcessorTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/ScreenshotEventProcessorTest.kt @@ -29,11 +29,14 @@ import androidx.compose.ui.text.style.TextOverflow import androidx.compose.ui.unit.dp import androidx.compose.ui.unit.sp import androidx.test.ext.junit.runners.AndroidJUnit4 +import com.google.common.truth.Truth.assertThat import io.sentry.Attachment import io.sentry.Hint +import io.sentry.ILogger import io.sentry.MainEventProcessor import io.sentry.SentryEvent import io.sentry.SentryIntegrationPackageStorage +import io.sentry.SentryLevel import io.sentry.TypeCheckHint.ANDROID_ACTIVITY import io.sentry.protocol.SentryException import io.sentry.util.thread.IThreadChecker @@ -48,6 +51,7 @@ import kotlin.test.assertSame import kotlin.test.assertTrue import org.junit.runner.RunWith import org.mockito.kotlin.mock +import org.mockito.kotlin.verify import org.mockito.kotlin.whenever import org.robolectric.Robolectric.buildActivity import org.robolectric.Shadows.shadowOf @@ -310,11 +314,38 @@ class ScreenshotEventProcessorTest { assertNull(hint.screenshot) } + @Test + fun `when capture callback throws, skips screenshot and retains event`() { + CurrentActivityHolder.getInstance().setActivity(fixture.activity) + val logger = mock() + fixture.options.isDebug = true + fixture.options.setLogger(logger) + val failure = IllegalStateException("callback failed") + fixture.options.setBeforeScreenshotCaptureCallback { _, _, _ -> throw failure } + val processor = fixture.getSut(true) + val event = SentryEvent().apply { exceptions = listOf(SentryException()) } + val hint = Hint() + + assertThat(processor.process(event, hint)).isSameInstanceAs(event) + assertThat(hint.screenshot).isNull() + verify(logger) + .log( + SentryLevel.ERROR, + "The beforeScreenshotCapture callback threw an exception. Skipping screenshot capture.", + failure, + ) + + fixture.options.setBeforeScreenshotCaptureCallback { _, _, _ -> true } + val nextHint = Hint() + assertThat(processor.process(event, nextHint)).isSameInstanceAs(event) + assertThat(nextHint.screenshot).isNotNull() + } + @Test fun `when capture callback returns true, a screenshot should be captured`() { CurrentActivityHolder.getInstance().setActivity(fixture.activity) - fixture.options.setBeforeViewHierarchyCaptureCallback { _, _, _ -> true } + fixture.options.setBeforeScreenshotCaptureCallback { _, _, _ -> true } val processor = fixture.getSut(true) val event = SentryEvent().apply { exceptions = listOf(SentryException()) } diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/ViewHierarchyEventProcessorTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/ViewHierarchyEventProcessorTest.kt index 4d908fcac1c..6f19035a8d9 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/ViewHierarchyEventProcessorTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/ViewHierarchyEventProcessorTest.kt @@ -5,11 +5,13 @@ import android.view.View import android.view.ViewGroup import android.view.Window import androidx.test.ext.junit.runners.AndroidJUnit4 +import com.google.common.truth.Truth.assertThat import io.sentry.Hint import io.sentry.JsonSerializable import io.sentry.JsonSerializer import io.sentry.SentryEvent import io.sentry.SentryIntegrationPackageStorage +import io.sentry.SentryLevel import io.sentry.TypeCheckHint import io.sentry.protocol.SentryException import io.sentry.util.thread.IThreadChecker @@ -342,6 +344,31 @@ class ViewHierarchyEventProcessorTest { assertNull(hint.viewHierarchy) } + @Test + fun `when capture callback throws, skips view hierarchy and retains event`() { + fixture.options.isDebug = true + fixture.options.setLogger(fixture.logger) + val failure = IllegalStateException("callback failed") + fixture.options.setBeforeViewHierarchyCaptureCallback { _, _, _ -> throw failure } + val processor = fixture.getSut(true) + val event = SentryEvent().apply { exceptions = listOf(SentryException()) } + val hint = Hint() + + assertThat(processor.process(event, hint)).isSameInstanceAs(event) + assertThat(hint.viewHierarchy).isNull() + verify(fixture.logger) + .log( + SentryLevel.ERROR, + "The beforeViewHierarchyCapture callback threw an exception. Skipping view hierarchy capture.", + failure, + ) + + fixture.options.setBeforeViewHierarchyCaptureCallback { _, _, _ -> true } + val nextHint = Hint() + assertThat(processor.process(event, nextHint)).isSameInstanceAs(event) + assertThat(nextHint.viewHierarchy).isNotNull() + } + @Test fun `when capture callback returns true, a view hierarchy should be captured`() { fixture.options.setBeforeViewHierarchyCaptureCallback { _, _, _ -> true } diff --git a/sentry-apollo-3/build.gradle.kts b/sentry-apollo-3/build.gradle.kts index 70f43d946ef..d143e30c508 100644 --- a/sentry-apollo-3/build.gradle.kts +++ b/sentry-apollo-3/build.gradle.kts @@ -33,6 +33,7 @@ dependencies { testImplementation(kotlin(Config.kotlinStdLib)) testImplementation(libs.apollo3.kotlin) testImplementation(libs.kotlin.test.junit) + testImplementation(libs.google.truth) testImplementation(libs.kotlinx.coroutines) testImplementation(libs.mockito.kotlin) testImplementation(libs.mockito.inline) 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 94ba52cb592..080afc5af7c 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 @@ -10,6 +10,7 @@ import com.apollographql.apollo3.network.http.HttpInterceptor import com.apollographql.apollo3.network.http.HttpInterceptorChain import io.sentry.BaggageHeader import io.sentry.Breadcrumb +import io.sentry.DataCategory import io.sentry.Hint import io.sentry.IScopes import io.sentry.ISpan @@ -23,6 +24,7 @@ import io.sentry.SpanDataConvention.HTTP_METHOD_KEY import io.sentry.SpanStatus import io.sentry.TypeCheckHint.APOLLO_REQUEST import io.sentry.TypeCheckHint.APOLLO_RESPONSE +import io.sentry.clientreport.DiscardReason import io.sentry.exception.ExceptionMechanismException import io.sentry.protocol.Mechanism import io.sentry.protocol.Request @@ -216,6 +218,7 @@ constructor( span.setData(SpanDataConvention.HTTP_RESPONSE_CONTENT_LENGTH_KEY, it) } if (beforeSpan != null) { + val wasSampled = span.isSampled == true try { val result = beforeSpan.execute(span, request, response) if (result == null) { @@ -223,6 +226,13 @@ constructor( span.spanContext.sampled = false } } catch (e: Throwable) { + span.spanContext.sampled = false + if (wasSampled) { + scopes.options.clientReportRecorder.recordLostEvent( + DiscardReason.CALLBACK_ERROR, + DataCategory.Span, + ) + } scopes.options.logger.log( SentryLevel.ERROR, "An error occurred while executing beforeSpan on ApolloInterceptor", 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 8316f6c0f33..09d298744bd 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 @@ -7,12 +7,16 @@ import com.apollographql.apollo3.exception.ApolloException import com.apollographql.apollo3.exception.ApolloHttpException import com.apollographql.apollo3.network.http.HttpInterceptor import com.apollographql.apollo3.network.http.HttpInterceptorChain +import com.google.common.truth.Truth.assertThat import io.sentry.BaggageHeader import io.sentry.Breadcrumb +import io.sentry.DataCategory +import io.sentry.ILogger import io.sentry.IScopes import io.sentry.ITransaction import io.sentry.Scope import io.sentry.ScopeCallback +import io.sentry.SentryLevel import io.sentry.SentryOptions import io.sentry.SentryOptions.DEFAULT_PROPAGATION_TARGETS import io.sentry.SentryTraceHeader @@ -25,6 +29,7 @@ import io.sentry.TracesSamplingDecision import io.sentry.TransactionContext import io.sentry.W3CTraceparentHeader import io.sentry.apollo3.SentryApollo3HttpInterceptor.BeforeSpanCallback +import io.sentry.clientreport.DiscardReason import io.sentry.mockServerRequestTimeoutMillis import io.sentry.protocol.SdkVersion import io.sentry.protocol.SentryTransaction @@ -47,6 +52,7 @@ import org.mockito.kotlin.check import org.mockito.kotlin.doAnswer import org.mockito.kotlin.mock import org.mockito.kotlin.verify +import org.mockito.kotlin.verifyNoMoreInteractions import org.mockito.kotlin.whenever class SentryApollo3InterceptorTest { @@ -291,7 +297,10 @@ class SentryApollo3InterceptorTest { @Test fun `returning null in beforeSpan callback drops span`() { + val onDiscard = mock() + fixture.options.onDiscard = onDiscard executeQuery(fixture.getSut(beforeSpan = { _, _, _ -> null })) + verifyNoMoreInteractions(onDiscard) verify(fixture.scopes) .captureTransaction( @@ -303,16 +312,68 @@ class SentryApollo3InterceptorTest { } @Test - fun `when customizer throws, exception is handled`() { - executeQuery(fixture.getSut(beforeSpan = { _, _, _ -> throw RuntimeException() })) + fun `reports callback errors only for sampled spans`(): Unit = runBlocking { + for (sampled in listOf(true, false, null)) { + val onDiscard = mock() + fixture.options.onDiscard = onDiscard + val tx = SentryTracer(TransactionContext("op", "desc"), fixture.scopes) + tx.spanContext.sampled = sampled + whenever(fixture.scopes.span).thenReturn(tx) + val sut = + fixture.getSut( + beforeSpan = { span, _, _ -> + span.spanContext.sampled = false + throw IllegalStateException("callback failed") + } + ) + + assertThat(sut.query(LaunchDetailsQuery("83")).execute().data).isNotNull() + tx.finish() + + if (sampled == true) { + verify(onDiscard).execute(DiscardReason.CALLBACK_ERROR, DataCategory.Span, 1) + } + verifyNoMoreInteractions(onDiscard) + } + } + + @Test + fun `when beforeSpan throws, drops span and preserves response`(): Unit = runBlocking { + val failure = IllegalStateException("callback failed") + val logger = mock() + fixture.options.isDebug = true + fixture.options.setLogger(logger) + val tx = + SentryTracer(TransactionContext("op", "desc", TracesSamplingDecision(true)), fixture.scopes) + whenever(fixture.scopes.span).thenReturn(tx) + val sut = + fixture.getSut( + beforeSpan = { span, _, _ -> + span.description = "partially modified" + throw failure + } + ) + val response = sut.query(LaunchDetailsQuery("83")).execute() + assertThat(response.data).isNotNull() + val span = tx.children.single() + assertThat(span.isSampled).isFalse() + assertThat(span.isFinished).isTrue() + tx.finish() verify(fixture.scopes) .captureTransaction( - check { assertEquals(1, it.spans.size) }, + check { assertThat(it.spans).isEmpty() }, anyOrNull(), anyOrNull(), anyOrNull(), ) + verify(fixture.scopes).addBreadcrumb(any(), anyOrNull()) + verify(logger) + .log( + SentryLevel.ERROR, + "An error occurred while executing beforeSpan on ApolloInterceptor", + failure, + ) } @Test diff --git a/sentry-apollo-4/build.gradle.kts b/sentry-apollo-4/build.gradle.kts index 4f1276f0bf4..304f34fb74d 100644 --- a/sentry-apollo-4/build.gradle.kts +++ b/sentry-apollo-4/build.gradle.kts @@ -33,6 +33,7 @@ dependencies { testImplementation(kotlin(Config.kotlinStdLib)) testImplementation(libs.apollo4.kotlin) testImplementation(libs.kotlin.test.junit) + testImplementation(libs.google.truth) testImplementation(libs.kotlinx.coroutines) testImplementation(libs.kotlinx.coroutines.test) testImplementation(libs.mockito.kotlin) diff --git a/sentry-apollo-4/src/main/java/io/sentry/apollo4/SentryApollo4HttpInterceptor.kt b/sentry-apollo-4/src/main/java/io/sentry/apollo4/SentryApollo4HttpInterceptor.kt index 54ad1d50fdb..11eed8ab9d3 100644 --- a/sentry-apollo-4/src/main/java/io/sentry/apollo4/SentryApollo4HttpInterceptor.kt +++ b/sentry-apollo-4/src/main/java/io/sentry/apollo4/SentryApollo4HttpInterceptor.kt @@ -8,6 +8,7 @@ import com.apollographql.apollo.network.http.HttpInterceptor import com.apollographql.apollo.network.http.HttpInterceptorChain import io.sentry.BaggageHeader import io.sentry.Breadcrumb +import io.sentry.DataCategory import io.sentry.Hint import io.sentry.IScopes import io.sentry.ISpan @@ -21,6 +22,7 @@ import io.sentry.SpanDataConvention.HTTP_METHOD_KEY import io.sentry.SpanStatus import io.sentry.TypeCheckHint.APOLLO_REQUEST import io.sentry.TypeCheckHint.APOLLO_RESPONSE +import io.sentry.clientreport.DiscardReason import io.sentry.exception.ExceptionMechanismException import io.sentry.protocol.Mechanism import io.sentry.protocol.Request @@ -215,6 +217,7 @@ constructor( span.setData(SpanDataConvention.HTTP_RESPONSE_CONTENT_LENGTH_KEY, it) } if (beforeSpan != null) { + val wasSampled = span.isSampled == true try { val result = beforeSpan.execute(span, request, response) if (result == null) { @@ -222,6 +225,13 @@ constructor( span.spanContext.sampled = false } } catch (e: Throwable) { + span.spanContext.sampled = false + if (wasSampled) { + scopes.options.clientReportRecorder.recordLostEvent( + DiscardReason.CALLBACK_ERROR, + DataCategory.Span, + ) + } scopes.options.logger.log( SentryLevel.ERROR, "An error occurred while executing beforeSpan in ApolloInterceptor", diff --git a/sentry-apollo-4/src/test/java/io/sentry/apollo4/SentryApollo4HttpInterceptorTest.kt b/sentry-apollo-4/src/test/java/io/sentry/apollo4/SentryApollo4HttpInterceptorTest.kt index d92cefe9772..ce94160303a 100644 --- a/sentry-apollo-4/src/test/java/io/sentry/apollo4/SentryApollo4HttpInterceptorTest.kt +++ b/sentry-apollo-4/src/test/java/io/sentry/apollo4/SentryApollo4HttpInterceptorTest.kt @@ -10,12 +10,16 @@ import com.apollographql.apollo.exception.ApolloException import com.apollographql.apollo.exception.ApolloHttpException import com.apollographql.apollo.network.http.HttpInterceptor import com.apollographql.apollo.network.http.HttpInterceptorChain +import com.google.common.truth.Truth.assertThat import io.sentry.BaggageHeader import io.sentry.Breadcrumb +import io.sentry.DataCategory +import io.sentry.ILogger import io.sentry.IScopes import io.sentry.ITransaction import io.sentry.Scope import io.sentry.ScopeCallback +import io.sentry.SentryLevel import io.sentry.SentryOptions import io.sentry.SentryOptions.DEFAULT_PROPAGATION_TARGETS import io.sentry.SentryTraceHeader @@ -29,6 +33,7 @@ import io.sentry.TransactionContext import io.sentry.W3CTraceparentHeader import io.sentry.apollo4.SentryApollo4HttpInterceptor.BeforeSpanCallback import io.sentry.apollo4.generated.LaunchDetailsQuery +import io.sentry.clientreport.DiscardReason import io.sentry.mockServerRequestTimeoutMillis import io.sentry.protocol.SdkVersion import io.sentry.protocol.SentryTransaction @@ -52,6 +57,7 @@ import org.mockito.kotlin.check import org.mockito.kotlin.doAnswer import org.mockito.kotlin.mock import org.mockito.kotlin.verify +import org.mockito.kotlin.verifyNoMoreInteractions import org.mockito.kotlin.whenever class SentryApollo4HttpInterceptorTestWithV4Implementation : @@ -305,7 +311,10 @@ abstract class SentryApollo4HttpInterceptorTest( @Test fun `returning null in beforeSpan callback drops span`() { + val onDiscard = mock() + fixture.options.onDiscard = onDiscard executeQuery(fixture.getSut(beforeSpan = { _, _, _ -> null })) + verifyNoMoreInteractions(onDiscard) verify(fixture.scopes) .captureTransaction( @@ -317,16 +326,68 @@ abstract class SentryApollo4HttpInterceptorTest( } @Test - fun `when customizer throws, exception is handled`() { - executeQuery(fixture.getSut(beforeSpan = { _, _, _ -> throw RuntimeException() })) + fun `reports callback errors only for sampled spans`(): Unit = runBlocking { + for (sampled in listOf(true, false, null)) { + val onDiscard = mock() + fixture.options.onDiscard = onDiscard + val tx = SentryTracer(TransactionContext("op", "desc"), fixture.scopes) + tx.spanContext.sampled = sampled + whenever(fixture.scopes.span).thenReturn(tx) + val sut = + fixture.getSut( + beforeSpan = { span, _, _ -> + span.spanContext.sampled = false + throw IllegalStateException("callback failed") + } + ) + + assertThat(executeQueryImplementation(sut.query(LaunchDetailsQuery("83"))).data).isNotNull() + tx.finish() + + if (sampled == true) { + verify(onDiscard).execute(DiscardReason.CALLBACK_ERROR, DataCategory.Span, 1) + } + verifyNoMoreInteractions(onDiscard) + } + } + + @Test + fun `when beforeSpan throws, drops span and preserves response`(): Unit = runBlocking { + val failure = IllegalStateException("callback failed") + val logger = mock() + fixture.options.isDebug = true + fixture.options.setLogger(logger) + val tx = + SentryTracer(TransactionContext("op", "desc", TracesSamplingDecision(true)), fixture.scopes) + whenever(fixture.scopes.span).thenReturn(tx) + val sut = + fixture.getSut( + beforeSpan = { span, _, _ -> + span.description = "partially modified" + throw failure + } + ) + val response = executeQueryImplementation(sut.query(LaunchDetailsQuery("83"))) + assertThat(response.data).isNotNull() + val span = tx.children.single() + assertThat(span.isSampled).isFalse() + assertThat(span.isFinished).isTrue() + tx.finish() verify(fixture.scopes) .captureTransaction( - check { assertEquals(1, it.spans.size) }, + check { assertThat(it.spans).isEmpty() }, anyOrNull(), anyOrNull(), anyOrNull(), ) + verify(fixture.scopes).addBreadcrumb(any(), anyOrNull()) + verify(logger) + .log( + SentryLevel.ERROR, + "An error occurred while executing beforeSpan in ApolloInterceptor", + failure, + ) } @Test diff --git a/sentry-apollo/build.gradle.kts b/sentry-apollo/build.gradle.kts index 2da8d8b20c1..534a5d92e70 100644 --- a/sentry-apollo/build.gradle.kts +++ b/sentry-apollo/build.gradle.kts @@ -34,6 +34,7 @@ dependencies { testImplementation(libs.apollo2.coroutines) testImplementation(libs.apollo2.runtime) testImplementation(libs.kotlin.test.junit) + testImplementation(libs.google.truth) testImplementation(libs.kotlinx.coroutines) testImplementation(libs.mockito.kotlin) testImplementation(libs.mockito.inline) 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 cb7df6472dd..3524d32dc1f 100644 --- a/sentry-apollo/src/main/java/io/sentry/apollo/SentryApolloInterceptor.kt +++ b/sentry-apollo/src/main/java/io/sentry/apollo/SentryApolloInterceptor.kt @@ -14,6 +14,7 @@ import com.apollographql.apollo.interceptor.ApolloInterceptorChain import com.apollographql.apollo.request.RequestHeaders import io.sentry.BaggageHeader import io.sentry.Breadcrumb +import io.sentry.DataCategory import io.sentry.Hint import io.sentry.IScopes import io.sentry.ISpan @@ -24,6 +25,7 @@ import io.sentry.SpanDataConvention import io.sentry.SpanStatus import io.sentry.TypeCheckHint.APOLLO_REQUEST import io.sentry.TypeCheckHint.APOLLO_RESPONSE +import io.sentry.clientreport.DiscardReason import io.sentry.util.IntegrationUtils.addIntegrationToSdkVersion import io.sentry.util.SpanUtils import io.sentry.util.TracingUtils @@ -175,9 +177,17 @@ class SentryApolloInterceptor( ) { var newSpan: ISpan? = span if (beforeSpan != null) { + val wasSampled = span.isSampled == true try { newSpan = beforeSpan.execute(span, request, response) } catch (e: Exception) { + span.spanContext.sampled = false + if (wasSampled) { + scopes.options.clientReportRecorder.recordLostEvent( + DiscardReason.CALLBACK_ERROR, + DataCategory.Span, + ) + } scopes.options.logger.log( SentryLevel.ERROR, "An error occurred while executing beforeSpan on ApolloInterceptor", 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 d43fe40c9e4..bc09f86a4e6 100644 --- a/sentry-apollo/src/test/java/io/sentry/apollo/SentryApolloInterceptorTest.kt +++ b/sentry-apollo/src/test/java/io/sentry/apollo/SentryApolloInterceptorTest.kt @@ -3,12 +3,16 @@ package io.sentry.apollo import com.apollographql.apollo.ApolloClient import com.apollographql.apollo.coroutines.await import com.apollographql.apollo.exception.ApolloException +import com.google.common.truth.Truth.assertThat import io.sentry.BaggageHeader import io.sentry.Breadcrumb +import io.sentry.DataCategory +import io.sentry.ILogger import io.sentry.IScopes import io.sentry.ITransaction import io.sentry.Scope import io.sentry.ScopeCallback +import io.sentry.SentryLevel import io.sentry.SentryOptions import io.sentry.SentryTraceHeader import io.sentry.SentryTracer @@ -17,6 +21,7 @@ import io.sentry.SpanStatus import io.sentry.TraceContext import io.sentry.TracesSamplingDecision import io.sentry.TransactionContext +import io.sentry.clientreport.DiscardReason import io.sentry.mockServerRequestTimeoutMillis import io.sentry.protocol.SdkVersion import io.sentry.protocol.SentryTransaction @@ -39,6 +44,7 @@ import org.mockito.kotlin.check import org.mockito.kotlin.doAnswer import org.mockito.kotlin.mock import org.mockito.kotlin.verify +import org.mockito.kotlin.verifyNoMoreInteractions import org.mockito.kotlin.whenever class SentryApolloInterceptorTest { @@ -229,7 +235,10 @@ class SentryApolloInterceptorTest { @Test fun `when beforeSpan callback returns null, span is dropped`() { + val onDiscard = mock() + fixture.options.onDiscard = onDiscard executeQuery(fixture.getSut { _, _, _ -> null }) + verifyNoMoreInteractions(onDiscard) verify(fixture.scopes) .captureTransaction( @@ -241,16 +250,62 @@ class SentryApolloInterceptorTest { } @Test - fun `when customizer throws, exception is handled`() { - executeQuery(fixture.getSut { _, _, _ -> throw RuntimeException() }) + fun `reports callback errors only for sampled spans`(): Unit = runBlocking { + for (sampled in listOf(true, false, null)) { + val onDiscard = mock() + fixture.options.onDiscard = onDiscard + val tx = SentryTracer(TransactionContext("op", "desc"), fixture.scopes) + tx.spanContext.sampled = sampled + whenever(fixture.scopes.span).thenReturn(tx) + val sut = fixture.getSut { span, _, _ -> + span.spanContext.sampled = false + throw IllegalStateException("callback failed") + } + + assertThat(sut.query(LaunchDetailsQuery.builder().id("83").build()).await().data).isNotNull() + tx.finish() + + if (sampled == true) { + verify(onDiscard).execute(DiscardReason.CALLBACK_ERROR, DataCategory.Span, 1) + } + verifyNoMoreInteractions(onDiscard) + } + } + @Test + fun `when beforeSpan throws, drops span and preserves response`(): Unit = runBlocking { + val failure = IllegalStateException("callback failed") + val logger = mock() + fixture.options.isDebug = true + fixture.options.setLogger(logger) + val tx = + SentryTracer(TransactionContext("op", "desc", TracesSamplingDecision(true)), fixture.scopes) + whenever(fixture.scopes.span).thenReturn(tx) + val sut = fixture.getSut { span, _, _ -> + span.description = "partially modified" + throw failure + } + + val response = sut.query(LaunchDetailsQuery.builder().id("83").build()).await() + assertThat(response.data).isNotNull() + val span = tx.children.single() + assertThat(span.isSampled).isFalse() + assertThat(span.isFinished).isTrue() + tx.finish() verify(fixture.scopes) .captureTransaction( - check { assertEquals(1, it.spans.size) }, + check { assertThat(it.spans).isEmpty() }, anyOrNull(), anyOrNull(), anyOrNull(), ) + verify(fixture.scopes).addBreadcrumb(any(), anyOrNull()) + verify(logger) + .log( + SentryLevel.ERROR, + "An error occurred while executing beforeSpan on ApolloInterceptor", + failure, + ) } @Test diff --git a/sentry-graphql-22/build.gradle.kts b/sentry-graphql-22/build.gradle.kts index 32db28fae8f..def6708f672 100644 --- a/sentry-graphql-22/build.gradle.kts +++ b/sentry-graphql-22/build.gradle.kts @@ -34,6 +34,7 @@ dependencies { testImplementation(kotlin(Config.kotlinStdLib)) testImplementation(libs.graphql.java22) testImplementation(libs.kotlin.test.junit) + testImplementation(libs.google.truth) testImplementation(libs.mockito.kotlin) testImplementation(libs.mockito.inline) testImplementation(libs.okhttp) diff --git a/sentry-graphql-22/src/test/kotlin/io/sentry/graphql22/SentryInstrumentationTest.kt b/sentry-graphql-22/src/test/kotlin/io/sentry/graphql22/SentryInstrumentationTest.kt index c684688299e..2331ab17aaf 100644 --- a/sentry-graphql-22/src/test/kotlin/io/sentry/graphql22/SentryInstrumentationTest.kt +++ b/sentry-graphql-22/src/test/kotlin/io/sentry/graphql22/SentryInstrumentationTest.kt @@ -1,5 +1,6 @@ package io.sentry.graphql22 +import com.google.common.truth.Truth.assertThat import graphql.GraphQL import graphql.GraphQLContext import graphql.execution.ExecutionContextBuilder @@ -18,17 +19,22 @@ import graphql.schema.GraphQLScalarType import graphql.schema.idl.RuntimeWiring import graphql.schema.idl.SchemaGenerator import graphql.schema.idl.SchemaParser +import io.sentry.DataCategory +import io.sentry.ILogger import io.sentry.IScopes import io.sentry.Sentry +import io.sentry.SentryLevel import io.sentry.SentryOptions import io.sentry.SentryTracer import io.sentry.SpanStatus import io.sentry.TransactionContext +import io.sentry.clientreport.DiscardReason import io.sentry.graphql.ExceptionReporter import io.sentry.graphql.NoOpSubscriptionHandler import io.sentry.graphql.SentryGraphqlInstrumentation import io.sentry.graphql.SentrySubscriptionHandler import java.lang.RuntimeException +import java.util.concurrent.CompletableFuture import kotlin.random.Random import kotlin.test.Test import kotlin.test.assertEquals @@ -38,6 +44,8 @@ import kotlin.test.assertTrue import org.mockito.Mockito import org.mockito.kotlin.any import org.mockito.kotlin.mock +import org.mockito.kotlin.verify +import org.mockito.kotlin.verifyNoMoreInteractions import org.mockito.kotlin.whenever class SentryInstrumentationTest { @@ -48,9 +56,11 @@ class SentryInstrumentationTest { fun getSut( isTransactionActive: Boolean = true, dataFetcherThrows: Boolean = false, + async: Boolean = false, beforeSpan: SentryGraphqlInstrumentation.BeforeSpanCallback? = null, ): GraphQL { - whenever(scopes.options).thenReturn(SentryOptions()) + whenever(scopes.options) + .thenReturn(SentryOptions().apply { dsn = "https://key@sentry.io/proj" }) activeSpan = SentryTracer(TransactionContext("name", "op"), scopes) val schema = """ @@ -66,7 +76,10 @@ class SentryInstrumentationTest { val graphQLSchema = SchemaGenerator() - .makeExecutableSchema(SchemaParser().parse(schema), buildRuntimeWiring(dataFetcherThrows)) + .makeExecutableSchema( + SchemaParser().parse(schema), + buildRuntimeWiring(dataFetcherThrows, async), + ) val graphQL = GraphQL.newGraphQL(graphQLSchema) .instrumentation( @@ -83,14 +96,15 @@ class SentryInstrumentationTest { return graphQL } - private fun buildRuntimeWiring(dataFetcherThrows: Boolean) = + private fun buildRuntimeWiring(dataFetcherThrows: Boolean, async: Boolean) = RuntimeWiring.newRuntimeWiring() .type("Query") { it.dataFetcher("shows") { if (dataFetcherThrows) { throw RuntimeException("error") } else { - listOf(Show(Random.nextInt()), Show(Random.nextInt())) + val shows = listOf(Show(Random.nextInt()), Show(Random.nextInt())) + if (async) CompletableFuture.completedFuture(shows) else shows } } } @@ -152,6 +166,9 @@ class SentryInstrumentationTest { fixture.getSut( beforeSpan = SentryGraphqlInstrumentation.BeforeSpanCallback { _, _, _ -> null } ) + val onDiscard = mock() + fixture.scopes.options.onDiscard = onDiscard + fixture.activeSpan.spanContext.sampled = true withMockScopes { val result = sut.execute("{ shows { id } }") @@ -162,6 +179,103 @@ class SentryInstrumentationTest { assertEquals("graphql", span.operation) assertEquals("Query.shows", span.description) assertNotNull(span.isSampled) { assertFalse(it) } + verifyNoMoreInteractions(onDiscard) + } + } + + @Test + fun `reports callback errors only for sampled spans`() { + for (sampled in listOf(true, false, null)) { + val onDiscard = mock() + val sut = + fixture.getSut( + beforeSpan = { span, _, _ -> + span.spanContext.sampled = false + throw IllegalStateException("callback failed") + } + ) + fixture.activeSpan.spanContext.sampled = sampled + fixture.scopes.options.onDiscard = onDiscard + + withMockScopes { + assertThat(sut.execute("{ shows { id } }").errors).isEmpty() + fixture.activeSpan.finish() + } + + if (sampled == true) { + verify(onDiscard).execute(DiscardReason.CALLBACK_ERROR, DataCategory.Span, 1) + } + verifyNoMoreInteractions(onDiscard) + } + } + + @Test + fun `when beforeSpan throws, drops span and preserves result`() { + val failure = IllegalStateException("callback failed") + val logger = mock() + val sut = + fixture.getSut( + beforeSpan = { span, _, _ -> + span.description = "partially modified" + throw failure + } + ) + fixture.scopes.options.isDebug = true + fixture.scopes.options.setLogger(logger) + + withMockScopes { + val result = sut.execute("{ shows { id } }") + assertThat(result.errors).isEmpty() + assertThat(result.getData>()).containsKey("shows") + val span = fixture.activeSpan.children.single() + assertThat(span.isSampled).isFalse() + assertThat(span.isFinished).isTrue() + verify(logger) + .log( + SentryLevel.ERROR, + "The beforeSpan callback threw an exception in SentryGraphqlInstrumentation. Dropping span.", + failure, + ) + } + } + + @Test + fun `when beforeSpan throws, drops async span and preserves result`() { + val sut = + fixture.getSut( + async = true, + beforeSpan = { _, _, _ -> + throw IllegalStateException("callback failed") + }, + ) + withMockScopes { + val result = sut.execute("{ shows { id } }") + assertThat(result.errors).isEmpty() + assertThat(result.getData>()).containsKey("shows") + val span = fixture.activeSpan.children.single() + assertThat(span.isSampled).isFalse() + assertThat(span.isFinished).isTrue() + } + } + + @Test + fun `when beforeSpan throws, preserves data fetcher error`() { + val sut = + fixture.getSut( + dataFetcherThrows = true, + beforeSpan = { _, _, _ -> + throw IllegalStateException("callback failed") + }, + ) + withMockScopes { + val result = sut.execute("{ shows { id } }") + assertThat(result.errors).hasSize(1) + assertThat(result.errors.single().message).contains("error") + assertThat(result.errors.single().message).doesNotContain("callback failed") + val span = fixture.activeSpan.children.single() + assertThat(span.isSampled).isFalse() + assertThat(span.isFinished).isTrue() + assertThat(span.status).isEqualTo(SpanStatus.INTERNAL_ERROR) } } diff --git a/sentry-graphql-core/src/main/java/io/sentry/graphql/SentryGraphqlInstrumentation.java b/sentry-graphql-core/src/main/java/io/sentry/graphql/SentryGraphqlInstrumentation.java index c316774c045..34044aea435 100644 --- a/sentry-graphql-core/src/main/java/io/sentry/graphql/SentryGraphqlInstrumentation.java +++ b/sentry-graphql-core/src/main/java/io/sentry/graphql/SentryGraphqlInstrumentation.java @@ -17,14 +17,17 @@ import graphql.schema.GraphQLObjectType; import graphql.schema.GraphQLOutputType; import io.sentry.Breadcrumb; +import io.sentry.DataCategory; import io.sentry.Hint; import io.sentry.IScopes; import io.sentry.ISpan; import io.sentry.NoOpScopes; import io.sentry.Sentry; +import io.sentry.SentryLevel; import io.sentry.SpanOptions; import io.sentry.SpanStatus; import io.sentry.TypeCheckHint; +import io.sentry.clientreport.DiscardReason; import io.sentry.util.StringUtils; import java.util.Arrays; import java.util.List; @@ -283,7 +286,26 @@ private void finish( final @NotNull DataFetchingEnvironment environment, final @Nullable Object result) { if (beforeSpan != null) { - final ISpan newSpan = beforeSpan.execute(span, environment, result); + final boolean wasSampled = Boolean.TRUE.equals(span.isSampled()); + ISpan newSpan = span; + try { + newSpan = beforeSpan.execute(span, environment, result); + } catch (Exception e) { + span.getSpanContext().setSampled(false); + if (wasSampled) { + scopesFromContext(environment.getGraphQlContext()) + .getOptions() + .getClientReportRecorder() + .recordLostEvent(DiscardReason.CALLBACK_ERROR, DataCategory.Span); + } + scopesFromContext(environment.getGraphQlContext()) + .getOptions() + .getLogger() + .log( + SentryLevel.ERROR, + "The beforeSpan callback threw an exception in SentryGraphqlInstrumentation. Dropping span.", + e); + } if (newSpan == null) { // span is dropped span.getSpanContext().setSampled(false); diff --git a/sentry-graphql/build.gradle.kts b/sentry-graphql/build.gradle.kts index d92dc52c6d7..f996e0310a1 100644 --- a/sentry-graphql/build.gradle.kts +++ b/sentry-graphql/build.gradle.kts @@ -34,6 +34,7 @@ dependencies { testImplementation(kotlin(Config.kotlinStdLib)) testImplementation(libs.graphql.java17) testImplementation(libs.kotlin.test.junit) + testImplementation(libs.google.truth) testImplementation(libs.mockito.kotlin) testImplementation(libs.mockito.inline) testImplementation(libs.okhttp) diff --git a/sentry-graphql/src/test/kotlin/io/sentry/graphql/SentryInstrumentationTest.kt b/sentry-graphql/src/test/kotlin/io/sentry/graphql/SentryInstrumentationTest.kt index 972b091a226..2652694b8f9 100644 --- a/sentry-graphql/src/test/kotlin/io/sentry/graphql/SentryInstrumentationTest.kt +++ b/sentry-graphql/src/test/kotlin/io/sentry/graphql/SentryInstrumentationTest.kt @@ -1,5 +1,6 @@ package io.sentry.graphql +import com.google.common.truth.Truth.assertThat import graphql.GraphQL import graphql.GraphQLContext import graphql.execution.ExecutionContextBuilder @@ -18,13 +19,18 @@ import graphql.schema.GraphQLScalarType import graphql.schema.idl.RuntimeWiring import graphql.schema.idl.SchemaGenerator import graphql.schema.idl.SchemaParser +import io.sentry.DataCategory +import io.sentry.ILogger import io.sentry.IScopes import io.sentry.Sentry +import io.sentry.SentryLevel import io.sentry.SentryOptions import io.sentry.SentryTracer import io.sentry.SpanStatus import io.sentry.TransactionContext +import io.sentry.clientreport.DiscardReason import java.lang.RuntimeException +import java.util.concurrent.CompletableFuture import kotlin.random.Random import kotlin.test.Test import kotlin.test.assertEquals @@ -34,6 +40,8 @@ import kotlin.test.assertTrue import org.mockito.Mockito import org.mockito.kotlin.any import org.mockito.kotlin.mock +import org.mockito.kotlin.verify +import org.mockito.kotlin.verifyNoMoreInteractions import org.mockito.kotlin.whenever class SentryInstrumentationTest { @@ -44,9 +52,11 @@ class SentryInstrumentationTest { fun getSut( isTransactionActive: Boolean = true, dataFetcherThrows: Boolean = false, + async: Boolean = false, beforeSpan: SentryGraphqlInstrumentation.BeforeSpanCallback? = null, ): GraphQL { - whenever(scopes.options).thenReturn(SentryOptions()) + whenever(scopes.options) + .thenReturn(SentryOptions().apply { dsn = "https://key@sentry.io/proj" }) activeSpan = SentryTracer(TransactionContext("name", "op"), scopes) val schema = """ @@ -62,7 +72,10 @@ class SentryInstrumentationTest { val graphQLSchema = SchemaGenerator() - .makeExecutableSchema(SchemaParser().parse(schema), buildRuntimeWiring(dataFetcherThrows)) + .makeExecutableSchema( + SchemaParser().parse(schema), + buildRuntimeWiring(dataFetcherThrows, async), + ) val graphQL = GraphQL.newGraphQL(graphQLSchema) .instrumentation( @@ -79,14 +92,15 @@ class SentryInstrumentationTest { return graphQL } - private fun buildRuntimeWiring(dataFetcherThrows: Boolean) = + private fun buildRuntimeWiring(dataFetcherThrows: Boolean, async: Boolean) = RuntimeWiring.newRuntimeWiring() .type("Query") { it.dataFetcher("shows") { if (dataFetcherThrows) { throw RuntimeException("error") } else { - listOf(Show(Random.nextInt()), Show(Random.nextInt())) + val shows = listOf(Show(Random.nextInt()), Show(Random.nextInt())) + if (async) CompletableFuture.completedFuture(shows) else shows } } } @@ -148,6 +162,9 @@ class SentryInstrumentationTest { fixture.getSut( beforeSpan = SentryGraphqlInstrumentation.BeforeSpanCallback { _, _, _ -> null } ) + val onDiscard = mock() + fixture.scopes.options.onDiscard = onDiscard + fixture.activeSpan.spanContext.sampled = true withMockScopes { val result = sut.execute("{ shows { id } }") @@ -158,6 +175,103 @@ class SentryInstrumentationTest { assertEquals("graphql", span.operation) assertEquals("Query.shows", span.description) assertNotNull(span.isSampled) { assertFalse(it) } + verifyNoMoreInteractions(onDiscard) + } + } + + @Test + fun `reports callback errors only for sampled spans`() { + for (sampled in listOf(true, false, null)) { + val onDiscard = mock() + val sut = + fixture.getSut( + beforeSpan = { span, _, _ -> + span.spanContext.sampled = false + throw IllegalStateException("callback failed") + } + ) + fixture.activeSpan.spanContext.sampled = sampled + fixture.scopes.options.onDiscard = onDiscard + + withMockScopes { + assertThat(sut.execute("{ shows { id } }").errors).isEmpty() + fixture.activeSpan.finish() + } + + if (sampled == true) { + verify(onDiscard).execute(DiscardReason.CALLBACK_ERROR, DataCategory.Span, 1) + } + verifyNoMoreInteractions(onDiscard) + } + } + + @Test + fun `when beforeSpan throws, drops span and preserves result`() { + val failure = IllegalStateException("callback failed") + val logger = mock() + val sut = + fixture.getSut( + beforeSpan = { span, _, _ -> + span.description = "partially modified" + throw failure + } + ) + fixture.scopes.options.isDebug = true + fixture.scopes.options.setLogger(logger) + + withMockScopes { + val result = sut.execute("{ shows { id } }") + assertThat(result.errors).isEmpty() + assertThat(result.getData>()).containsKey("shows") + val span = fixture.activeSpan.children.single() + assertThat(span.isSampled).isFalse() + assertThat(span.isFinished).isTrue() + verify(logger) + .log( + SentryLevel.ERROR, + "The beforeSpan callback threw an exception in SentryGraphqlInstrumentation. Dropping span.", + failure, + ) + } + } + + @Test + fun `when beforeSpan throws, drops async span and preserves result`() { + val sut = + fixture.getSut( + async = true, + beforeSpan = { _, _, _ -> + throw IllegalStateException("callback failed") + }, + ) + withMockScopes { + val result = sut.execute("{ shows { id } }") + assertThat(result.errors).isEmpty() + assertThat(result.getData>()).containsKey("shows") + val span = fixture.activeSpan.children.single() + assertThat(span.isSampled).isFalse() + assertThat(span.isFinished).isTrue() + } + } + + @Test + fun `when beforeSpan throws, preserves data fetcher error`() { + val sut = + fixture.getSut( + dataFetcherThrows = true, + beforeSpan = { _, _, _ -> + throw IllegalStateException("callback failed") + }, + ) + withMockScopes { + val result = sut.execute("{ shows { id } }") + assertThat(result.errors).hasSize(1) + assertThat(result.errors.single().message).contains("error") + assertThat(result.errors.single().message).doesNotContain("callback failed") + val span = fixture.activeSpan.children.single() + assertThat(span.isSampled).isFalse() + assertThat(span.isFinished).isTrue() + assertThat(span.status).isEqualTo(SpanStatus.INTERNAL_ERROR) } } diff --git a/sentry-ktor-client/build.gradle.kts b/sentry-ktor-client/build.gradle.kts index fefcdbfebaf..ecd48c5f1df 100644 --- a/sentry-ktor-client/build.gradle.kts +++ b/sentry-ktor-client/build.gradle.kts @@ -34,6 +34,7 @@ dependencies { testImplementation(projects.sentryTestSupport) testImplementation(libs.kotlin.test.junit) + testImplementation(libs.google.truth) testImplementation(libs.mockito.kotlin) testImplementation(libs.mockito.inline) testImplementation(libs.ktor.client.core) diff --git a/sentry-ktor-client/src/main/java/io/sentry/ktorClient/SentryKtorClientPlugin.kt b/sentry-ktor-client/src/main/java/io/sentry/ktorClient/SentryKtorClientPlugin.kt index 95cdfb5fdae..609ed239961 100644 --- a/sentry-ktor-client/src/main/java/io/sentry/ktorClient/SentryKtorClientPlugin.kt +++ b/sentry-ktor-client/src/main/java/io/sentry/ktorClient/SentryKtorClientPlugin.kt @@ -9,6 +9,7 @@ import io.ktor.util.* import io.ktor.util.pipeline.* import io.sentry.BaggageHeader import io.sentry.BuildConfig +import io.sentry.DataCategory import io.sentry.HttpStatusCodeRange import io.sentry.IScopes import io.sentry.ISpan @@ -16,8 +17,10 @@ import io.sentry.ScopesAdapter import io.sentry.Sentry import io.sentry.SentryDate import io.sentry.SentryIntegrationPackageStorage +import io.sentry.SentryLevel import io.sentry.SentryOptions import io.sentry.SpanStatus +import io.sentry.clientreport.DiscardReason import io.sentry.kotlin.SentryContext import io.sentry.util.IntegrationUtils.addIntegrationToSdkVersion import io.sentry.util.Platform @@ -186,7 +189,28 @@ public val SentryKtorClientPlugin: ClientPlugin = var result: ISpan? = span if (beforeSpan != null) { - result = beforeSpan.execute(span, request) + val wasSampled = span.isSampled == true + result = + try { + beforeSpan.execute(span, request) + } catch (e: Exception) { + span.spanContext.sampled = false + if (wasSampled) { + (if (forceScopes) scopes else Sentry.getCurrentScopes()) + .options + .clientReportRecorder + .recordLostEvent(DiscardReason.CALLBACK_ERROR, DataCategory.Span) + } + (if (forceScopes) scopes else Sentry.getCurrentScopes()) + .options + .logger + .log( + SentryLevel.ERROR, + "The beforeSpan callback threw an exception in SentryKtorClientPlugin. Dropping span.", + e, + ) + null + } } if (result == null) { diff --git a/sentry-ktor-client/src/test/java/io/sentry/ktorClient/SentryKtorClientPluginTest.kt b/sentry-ktor-client/src/test/java/io/sentry/ktorClient/SentryKtorClientPluginTest.kt index 38ffe609b2c..4aadfe823a0 100644 --- a/sentry-ktor-client/src/test/java/io/sentry/ktorClient/SentryKtorClientPluginTest.kt +++ b/sentry-ktor-client/src/test/java/io/sentry/ktorClient/SentryKtorClientPluginTest.kt @@ -1,17 +1,21 @@ package io.sentry.ktorClient +import com.google.common.truth.Truth.assertThat import io.ktor.client.HttpClient import io.ktor.client.engine.HttpClientEngine import io.ktor.client.engine.java.Java import io.ktor.client.request.get import io.ktor.client.request.post import io.ktor.client.request.setBody +import io.ktor.client.statement.bodyAsText import io.ktor.http.ContentType import io.ktor.http.contentType import io.sentry.BaggageHeader import io.sentry.Breadcrumb +import io.sentry.DataCategory import io.sentry.Hint import io.sentry.HttpStatusCodeRange +import io.sentry.ILogger import io.sentry.IScope import io.sentry.IScopes import io.sentry.KeyValueCollectionBehavior @@ -19,6 +23,7 @@ import io.sentry.Scope import io.sentry.ScopeCallback import io.sentry.Sentry import io.sentry.SentryEvent +import io.sentry.SentryLevel import io.sentry.SentryOptions import io.sentry.SentryTraceHeader import io.sentry.SentryTracer @@ -26,6 +31,7 @@ import io.sentry.SpanDataConvention import io.sentry.SpanStatus import io.sentry.TransactionContext import io.sentry.W3CTraceparentHeader +import io.sentry.clientreport.DiscardReason import io.sentry.exception.SentryHttpClientException import io.sentry.mockServerRequestTimeoutMillis import java.util.concurrent.TimeUnit @@ -46,6 +52,7 @@ import org.mockito.kotlin.doAnswer import org.mockito.kotlin.mock import org.mockito.kotlin.never import org.mockito.kotlin.verify +import org.mockito.kotlin.verifyNoMoreInteractions import org.mockito.kotlin.whenever class SentryKtorClientPluginTest { @@ -112,6 +119,7 @@ class SentryKtorClientPluginTest { return HttpClient(httpClientEngine) { install(SentryKtorClientPlugin) { this.scopes = this@Fixture.scopes + this.beforeSpan = beforeSpan this.captureFailedRequests = captureFailedRequests this.failedRequestTargets = failedRequestTargets this.failedRequestStatusCodes = failedRequestStatusCodes @@ -434,6 +442,90 @@ class SentryKtorClientPluginTest { ) } + @Test + fun `reports callback errors only for sampled spans`(): Unit = runBlocking { + for (sampled in listOf(true, false, null)) { + val fixture = Fixture() + val onDiscard = mock() + val sut = + fixture.getSut( + beforeSpan = { span, _ -> + span.spanContext.sampled = false + throw IllegalStateException("callback failed") + } + ) + fixture.sentryTracer.spanContext.sampled = sampled + fixture.options.onDiscard = onDiscard + + sut.use { sut.get(fixture.server.url("/hello").toString()) } + fixture.sentryTracer.finish() + + if (sampled == true) { + verify(onDiscard).execute(DiscardReason.CALLBACK_ERROR, DataCategory.Span, 1) + } + verifyNoMoreInteractions(onDiscard) + } + } + + @Test + fun `when beforeSpan throws, drops span and preserves response`(): Unit = runBlocking { + val failure = IllegalStateException("callback failed") + val logger = mock() + val sut = + fixture.getSut( + beforeSpan = { span, _ -> + span.description = "partially modified" + throw failure + } + ) + fixture.options.isDebug = true + fixture.options.setLogger(logger) + sut.use { + val response = sut.get(fixture.server.url("/hello").toString()) + assertThat(response.status.value).isEqualTo(201) + assertThat(response.bodyAsText()).isEqualTo("success") + } + val span = fixture.sentryTracer.children.single() + assertThat(span.isSampled).isFalse() + assertThat(span.isFinished).isTrue() + assertThat(span.status).isEqualTo(SpanStatus.OK) + verify(fixture.scopes).addBreadcrumb(any(), anyOrNull()) + verify(logger) + .log( + SentryLevel.ERROR, + "The beforeSpan callback threw an exception in SentryKtorClientPlugin. Dropping span.", + failure, + ) + } + + @Test + fun `beforeSpan can drop span`(): Unit = runBlocking { + val sut = fixture.getSut(beforeSpan = { _, _ -> null }) + val onDiscard = mock() + fixture.options.onDiscard = onDiscard + fixture.sentryTracer.spanContext.sampled = true + sut.use { sut.get(fixture.server.url("/hello").toString()) } + verifyNoMoreInteractions(onDiscard) + val span = fixture.sentryTracer.children.single() + assertThat(span.isSampled).isFalse() + assertThat(span.isFinished).isTrue() + } + + @Test + fun `beforeSpan can modify span`(): Unit = runBlocking { + val sut = + fixture.getSut( + beforeSpan = { span, _ -> + span.description = "changed" + span + } + ) + sut.use { sut.get(fixture.server.url("/hello").toString()) } + val span = fixture.sentryTracer.children.single() + assertThat(span.description).isEqualTo("changed") + assertThat(span.isFinished).isTrue() + } + @Test fun `creates a span around the request`(): Unit = runBlocking { val sut = fixture.getSut() diff --git a/sentry-okhttp/src/main/java/io/sentry/okhttp/SentryOkHttpInterceptor.kt b/sentry-okhttp/src/main/java/io/sentry/okhttp/SentryOkHttpInterceptor.kt index ed704966610..6a5f6348dd1 100644 --- a/sentry-okhttp/src/main/java/io/sentry/okhttp/SentryOkHttpInterceptor.kt +++ b/sentry-okhttp/src/main/java/io/sentry/okhttp/SentryOkHttpInterceptor.kt @@ -2,6 +2,7 @@ package io.sentry.okhttp import io.sentry.BaggageHeader import io.sentry.Breadcrumb +import io.sentry.DataCategory import io.sentry.Hint import io.sentry.HttpStatusCodeRange import io.sentry.ILogger @@ -9,6 +10,7 @@ import io.sentry.IScopes import io.sentry.ISpan import io.sentry.ScopesAdapter import io.sentry.SentryIntegrationPackageStorage +import io.sentry.SentryLevel import io.sentry.SentryOptions.DEFAULT_PROPAGATION_TARGETS import io.sentry.SentryReplayOptions import io.sentry.SpanDataConvention @@ -16,6 +18,7 @@ import io.sentry.SpanStatus import io.sentry.TypeCheckHint.OKHTTP_REQUEST import io.sentry.TypeCheckHint.OKHTTP_RESPONSE import io.sentry.TypeCheckHint.SENTRY_REPLAY_NETWORK_DETAILS +import io.sentry.clientreport.DiscardReason import io.sentry.okhttp.SentryOkHttpInterceptor.BeforeSpanCallback import io.sentry.transport.CurrentDateProvider import io.sentry.util.IntegrationUtils.addIntegrationToSdkVersion @@ -364,7 +367,25 @@ public open class SentryOkHttpInterceptor( return } if (beforeSpan != null) { - val result = beforeSpan.execute(span, request, response) + val wasSampled = span.isSampled == true + val result = + try { + beforeSpan.execute(span, request, response) + } catch (e: Exception) { + span.spanContext.sampled = false + if (wasSampled) { + scopes.options.clientReportRecorder.recordLostEvent( + DiscardReason.CALLBACK_ERROR, + DataCategory.Span, + ) + } + scopes.options.logger.log( + SentryLevel.ERROR, + "The beforeSpan callback threw an exception in SentryOkHttpInterceptor. Dropping span.", + e, + ) + null + } if (result == null) { // span is dropped span.spanContext.sampled = false diff --git a/sentry-okhttp/src/test/java/io/sentry/okhttp/SentryOkHttpInterceptorTest.kt b/sentry-okhttp/src/test/java/io/sentry/okhttp/SentryOkHttpInterceptorTest.kt index 750406f3d22..a3ba0073f8a 100644 --- a/sentry-okhttp/src/test/java/io/sentry/okhttp/SentryOkHttpInterceptorTest.kt +++ b/sentry-okhttp/src/test/java/io/sentry/okhttp/SentryOkHttpInterceptorTest.kt @@ -2,16 +2,20 @@ package io.sentry.okhttp +import com.google.common.truth.Truth.assertThat import io.sentry.BaggageHeader import io.sentry.Breadcrumb +import io.sentry.DataCategory import io.sentry.Hint import io.sentry.HttpStatusCodeRange +import io.sentry.ILogger import io.sentry.IScope import io.sentry.IScopes import io.sentry.KeyValueCollectionBehavior import io.sentry.Scope import io.sentry.ScopeCallback import io.sentry.Sentry +import io.sentry.SentryLevel import io.sentry.SentryOptions import io.sentry.SentryTraceHeader import io.sentry.SentryTracer @@ -21,6 +25,7 @@ import io.sentry.SpanStatus import io.sentry.TransactionContext import io.sentry.TypeCheckHint import io.sentry.W3CTraceparentHeader +import io.sentry.clientreport.DiscardReason import io.sentry.exception.SentryHttpClientException import io.sentry.mockServerRequestTimeoutMillis import io.sentry.util.network.NetworkRequestData @@ -28,6 +33,7 @@ import java.io.IOException import java.util.concurrent.TimeUnit import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertFailsWith import kotlin.test.assertFalse import kotlin.test.assertNotNull import kotlin.test.assertNull @@ -53,6 +59,7 @@ import org.mockito.kotlin.doAnswer import org.mockito.kotlin.mock import org.mockito.kotlin.never import org.mockito.kotlin.verify +import org.mockito.kotlin.verifyNoMoreInteractions import org.mockito.kotlin.whenever class SentryOkHttpInterceptorTest { @@ -428,12 +435,111 @@ class SentryOkHttpInterceptorTest { @Test fun `customizer can drop the span`() { val sut = fixture.getSut(beforeSpan = { _, _, _ -> null }) + val onDiscard = mock() + fixture.options.onDiscard = onDiscard + fixture.sentryTracer.spanContext.sampled = true sut.newCall(getRequest()).execute() + verifyNoMoreInteractions(onDiscard) val httpClientSpan = fixture.sentryTracer.children.first() assertTrue(httpClientSpan.isFinished) assertNotNull(httpClientSpan.spanContext.sampled) { assertFalse(it) } } + @Test + fun `reports callback errors only for sampled spans`() { + for (sampled in listOf(true, false, null)) { + val fixture = Fixture() + val onDiscard = mock() + val sut = + fixture.getSut( + beforeSpan = { span, _, _ -> + span.spanContext.sampled = false + throw IllegalStateException("callback failed") + } + ) + fixture.sentryTracer.spanContext.sampled = sampled + fixture.options.onDiscard = onDiscard + + sut.newCall(Request.Builder().url(fixture.server.url("/hello")).build()).execute().close() + fixture.sentryTracer.finish() + + if (sampled == true) { + verify(onDiscard).execute(DiscardReason.CALLBACK_ERROR, DataCategory.Span, 1) + } + verifyNoMoreInteractions(onDiscard) + } + } + + @Test + fun `when beforeSpan throws, drops span and preserves response`() { + val failure = IllegalStateException("callback failed") + val logger = mock() + val sut = + fixture.getSut( + beforeSpan = { span, _, _ -> + span.description = "partially modified" + throw failure + } + ) + fixture.options.isDebug = true + fixture.options.setLogger(logger) + + sut.newCall(getRequest()).execute().use { response -> + assertThat(response.code).isEqualTo(201) + assertThat(response.body!!.string()).isEqualTo("success") + } + val span = fixture.sentryTracer.children.single() + assertThat(span.isSampled).isFalse() + assertThat(span.isFinished).isTrue() + verify(fixture.scopes).addBreadcrumb(any(), anyOrNull()) + verify(logger) + .log( + SentryLevel.ERROR, + "The beforeSpan callback threw an exception in SentryOkHttpInterceptor. Dropping span.", + failure, + ) + } + + @Test + fun `when beforeSpan throws, event listener still finishes span`() { + val sut = + fixture.getSut( + beforeSpan = { _, _, _ -> throw IllegalStateException("callback failed") }, + eventListener = SentryOkHttpEventListener(fixture.scopes), + ) + val call = sut.newCall(getRequest()) + call.execute().use { response -> + assertThat(response.code).isEqualTo(201) + assertThat(SentryOkHttpEventListener.eventMap[call]!!.isEventFinished.get()).isTrue() + } + val span = fixture.sentryTracer.children.first { it.operation == "http.client" } + assertThat(span.isSampled).isFalse() + assertThat(span.isFinished).isTrue() + assertThat(SentryOkHttpEventListener.eventMap).doesNotContainKey(call) + } + + @Test + fun `when beforeSpan throws, preserves original request exception`() { + val requestFailure = IOException("request failed") + val sut = + fixture + .getSut( + beforeSpan = { _, _, _ -> + throw IllegalStateException("callback failed") + } + ) + .newBuilder() + .addInterceptor { throw requestFailure } + .build() + + val thrown = assertFailsWith { sut.newCall(getRequest()).execute() } + assertThat(thrown).isSameInstanceAs(requestFailure) + val span = fixture.sentryTracer.children.single() + assertThat(span.throwable).isSameInstanceAs(requestFailure) + assertThat(span.isSampled).isFalse() + assertThat(span.isFinished).isTrue() + } + @Test fun `captures failed requests by default`() { val sut = fixture.getSut(httpStatusCode = 500, captureFailedRequests = null) diff --git a/sentry-openfeign/build.gradle.kts b/sentry-openfeign/build.gradle.kts index 3baa85dee26..8b7adc97544 100644 --- a/sentry-openfeign/build.gradle.kts +++ b/sentry-openfeign/build.gradle.kts @@ -31,6 +31,7 @@ dependencies { testImplementation(libs.awaitility.kotlin) testImplementation(libs.feign.core) testImplementation(libs.kotlin.test.junit) + testImplementation(libs.google.truth) testImplementation(libs.mockito.kotlin) testImplementation(libs.okhttp.mockwebserver) } diff --git a/sentry-openfeign/src/main/java/io/sentry/openfeign/SentryFeignClient.java b/sentry-openfeign/src/main/java/io/sentry/openfeign/SentryFeignClient.java index 520828c0a75..7ed48d0dc10 100644 --- a/sentry-openfeign/src/main/java/io/sentry/openfeign/SentryFeignClient.java +++ b/sentry-openfeign/src/main/java/io/sentry/openfeign/SentryFeignClient.java @@ -10,14 +10,17 @@ import io.sentry.BaggageHeader; import io.sentry.Breadcrumb; import io.sentry.BuildConfig; +import io.sentry.DataCategory; import io.sentry.Hint; import io.sentry.IScopes; import io.sentry.ISpan; import io.sentry.SentryIntegrationPackageStorage; +import io.sentry.SentryLevel; import io.sentry.SpanDataConvention; import io.sentry.SpanOptions; import io.sentry.SpanStatus; import io.sentry.W3CTraceparentHeader; +import io.sentry.clientreport.DiscardReason; import io.sentry.util.Objects; import io.sentry.util.SpanUtils; import io.sentry.util.TracingUtils; @@ -95,7 +98,26 @@ public Response execute(final @NotNull Request request, final @NotNull Request.O throw e; } finally { if (beforeSpan != null) { - final ISpan result = beforeSpan.execute(span, request, response); + final boolean wasSampled = Boolean.TRUE.equals(span.isSampled()); + ISpan result = span; + try { + result = beforeSpan.execute(span, request, response); + } catch (Exception e) { + span.getSpanContext().setSampled(false); + if (wasSampled) { + scopes + .getOptions() + .getClientReportRecorder() + .recordLostEvent(DiscardReason.CALLBACK_ERROR, DataCategory.Span); + } + scopes + .getOptions() + .getLogger() + .log( + SentryLevel.ERROR, + "The beforeSpan callback threw an exception in SentryFeignClient. Dropping span.", + e); + } if (result == null) { // span is dropped diff --git a/sentry-openfeign/src/test/kotlin/io/sentry/openfeign/SentryFeignClientTest.kt b/sentry-openfeign/src/test/kotlin/io/sentry/openfeign/SentryFeignClientTest.kt index c25a81f9501..9a2bd98d365 100644 --- a/sentry-openfeign/src/test/kotlin/io/sentry/openfeign/SentryFeignClientTest.kt +++ b/sentry-openfeign/src/test/kotlin/io/sentry/openfeign/SentryFeignClientTest.kt @@ -1,15 +1,20 @@ package io.sentry.openfeign +import com.google.common.truth.Truth.assertThat import feign.Client import feign.Feign import feign.FeignException import feign.HeaderMap +import feign.Request import feign.RequestLine import io.sentry.BaggageHeader import io.sentry.Breadcrumb +import io.sentry.DataCategory +import io.sentry.ILogger import io.sentry.IScopes import io.sentry.Scope import io.sentry.ScopeCallback +import io.sentry.SentryLevel import io.sentry.SentryOptions import io.sentry.SentryTraceHeader import io.sentry.SentryTracer @@ -17,11 +22,14 @@ import io.sentry.SpanDataConvention import io.sentry.SpanStatus import io.sentry.TransactionContext import io.sentry.W3CTraceparentHeader +import io.sentry.clientreport.DiscardReason import io.sentry.mockServerRequestTimeoutMillis +import java.io.IOException import java.util.concurrent.TimeUnit import kotlin.test.BeforeTest import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertFailsWith import kotlin.test.assertFalse import kotlin.test.assertNotNull import kotlin.test.assertNull @@ -35,6 +43,7 @@ import org.mockito.kotlin.check import org.mockito.kotlin.doAnswer import org.mockito.kotlin.mock import org.mockito.kotlin.verify +import org.mockito.kotlin.verifyNoMoreInteractions import org.mockito.kotlin.whenever class SentryFeignClientTest { @@ -284,6 +293,86 @@ class SentryFeignClientTest { assertTrue(httpClientSpan.throwable is Exception) } + @Test + fun `reports callback errors only for sampled spans`() { + for (sampled in listOf(true, false, null)) { + val fixture = Fixture() + val onDiscard = mock() + val sut = + fixture.getSut( + beforeSpan = { span, _, _ -> + span.spanContext.sampled = false + throw IllegalStateException("callback failed") + } + ) + fixture.sentryTracer.spanContext.sampled = sampled + fixture.sentryOptions.onDiscard = onDiscard + + assertThat(sut.getOk()).isEqualTo("success") + fixture.sentryTracer.finish() + + if (sampled == true) { + verify(onDiscard).execute(DiscardReason.CALLBACK_ERROR, DataCategory.Span, 1) + } + verifyNoMoreInteractions(onDiscard) + } + } + + @Test + fun `when beforeSpan throws, drops span and preserves response`() { + val failure = IllegalStateException("callback failed") + val logger = mock() + fixture.sentryOptions.isDebug = true + fixture.sentryOptions.setLogger(logger) + val sut = + fixture.getSut( + beforeSpan = { span, _, _ -> + span.description = "partially modified" + throw failure + } + ) + + assertThat(sut.getOk()).isEqualTo("success") + val span = fixture.sentryTracer.children.single() + assertThat(span.isSampled).isFalse() + assertThat(span.isFinished).isTrue() + verify(fixture.scopes).addBreadcrumb(any(), anyOrNull()) + verify(logger) + .log( + SentryLevel.ERROR, + "The beforeSpan callback threw an exception in SentryFeignClient. Dropping span.", + failure, + ) + } + + @Test + fun `when beforeSpan throws, preserves original request exception`() { + val requestFailure = IOException("request failed") + val delegate = mock() + whenever(delegate.execute(any(), any())).thenThrow(requestFailure) + whenever(fixture.scopes.span).thenReturn(fixture.sentryTracer) + val sut = + SentryFeignClient(delegate, fixture.scopes) { _, _, _ -> + throw IllegalStateException("callback failed") + } + val request = + Request.create( + Request.HttpMethod.GET, + "https://example.com", + emptyMap>(), + null as ByteArray?, + null, + ) + + val thrown = assertFailsWith { sut.execute(request, Request.Options()) } + assertThat(thrown).isSameInstanceAs(requestFailure) + val span = fixture.sentryTracer.children.single() + assertThat(span.throwable).isSameInstanceAs(requestFailure) + assertThat(span.isSampled).isFalse() + assertThat(span.isFinished).isTrue() + verify(fixture.scopes).addBreadcrumb(any(), anyOrNull()) + } + @Test fun `customizer modifies span`() { val sut = fixture.getSut { span, _, _ -> @@ -310,7 +399,11 @@ class SentryFeignClientTest { @Test fun `customizer can drop the span`() { val sut = fixture.getSut { _, _, _ -> null } + val onDiscard = mock() + fixture.sentryOptions.onDiscard = onDiscard + fixture.sentryTracer.spanContext.sampled = true sut.getOk() + verifyNoMoreInteractions(onDiscard) val httpClientSpan = fixture.sentryTracer.children.first() assertNotNull(httpClientSpan.spanContext.sampled) { assertFalse(it) } }