Merge pull request #6167 from getsentry/fix/callback-error-handling-custom-callbacks

fix(integrations): [Callback Errors 9] Guard custom callbacks

Author: adinauer

CHANGELOG.md

@@ -65,6 +65,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))

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) {

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) {

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<ILogger>()
+    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()) }

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 }

sentry-apollo-3/build.gradle.kts

@@ -34,6 +34,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)

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,13 +218,21 @@ 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) {
             // Span is dropped
             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",

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<SentryOptions.OnDiscardCallback>()
+    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<SentryOptions.OnDiscardCallback>()
+      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<ILogger>()
+    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<TraceContext>(),
         anyOrNull(),
         anyOrNull(),
       )
+    verify(fixture.scopes).addBreadcrumb(any<Breadcrumb>(), anyOrNull())
+    verify(logger)
+      .log(
+        SentryLevel.ERROR,
+        "An error occurred while executing beforeSpan on ApolloInterceptor",
+        failure,
+      )
   }
 
   @Test

sentry-apollo-4/build.gradle.kts

@@ -34,6 +34,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)

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,13 +217,21 @@ 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) {
             // Span is dropped
             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",