From 8845736834a33b83ce97884ee5b1cab9be8e2cd1 Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Fri, 25 Sep 2026 06:45:26 +0200 Subject: [PATCH 1/3] fix(integrations): [Callback Errors 9] Guard custom callbacks Skip Android screenshot and view hierarchy capture when their callbacks throw, while retaining the error event. Drop spans on beforeSpan failures in OkHttp, OpenFeign, GraphQL, Ktor, and Apollo without disrupting requests or replacing the original request error. Preserve normal callback results and existing catch types. Finish failed spans and retain request cleanup and breadcrumbs. Add regression coverage for callback failures, partial mutations, original request errors, asynchronous GraphQL results, and subsequent Android captures. Correct callback wiring in the Ktor and screenshot test fixtures. Verify 18 regression cases fail before the fix and pass afterward; all 454 tests in the affected suites pass, along with formatting and API checks. Refs #6081 Co-Authored-By: Claude --- CHANGELOG.md | 2 + .../core/ScreenshotEventProcessor.java | 12 ++- .../core/ViewHierarchyEventProcessor.java | 12 ++- .../core/ScreenshotEventProcessorTest.kt | 33 ++++++- .../core/ViewHierarchyEventProcessorTest.kt | 27 ++++++ sentry-apollo-3/build.gradle.kts | 1 + .../apollo3/SentryApollo3HttpInterceptor.kt | 1 + .../apollo3/SentryApollo3InterceptorTest.kt | 35 +++++++- sentry-apollo-4/build.gradle.kts | 1 + .../apollo4/SentryApollo4HttpInterceptor.kt | 1 + .../SentryApollo4HttpInterceptorTest.kt | 35 +++++++- sentry-apollo/build.gradle.kts | 1 + .../sentry/apollo/SentryApolloInterceptor.kt | 1 + .../apollo/SentryApolloInterceptorTest.kt | 32 ++++++- sentry-graphql-22/build.gradle.kts | 1 + .../graphql22/SentryInstrumentationTest.kt | 86 ++++++++++++++++++- .../graphql/SentryGraphqlInstrumentation.java | 15 +++- sentry-graphql/build.gradle.kts | 1 + .../graphql/SentryInstrumentationTest.kt | 86 ++++++++++++++++++- sentry-ktor-client/build.gradle.kts | 1 + .../ktorClient/SentryKtorClientPlugin.kt | 16 +++- .../ktorClient/SentryKtorClientPluginTest.kt | 60 +++++++++++++ .../sentry/okhttp/SentryOkHttpInterceptor.kt | 13 ++- .../okhttp/SentryOkHttpInterceptorTest.kt | 74 ++++++++++++++++ sentry-openfeign/build.gradle.kts | 1 + .../sentry/openfeign/SentryFeignClient.java | 15 +++- .../sentry/openfeign/SentryFeignClientTest.kt | 61 +++++++++++++ 27 files changed, 602 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 414585486f6..607d76b6fd8 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. + - Drop spans when `beforeSpan` throws in OkHttp, OpenFeign, GraphQL, Ktor, or Apollo, without disrupting the request. - Skip replay capture when `beforeErrorSampling` throws, while still sending the error event ([#6165](https://github.com/getsentry/sentry-java/pull/6165)) - When `tracesSampler` throws, inherit the parent sampling decision or leave the trace unsampled if there is no parent decision, instead of 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..c276f56db9c 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 @@ -223,6 +223,7 @@ constructor( span.spanContext.sampled = false } } catch (e: Throwable) { + span.spanContext.sampled = false 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..4c43f320e04 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,15 @@ 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.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 @@ -303,16 +306,42 @@ class SentryApollo3InterceptorTest { } @Test - fun `when customizer throws, exception is handled`() { - executeQuery(fixture.getSut(beforeSpan = { _, _, _ -> throw RuntimeException() })) + 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..a75743d4565 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 @@ -222,6 +222,7 @@ constructor( span.spanContext.sampled = false } } catch (e: Throwable) { + span.spanContext.sampled = false 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..e1d6a61a07a 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,15 @@ 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.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 @@ -317,16 +320,42 @@ abstract class SentryApollo4HttpInterceptorTest( } @Test - fun `when customizer throws, exception is handled`() { - executeQuery(fixture.getSut(beforeSpan = { _, _, _ -> throw RuntimeException() })) + 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..52ac48f9e50 100644 --- a/sentry-apollo/src/main/java/io/sentry/apollo/SentryApolloInterceptor.kt +++ b/sentry-apollo/src/main/java/io/sentry/apollo/SentryApolloInterceptor.kt @@ -178,6 +178,7 @@ class SentryApolloInterceptor( try { newSpan = beforeSpan.execute(span, request, response) } catch (e: Exception) { + span.spanContext.sampled = false 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..3e5986ef8a0 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,15 @@ 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.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 @@ -241,16 +244,39 @@ class SentryApolloInterceptorTest { } @Test - fun `when customizer throws, exception is handled`() { - executeQuery(fixture.getSut { _, _, _ -> throw RuntimeException() }) + 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..16dbdd685c3 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,8 +19,10 @@ import graphql.schema.GraphQLScalarType import graphql.schema.idl.RuntimeWiring import graphql.schema.idl.SchemaGenerator import graphql.schema.idl.SchemaParser +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 @@ -29,6 +32,7 @@ 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 +42,7 @@ 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.whenever class SentryInstrumentationTest { @@ -48,6 +53,7 @@ class SentryInstrumentationTest { fun getSut( isTransactionActive: Boolean = true, dataFetcherThrows: Boolean = false, + async: Boolean = false, beforeSpan: SentryGraphqlInstrumentation.BeforeSpanCallback? = null, ): GraphQL { whenever(scopes.options).thenReturn(SentryOptions()) @@ -66,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( @@ -83,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 } } } @@ -165,6 +175,76 @@ class SentryInstrumentationTest { } } + @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) + } + } + @Test fun `beforeSpan can modify spans`() { val sut = 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..5e71b14647b 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 @@ -22,6 +22,7 @@ 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; @@ -283,7 +284,19 @@ private void finish( final @NotNull DataFetchingEnvironment environment, final @Nullable Object result) { if (beforeSpan != null) { - final ISpan newSpan = beforeSpan.execute(span, environment, result); + ISpan newSpan = span; + try { + newSpan = beforeSpan.execute(span, environment, result); + } catch (Exception e) { + span.getSpanContext().setSampled(false); + 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..bc76286abe4 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,16 @@ import graphql.schema.GraphQLScalarType import graphql.schema.idl.RuntimeWiring import graphql.schema.idl.SchemaGenerator import graphql.schema.idl.SchemaParser +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 java.lang.RuntimeException +import java.util.concurrent.CompletableFuture import kotlin.random.Random import kotlin.test.Test import kotlin.test.assertEquals @@ -34,6 +38,7 @@ 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.whenever class SentryInstrumentationTest { @@ -44,6 +49,7 @@ class SentryInstrumentationTest { fun getSut( isTransactionActive: Boolean = true, dataFetcherThrows: Boolean = false, + async: Boolean = false, beforeSpan: SentryGraphqlInstrumentation.BeforeSpanCallback? = null, ): GraphQL { whenever(scopes.options).thenReturn(SentryOptions()) @@ -62,7 +68,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 +88,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 } } } @@ -161,6 +171,76 @@ class SentryInstrumentationTest { } } + @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) + } + } + @Test fun `beforeSpan can modify spans`() { val sut = 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..80e9c3e0ddc 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 @@ -16,6 +16,7 @@ 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.kotlin.SentryContext @@ -186,7 +187,20 @@ public val SentryKtorClientPlugin: ClientPlugin = var result: ISpan? = span if (beforeSpan != null) { - result = beforeSpan.execute(span, request) + result = + try { + beforeSpan.execute(span, request) + } catch (e: Exception) { + (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..5cf1257019f 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,20 @@ 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.Hint import io.sentry.HttpStatusCodeRange +import io.sentry.ILogger import io.sentry.IScope import io.sentry.IScopes import io.sentry.KeyValueCollectionBehavior @@ -19,6 +22,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 @@ -112,6 +116,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 +439,61 @@ class SentryKtorClientPluginTest { ) } + @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 }) + sut.use { sut.get(fixture.server.url("/hello").toString()) } + 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..1adc59386aa 100644 --- a/sentry-okhttp/src/main/java/io/sentry/okhttp/SentryOkHttpInterceptor.kt +++ b/sentry-okhttp/src/main/java/io/sentry/okhttp/SentryOkHttpInterceptor.kt @@ -9,6 +9,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 @@ -364,7 +365,17 @@ public open class SentryOkHttpInterceptor( return } if (beforeSpan != null) { - val result = beforeSpan.execute(span, request, response) + val result = + try { + beforeSpan.execute(span, request, response) + } catch (e: Exception) { + 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..0499b5273dc 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,19 @@ package io.sentry.okhttp +import com.google.common.truth.Truth.assertThat import io.sentry.BaggageHeader import io.sentry.Breadcrumb 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 @@ -28,6 +31,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 @@ -434,6 +438,76 @@ class SentryOkHttpInterceptorTest { assertNotNull(httpClientSpan.spanContext.sampled) { assertFalse(it) } } + @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..4700884c87d 100644 --- a/sentry-openfeign/src/main/java/io/sentry/openfeign/SentryFeignClient.java +++ b/sentry-openfeign/src/main/java/io/sentry/openfeign/SentryFeignClient.java @@ -14,6 +14,7 @@ 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; @@ -95,7 +96,19 @@ 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); + ISpan result = span; + try { + result = beforeSpan.execute(span, request, response); + } catch (Exception e) { + span.getSpanContext().setSampled(false); + 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..d76b0276351 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,19 @@ 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.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 @@ -18,10 +22,12 @@ import io.sentry.SpanStatus import io.sentry.TransactionContext import io.sentry.W3CTraceparentHeader 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 @@ -284,6 +290,61 @@ class SentryFeignClientTest { assertTrue(httpClientSpan.throwable is Exception) } + @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, _, _ -> From 44ed7c6bcf38e335a362ec011df496b5a13eddda Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Fri, 25 Sep 2026 07:00:42 +0200 Subject: [PATCH 2/3] changelog Link the Android capture and integration beforeSpan callback fixes to Callback Errors 9 (#6167). Co-Authored-By: Claude --- CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 607d76b6fd8..6ec43fa46f7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -127,8 +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. - - Drop spans when `beforeSpan` throws in OkHttp, OpenFeign, GraphQL, Ktor, or Apollo, without disrupting the request. + - 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 ([#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, inherit the parent sampling decision or leave the trace unsampled if there is no parent decision, instead of 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)) From 2a38eeb86b23409d6d8ae20d795393be06e7428f Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Fri, 25 Sep 2026 12:21:22 +0200 Subject: [PATCH 3/3] fix(integrations): Report beforeSpan callback losses Record callback_error/span when a throwing beforeSpan callback drops a sampled span in OkHttp, OpenFeign, GraphQL, Ktor, and Apollo. Use the pre-callback sampling decision so partial callback mutations do not hide losses, and leave unsampled spans and intentional null drops uncounted. Add regression coverage for discard notifications, sampling states, and intentional drops while preserving request and span completion behavior. Refs #6167 Co-Authored-By: Claude --- CHANGELOG.md | 2 +- .../apollo3/SentryApollo3HttpInterceptor.kt | 9 +++++ .../apollo3/SentryApollo3InterceptorTest.kt | 32 +++++++++++++++++ .../apollo4/SentryApollo4HttpInterceptor.kt | 9 +++++ .../SentryApollo4HttpInterceptorTest.kt | 32 +++++++++++++++++ .../sentry/apollo/SentryApolloInterceptor.kt | 9 +++++ .../apollo/SentryApolloInterceptorTest.kt | 29 +++++++++++++++ .../graphql22/SentryInstrumentationTest.kt | 36 ++++++++++++++++++- .../graphql/SentryGraphqlInstrumentation.java | 9 +++++ .../graphql/SentryInstrumentationTest.kt | 36 ++++++++++++++++++- .../ktorClient/SentryKtorClientPlugin.kt | 10 ++++++ .../ktorClient/SentryKtorClientPluginTest.kt | 32 +++++++++++++++++ .../sentry/okhttp/SentryOkHttpInterceptor.kt | 10 ++++++ .../okhttp/SentryOkHttpInterceptorTest.kt | 32 +++++++++++++++++ .../sentry/openfeign/SentryFeignClient.java | 9 +++++ .../sentry/openfeign/SentryFeignClientTest.kt | 32 +++++++++++++++++ 16 files changed, 325 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 357c40c30bf..309d16238ce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -128,7 +128,7 @@ - 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 ([#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-apollo-3/src/main/java/io/sentry/apollo3/SentryApollo3HttpInterceptor.kt b/sentry-apollo-3/src/main/java/io/sentry/apollo3/SentryApollo3HttpInterceptor.kt index c276f56db9c..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) { @@ -224,6 +227,12 @@ constructor( } } 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 4c43f320e04..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 @@ -10,6 +10,7 @@ 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 @@ -28,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 @@ -50,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 { @@ -294,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( @@ -305,6 +311,32 @@ class SentryApollo3InterceptorTest { ) } + @Test + 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") 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 a75743d4565..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) { @@ -223,6 +226,12 @@ constructor( } } 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 e1d6a61a07a..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 @@ -13,6 +13,7 @@ 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 @@ -32,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 @@ -55,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 : @@ -308,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( @@ -319,6 +325,32 @@ abstract class SentryApollo4HttpInterceptorTest( ) } + @Test + 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") 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 52ac48f9e50..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,10 +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 3e5986ef8a0..bc09f86a4e6 100644 --- a/sentry-apollo/src/test/java/io/sentry/apollo/SentryApolloInterceptorTest.kt +++ b/sentry-apollo/src/test/java/io/sentry/apollo/SentryApolloInterceptorTest.kt @@ -6,6 +6,7 @@ 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 @@ -20,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 @@ -42,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 { @@ -232,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( @@ -243,6 +249,29 @@ class SentryApolloInterceptorTest { ) } + @Test + 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") 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 16dbdd685c3..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 @@ -19,6 +19,7 @@ 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 @@ -27,6 +28,7 @@ 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 @@ -43,6 +45,7 @@ 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 { @@ -56,7 +59,8 @@ class SentryInstrumentationTest { 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 = """ @@ -162,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 } }") @@ -172,6 +179,33 @@ 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) } } 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 5e71b14647b..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,6 +17,7 @@ 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; @@ -26,6 +27,7 @@ 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; @@ -284,11 +286,18 @@ private void finish( final @NotNull DataFetchingEnvironment environment, final @Nullable Object result) { if (beforeSpan != null) { + 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() 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 bc76286abe4..2652694b8f9 100644 --- a/sentry-graphql/src/test/kotlin/io/sentry/graphql/SentryInstrumentationTest.kt +++ b/sentry-graphql/src/test/kotlin/io/sentry/graphql/SentryInstrumentationTest.kt @@ -19,6 +19,7 @@ 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 @@ -27,6 +28,7 @@ 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 @@ -39,6 +41,7 @@ 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 { @@ -52,7 +55,8 @@ class SentryInstrumentationTest { 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 = """ @@ -158,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 } }") @@ -168,6 +175,33 @@ 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) } } 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 80e9c3e0ddc..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 @@ -19,6 +20,7 @@ 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 @@ -187,10 +189,18 @@ public val SentryKtorClientPlugin: ClientPlugin = var result: ISpan? = span if (beforeSpan != null) { + 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 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 5cf1257019f..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 @@ -12,6 +12,7 @@ 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 @@ -30,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 @@ -50,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 { @@ -439,6 +442,31 @@ 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") @@ -473,7 +501,11 @@ class SentryKtorClientPluginTest { @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() 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 1adc59386aa..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 @@ -17,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 @@ -365,10 +367,18 @@ public open class SentryOkHttpInterceptor( return } if (beforeSpan != null) { + 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.", 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 0499b5273dc..a3ba0073f8a 100644 --- a/sentry-okhttp/src/test/java/io/sentry/okhttp/SentryOkHttpInterceptorTest.kt +++ b/sentry-okhttp/src/test/java/io/sentry/okhttp/SentryOkHttpInterceptorTest.kt @@ -5,6 +5,7 @@ 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 @@ -24,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 @@ -57,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 { @@ -432,12 +435,41 @@ 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") 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 4700884c87d..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,6 +10,7 @@ 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; @@ -19,6 +20,7 @@ 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; @@ -96,11 +98,18 @@ public Response execute(final @NotNull Request request, final @NotNull Request.O throw e; } finally { if (beforeSpan != null) { + 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() 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 d76b0276351..9a2bd98d365 100644 --- a/sentry-openfeign/src/test/kotlin/io/sentry/openfeign/SentryFeignClientTest.kt +++ b/sentry-openfeign/src/test/kotlin/io/sentry/openfeign/SentryFeignClientTest.kt @@ -9,6 +9,7 @@ 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 @@ -21,6 +22,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.mockServerRequestTimeoutMillis import java.io.IOException import java.util.concurrent.TimeUnit @@ -41,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 { @@ -290,6 +293,31 @@ 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") @@ -371,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) } }