Skip to content

Commit 2afa056

Browse files
adinaueradinauer
authored
Merge branch 'fix/callback-error-handling' into fix/callback-error-handling-user-feedback
2 parents 08377ad + 84e8b86 commit 2afa056

9 files changed

Lines changed: 260 additions & 12 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@
55
### Fixes
66

77
- Keep the Android feedback form available when an options configurator throws ([#6239](https://github.com/getsentry/sentry-java/pull/6239))
8+
- Keep Spring requests running when a `SentryUserProvider` throws and discard the incomplete user identity ([#6240](https://github.com/getsentry/sentry-java/pull/6240))
9+
- Drop Apollo 5 spans when `beforeSpan` throws without disrupting the GraphQL request ([#6238](https://github.com/getsentry/sentry-java/pull/6238))
810

911
### Features
1012

‎sentry-apollo-5/src/main/java/io/sentry/apollo5/SentryApollo5HttpInterceptor.kt‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import com.apollographql.apollo.network.http.HttpInterceptor
88
import com.apollographql.apollo.network.http.HttpInterceptorChain
99
import io.sentry.BaggageHeader
1010
import io.sentry.Breadcrumb
11+
import io.sentry.DataCategory
1112
import io.sentry.Hint
1213
import io.sentry.IScopes
1314
import io.sentry.ISpan
@@ -21,6 +22,7 @@ import io.sentry.SpanDataConvention.HTTP_METHOD_KEY
2122
import io.sentry.SpanStatus
2223
import io.sentry.TypeCheckHint.APOLLO_REQUEST
2324
import io.sentry.TypeCheckHint.APOLLO_RESPONSE
25+
import io.sentry.clientreport.DiscardReason
2426
import io.sentry.exception.ExceptionMechanismException
2527
import io.sentry.protocol.Mechanism
2628
import io.sentry.protocol.Request
@@ -199,6 +201,7 @@ constructor(
199201
span.setData(SpanDataConvention.HTTP_RESPONSE_CONTENT_LENGTH_KEY, it)
200202
}
201203
if (beforeSpan != null) {
204+
val wasSampled = span.isSampled == true
202205
try {
203206
val result = beforeSpan.execute(span, request, response)
204207
if (result == null) {
@@ -207,6 +210,13 @@ constructor(
207210
}
208211
} catch (e: Throwable) {
209212
ExceptionUtils.rethrowIfFatal(e)
213+
span.spanContext.sampled = false
214+
if (wasSampled) {
215+
scopes.options.clientReportRecorder.recordLostEvent(
216+
DiscardReason.CALLBACK_ERROR,
217+
DataCategory.Span,
218+
)
219+
}
210220
scopes.options.logger.log(
211221
SentryLevel.ERROR,
212222
"An error occurred while executing beforeSpan in ApolloInterceptor",

‎sentry-apollo-5/src/test/java/io/sentry/apollo5/SentryApollo5HttpInterceptorTest.kt‎

Lines changed: 74 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import com.apollographql.apollo.network.http.HttpInterceptorChain
1414
import com.google.common.truth.Truth.assertThat
1515
import io.sentry.BaggageHeader
1616
import io.sentry.Breadcrumb
17+
import io.sentry.DataCategory
1718
import io.sentry.Hint
1819
import io.sentry.IScopes
1920
import io.sentry.ITransaction
@@ -32,6 +33,7 @@ import io.sentry.TransactionContext
3233
import io.sentry.W3CTraceparentHeader
3334
import io.sentry.apollo5.SentryApollo5HttpInterceptor.BeforeSpanCallback
3435
import io.sentry.apollo5.generated.LaunchDetailsQuery
36+
import io.sentry.clientreport.DiscardReason
3537
import io.sentry.mockServerRequestTimeoutMillis
3638
import io.sentry.protocol.SdkVersion
3739
import io.sentry.protocol.SentryTransaction
@@ -40,6 +42,7 @@ import java.util.concurrent.TimeUnit
4042
import kotlin.reflect.KSuspendFunction1
4143
import kotlin.test.Test
4244
import kotlin.test.assertEquals
45+
import kotlin.test.assertFailsWith
4346
import kotlin.test.assertNotNull
4447
import kotlin.test.assertNull
4548
import kotlin.test.assertTrue
@@ -60,6 +63,7 @@ import org.mockito.kotlin.doAnswer
6063
import org.mockito.kotlin.mock
6164
import org.mockito.kotlin.never
6265
import org.mockito.kotlin.verify
66+
import org.mockito.kotlin.verifyNoMoreInteractions
6367
import org.mockito.kotlin.whenever
6468

6569
class SentryApollo5HttpInterceptorTestWithV5Implementation :
@@ -421,16 +425,83 @@ abstract class SentryApollo5HttpInterceptorTest(
421425
}
422426

423427
@Test
424-
fun `when customizer throws, exception is handled`() {
425-
executeQuery(fixture.getSut(beforeSpan = { _, _, _ -> throw RuntimeException() }))
428+
fun `reports callback errors only for sampled spans`(): Unit = runBlocking {
429+
for (sampled in listOf(true, false, null)) {
430+
val onDiscard = mock<SentryOptions.OnDiscardCallback>()
431+
fixture.options.onDiscard = onDiscard
432+
val tx = SentryTracer(TransactionContext("op", "desc"), fixture.scopes)
433+
tx.spanContext.sampled = sampled
434+
whenever(fixture.scopes.span).thenReturn(tx)
435+
val sut =
436+
fixture.getSut(
437+
beforeSpan = { span, _, _ ->
438+
span.spanContext.sampled = false
439+
throw IllegalStateException("callback failed")
440+
}
441+
)
442+
443+
assertThat(executeQueryImplementation(sut.query(LaunchDetailsQuery("83"))).data).isNotNull()
444+
tx.finish()
445+
446+
if (sampled == true) {
447+
verify(onDiscard).execute(DiscardReason.CALLBACK_ERROR, DataCategory.Span, 1)
448+
}
449+
verifyNoMoreInteractions(onDiscard)
450+
}
451+
}
426452

453+
@Test
454+
fun `when beforeSpan throws, drops span and preserves response`(): Unit = runBlocking {
455+
val failure = IllegalStateException("callback failed")
456+
val tx =
457+
SentryTracer(TransactionContext("op", "desc", TracesSamplingDecision(true)), fixture.scopes)
458+
whenever(fixture.scopes.span).thenReturn(tx)
459+
val sut =
460+
fixture.getSut(
461+
beforeSpan = { span, _, _ ->
462+
span.description = "partially modified"
463+
throw failure
464+
}
465+
)
466+
467+
val response = executeQueryImplementation(sut.query(LaunchDetailsQuery("83")))
468+
assertThat(response.data).isNotNull()
469+
val span = tx.children.single()
470+
assertThat(span.isSampled).isFalse()
471+
assertThat(span.isFinished).isTrue()
472+
tx.finish()
427473
verify(fixture.scopes)
428474
.captureTransaction(
429-
check { assertEquals(1, it.spans.size) },
475+
check { assertThat(it.spans).isEmpty() },
430476
anyOrNull<TraceContext>(),
431477
anyOrNull(),
432478
anyOrNull(),
433479
)
480+
verify(fixture.scopes).addBreadcrumb(any<Breadcrumb>(), anyOrNull())
481+
}
482+
483+
@Test
484+
fun `fatal error from beforeSpan is not swallowed`(): Unit = runBlocking {
485+
val failure = OutOfMemoryError("callback failed")
486+
val tx =
487+
SentryTracer(TransactionContext("op", "desc", TracesSamplingDecision(true)), fixture.scopes)
488+
whenever(fixture.scopes.span).thenReturn(tx)
489+
val request = HttpRequest.Builder(HttpMethod.Post, "https://example.com/graphql").build()
490+
val response = HttpResponse.Builder(200).build()
491+
val chain =
492+
object : HttpInterceptorChain {
493+
override suspend fun proceed(request: HttpRequest): HttpResponse = response
494+
}
495+
val interceptor =
496+
SentryApollo5HttpInterceptor(
497+
fixture.scopes,
498+
beforeSpan = { _, _, _ -> throw failure },
499+
captureFailedRequests = false,
500+
)
501+
502+
val thrown = assertFailsWith<OutOfMemoryError> { interceptor.intercept(request, chain) }
503+
504+
assertThat(thrown).isSameInstanceAs(failure)
434505
}
435506

436507
@Test

‎sentry-spring-7/src/main/java/io/sentry/spring7/SentryUserFilter.java‎

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,9 @@
44
import io.sentry.IScope;
55
import io.sentry.IScopes;
66
import io.sentry.IpAddressUtils;
7+
import io.sentry.SentryLevel;
78
import io.sentry.protocol.User;
9+
import io.sentry.util.ExceptionUtils;
810
import io.sentry.util.Objects;
911
import jakarta.servlet.FilterChain;
1012
import jakarta.servlet.ServletException;
@@ -43,16 +45,30 @@ protected void doFilterInternal(
4345
final @NotNull FilterChain chain)
4446
throws ServletException, IOException {
4547
final User user = new User();
48+
boolean providerFailed = false;
4649
for (final SentryUserProvider provider : sentryUserProviders) {
47-
apply(user, provider.provideUser());
50+
try {
51+
apply(user, provider.provideUser());
52+
} catch (Throwable e) {
53+
ExceptionUtils.rethrowIfFatal(e);
54+
providerFailed = true;
55+
scopes
56+
.getOptions()
57+
.getLogger()
58+
.log(
59+
SentryLevel.ERROR,
60+
e,
61+
"The SentryUserProvider callback %s threw an exception.",
62+
provider.getClass().getName());
63+
}
4864
}
49-
if (scopes.getOptions().getDataCollectionResolver().isUserInfo()) {
65+
if (!providerFailed && scopes.getOptions().getDataCollectionResolver().isUserInfo()) {
5066
if (IpAddressUtils.isDefault(user.getIpAddress())) {
5167
// unset {{auto}} as it would set the server's ip address as a user ip address
5268
user.setIpAddress(null);
5369
}
5470
}
55-
scopes.setUser(user);
71+
scopes.setUser(providerFailed ? new User() : user);
5672
chain.doFilter(request, response);
5773
}
5874

‎sentry-spring-7/src/test/kotlin/io/sentry/spring7/SentryUserFilterTest.kt‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,12 @@ import io.sentry.protocol.User
66
import jakarta.servlet.FilterChain
77
import kotlin.test.Test
88
import kotlin.test.assertEquals
9+
import kotlin.test.assertFailsWith
910
import kotlin.test.assertNull
11+
import org.assertj.core.api.Assertions.assertThat
1012
import org.mockito.kotlin.check
1113
import org.mockito.kotlin.mock
14+
import org.mockito.kotlin.never
1215
import org.mockito.kotlin.verify
1316
import org.mockito.kotlin.whenever
1417
import org.springframework.mock.web.MockHttpServletRequest
@@ -151,6 +154,42 @@ class SentryUserFilterTest {
151154
verify(fixture.scopes).setUser(check { assertNull(it.ipAddress) })
152155
}
153156

157+
@Test
158+
fun `provider failure discards user data while later providers and request continue`() {
159+
val failure = RuntimeException("provider failed")
160+
val laterProvider = mock<SentryUserProvider>()
161+
whenever(laterProvider.provideUser()).thenReturn(sampleUser)
162+
val filter =
163+
fixture.getSut(
164+
userProviders =
165+
listOf(
166+
SentryUserProvider { sampleUser },
167+
SentryUserProvider { throw failure },
168+
laterProvider,
169+
)
170+
)
171+
172+
filter.doFilter(fixture.request, fixture.response, fixture.chain)
173+
174+
verify(fixture.scopes).setUser(check { assertEquals(User(), it) })
175+
verify(laterProvider).provideUser()
176+
verify(fixture.chain).doFilter(fixture.request, fixture.response)
177+
}
178+
179+
@Test
180+
fun `fatal provider failure propagates`() {
181+
val failure = OutOfMemoryError("fatal")
182+
val filter = fixture.getSut(userProviders = listOf(SentryUserProvider { throw failure }))
183+
184+
assertThat(
185+
assertFailsWith<OutOfMemoryError> {
186+
filter.doFilter(fixture.request, fixture.response, fixture.chain)
187+
}
188+
)
189+
.isSameAs(failure)
190+
verify(fixture.chain, never()).doFilter(fixture.request, fixture.response)
191+
}
192+
154193
private fun assertEquals(user1: User, user2: User) {
155194
assertEquals(user1.username, user2.username)
156195
assertEquals(user1.id, user2.id)

‎sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/SentryUserFilter.java‎

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,9 @@
44
import io.sentry.IScope;
55
import io.sentry.IScopes;
66
import io.sentry.IpAddressUtils;
7+
import io.sentry.SentryLevel;
78
import io.sentry.protocol.User;
9+
import io.sentry.util.ExceptionUtils;
810
import io.sentry.util.Objects;
911
import jakarta.servlet.FilterChain;
1012
import jakarta.servlet.ServletException;
@@ -43,16 +45,30 @@ protected void doFilterInternal(
4345
final @NotNull FilterChain chain)
4446
throws ServletException, IOException {
4547
final User user = new User();
48+
boolean providerFailed = false;
4649
for (final SentryUserProvider provider : sentryUserProviders) {
47-
apply(user, provider.provideUser());
50+
try {
51+
apply(user, provider.provideUser());
52+
} catch (Throwable e) {
53+
ExceptionUtils.rethrowIfFatal(e);
54+
providerFailed = true;
55+
scopes
56+
.getOptions()
57+
.getLogger()
58+
.log(
59+
SentryLevel.ERROR,
60+
e,
61+
"The SentryUserProvider callback %s threw an exception.",
62+
provider.getClass().getName());
63+
}
4864
}
49-
if (scopes.getOptions().getDataCollectionResolver().isUserInfo()) {
65+
if (!providerFailed && scopes.getOptions().getDataCollectionResolver().isUserInfo()) {
5066
if (IpAddressUtils.isDefault(user.getIpAddress())) {
5167
// unset {{auto}} as it would set the server's ip address as a user ip address
5268
user.setIpAddress(null);
5369
}
5470
}
55-
scopes.setUser(user);
71+
scopes.setUser(providerFailed ? new User() : user);
5672
chain.doFilter(request, response);
5773
}
5874

‎sentry-spring-jakarta/src/test/kotlin/io/sentry/spring/jakarta/SentryUserFilterTest.kt‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,12 @@ import io.sentry.protocol.User
66
import jakarta.servlet.FilterChain
77
import kotlin.test.Test
88
import kotlin.test.assertEquals
9+
import kotlin.test.assertFailsWith
910
import kotlin.test.assertNull
11+
import org.assertj.core.api.Assertions.assertThat
1012
import org.mockito.kotlin.check
1113
import org.mockito.kotlin.mock
14+
import org.mockito.kotlin.never
1215
import org.mockito.kotlin.verify
1316
import org.mockito.kotlin.whenever
1417
import org.springframework.mock.web.MockHttpServletRequest
@@ -151,6 +154,42 @@ class SentryUserFilterTest {
151154
verify(fixture.scopes).setUser(check { assertNull(it.ipAddress) })
152155
}
153156

157+
@Test
158+
fun `provider failure discards user data while later providers and request continue`() {
159+
val failure = RuntimeException("provider failed")
160+
val laterProvider = mock<SentryUserProvider>()
161+
whenever(laterProvider.provideUser()).thenReturn(sampleUser)
162+
val filter =
163+
fixture.getSut(
164+
userProviders =
165+
listOf(
166+
SentryUserProvider { sampleUser },
167+
SentryUserProvider { throw failure },
168+
laterProvider,
169+
)
170+
)
171+
172+
filter.doFilter(fixture.request, fixture.response, fixture.chain)
173+
174+
verify(fixture.scopes).setUser(check { assertEquals(User(), it) })
175+
verify(laterProvider).provideUser()
176+
verify(fixture.chain).doFilter(fixture.request, fixture.response)
177+
}
178+
179+
@Test
180+
fun `fatal provider failure propagates`() {
181+
val failure = OutOfMemoryError("fatal")
182+
val filter = fixture.getSut(userProviders = listOf(SentryUserProvider { throw failure }))
183+
184+
assertThat(
185+
assertFailsWith<OutOfMemoryError> {
186+
filter.doFilter(fixture.request, fixture.response, fixture.chain)
187+
}
188+
)
189+
.isSameAs(failure)
190+
verify(fixture.chain, never()).doFilter(fixture.request, fixture.response)
191+
}
192+
154193
private fun assertEquals(user1: User, user2: User) {
155194
assertEquals(user1.username, user2.username)
156195
assertEquals(user1.id, user2.id)

0 commit comments

Comments
 (0)