Skip to content

Commit 6dffaa7

Browse files
adinaueradinauer
authored
Merge pull request #6241 from getsentry/fix/callback-error-handling-spring-transaction-name-provider
fix(spring): Handle transaction name provider failures
2 parents 155fa5a + bec3873 commit 6dffaa7

37 files changed

Lines changed: 888 additions & 53 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
### Fixes
66

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

‎sentry-spring-7/api/sentry-spring-7.api‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,11 @@ public final class io/sentry/spring7/SpringSecuritySentryUserProvider : io/sentr
104104
public fun provideUser ()Lio/sentry/protocol/User;
105105
}
106106

107+
public final class io/sentry/spring7/TransactionNameProviderUtils {
108+
public static fun provideTransactionName (Lio/sentry/spring7/tracing/TransactionNameProvider;Ljakarta/servlet/http/HttpServletRequest;Lio/sentry/ILogger;)Ljava/lang/String;
109+
public static fun provideTransactionNameAndSource (Lio/sentry/spring7/tracing/TransactionNameProvider;Ljakarta/servlet/http/HttpServletRequest;Lio/sentry/ILogger;Lio/sentry/spring7/tracing/TransactionNameWithSource;)Lio/sentry/spring7/tracing/TransactionNameWithSource;
110+
}
111+
107112
public final class io/sentry/spring7/cache/SentryCacheBeanPostProcessor : org/springframework/beans/factory/config/BeanPostProcessor, org/springframework/core/PriorityOrdered {
108113
public fun <init> ()V
109114
public fun getOrder ()I

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,9 @@ protected SentryEvent createEvent(
7575
new ExceptionMechanismException(mechanism, ex, Thread.currentThread());
7676
final SentryEvent event = new SentryEvent(throwable);
7777
event.setLevel(SentryLevel.FATAL);
78-
event.setTransaction(transactionNameProvider.provideTransactionName(request));
78+
event.setTransaction(
79+
TransactionNameProviderUtils.provideTransactionName(
80+
transactionNameProvider, request, scopes.getOptions().getLogger()));
7981

8082
return event;
8183
}

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

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@
22

33
import com.jakewharton.nopen.annotation.Open;
44
import io.sentry.Hint;
5+
import io.sentry.ILogger;
6+
import io.sentry.ScopesAdapter;
57
import io.sentry.SentryEvent;
68
import io.sentry.internal.eventprocessor.SentryEventProcessor;
79
import io.sentry.spring7.tracing.TransactionNameProvider;
@@ -15,19 +17,30 @@
1517
public class SentryRequestHttpServletRequestProcessor implements SentryEventProcessor {
1618
private final @NotNull TransactionNameProvider transactionNameProvider;
1719
private final @NotNull HttpServletRequest request;
20+
private final @NotNull ILogger logger;
1821

1922
public SentryRequestHttpServletRequestProcessor(
2023
final @NotNull TransactionNameProvider transactionNameProvider,
2124
final @NotNull HttpServletRequest request) {
25+
this(transactionNameProvider, request, ScopesAdapter.getInstance().getOptions().getLogger());
26+
}
27+
28+
SentryRequestHttpServletRequestProcessor(
29+
final @NotNull TransactionNameProvider transactionNameProvider,
30+
final @NotNull HttpServletRequest request,
31+
final @NotNull ILogger logger) {
2232
this.transactionNameProvider =
2333
Objects.requireNonNull(transactionNameProvider, "transactionNameProvider is required");
2434
this.request = Objects.requireNonNull(request, "request is required");
35+
this.logger = Objects.requireNonNull(logger, "logger is required");
2536
}
2637

2738
@Override
2839
public @NotNull SentryEvent process(final @NotNull SentryEvent event, final @NotNull Hint hint) {
2940
if (event.getTransaction() == null) {
30-
event.setTransaction(transactionNameProvider.provideTransactionName(request));
41+
event.setTransaction(
42+
TransactionNameProviderUtils.provideTransactionName(
43+
transactionNameProvider, request, logger));
3144
}
3245
return event;
3346
}

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,8 @@ private void configureScope(
9191
// transaction name is known at the later stage of request processing, thus it cannot
9292
// be set on the scope
9393
scope.addEventProcessor(
94-
new SentryRequestHttpServletRequestProcessor(transactionNameProvider, request));
94+
new SentryRequestHttpServletRequestProcessor(
95+
transactionNameProvider, request, scopes.getOptions().getLogger()));
9596
// only if request caches body, add an event processor that sets body on the event
9697
// body is not on the scope, to avoid using memory when no event is triggered during
9798
// request processing

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

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
package io.sentry.spring7;
2+
3+
import static io.sentry.TransactionContext.DEFAULT_TRANSACTION_NAME;
4+
5+
import io.sentry.ILogger;
6+
import io.sentry.SentryLevel;
7+
import io.sentry.spring7.tracing.TransactionNameProvider;
8+
import io.sentry.spring7.tracing.TransactionNameWithSource;
9+
import io.sentry.util.ExceptionUtils;
10+
import jakarta.servlet.http.HttpServletRequest;
11+
import org.jetbrains.annotations.ApiStatus;
12+
import org.jetbrains.annotations.NotNull;
13+
import org.jetbrains.annotations.Nullable;
14+
15+
@ApiStatus.Internal
16+
public final class TransactionNameProviderUtils {
17+
private TransactionNameProviderUtils() {}
18+
19+
public static @Nullable String provideTransactionName(
20+
final @NotNull TransactionNameProvider provider,
21+
final @NotNull HttpServletRequest request,
22+
final @NotNull ILogger logger) {
23+
try {
24+
return provider.provideTransactionName(request);
25+
} catch (Throwable e) {
26+
ExceptionUtils.rethrowIfFatal(e);
27+
logCallbackError(logger, e);
28+
return DEFAULT_TRANSACTION_NAME;
29+
}
30+
}
31+
32+
public static @NotNull TransactionNameWithSource provideTransactionNameAndSource(
33+
final @NotNull TransactionNameProvider provider,
34+
final @NotNull HttpServletRequest request,
35+
final @NotNull ILogger logger,
36+
final @NotNull TransactionNameWithSource fallbackOnError) {
37+
try {
38+
return provider.provideTransactionNameAndSource(request);
39+
} catch (Throwable e) {
40+
ExceptionUtils.rethrowIfFatal(e);
41+
logCallbackError(logger, e);
42+
return fallbackOnError;
43+
}
44+
}
45+
46+
private static void logCallbackError(
47+
final @NotNull ILogger logger, final @NotNull Throwable throwable) {
48+
logger.log(
49+
SentryLevel.ERROR, "The TransactionNameProvider callback threw an exception.", throwable);
50+
}
51+
}

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

Lines changed: 42 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,11 @@
11
package io.sentry.spring7.tracing;
22

3+
import static io.sentry.TransactionContext.DEFAULT_TRANSACTION_NAME;
4+
5+
import io.sentry.ScopesAdapter;
6+
import io.sentry.SentryLevel;
37
import io.sentry.protocol.TransactionNameSource;
8+
import io.sentry.util.ExceptionUtils;
49
import jakarta.servlet.http.HttpServletRequest;
510
import java.util.List;
611
import org.jetbrains.annotations.ApiStatus;
@@ -21,15 +26,21 @@ public CombinedTransactionNameProvider(final @NotNull List<TransactionNameProvid
2126
}
2227

2328
@Override
24-
public @Nullable String provideTransactionName(@NotNull HttpServletRequest request) {
25-
for (TransactionNameProvider provider : providers) {
26-
String transactionName = provider.provideTransactionName(request);
27-
if (transactionName != null) {
28-
return transactionName;
29+
public @Nullable String provideTransactionName(final @NotNull HttpServletRequest request) {
30+
boolean callbackFailed = false;
31+
for (final TransactionNameProvider provider : providers) {
32+
try {
33+
final @Nullable String transactionName = provider.provideTransactionName(request);
34+
if (transactionName != null) {
35+
return transactionName;
36+
}
37+
} catch (Throwable e) {
38+
ExceptionUtils.rethrowIfFatal(e);
39+
logCallbackError(e);
40+
callbackFailed = true;
2941
}
3042
}
31-
32-
return null;
43+
return callbackFailed ? DEFAULT_TRANSACTION_NAME : null;
3344
}
3445

3546
@Override
@@ -42,14 +53,32 @@ public CombinedTransactionNameProvider(final @NotNull List<TransactionNameProvid
4253
@Override
4354
public @NotNull TransactionNameWithSource provideTransactionNameAndSource(
4455
@NotNull HttpServletRequest request) {
45-
for (TransactionNameProvider provider : providers) {
46-
String transactionName = provider.provideTransactionName(request);
47-
if (transactionName != null) {
48-
final @NotNull TransactionNameSource source = provider.provideTransactionSource();
49-
return new TransactionNameWithSource(transactionName, source);
56+
boolean callbackFailed = false;
57+
for (final TransactionNameProvider provider : providers) {
58+
try {
59+
final @Nullable String transactionName = provider.provideTransactionName(request);
60+
if (transactionName != null) {
61+
return new TransactionNameWithSource(
62+
transactionName, provider.provideTransactionSource());
63+
}
64+
} catch (Throwable e) {
65+
ExceptionUtils.rethrowIfFatal(e);
66+
logCallbackError(e);
67+
callbackFailed = true;
5068
}
5169
}
70+
return callbackFailed
71+
? new TransactionNameWithSource(DEFAULT_TRANSACTION_NAME, TransactionNameSource.CUSTOM)
72+
: new TransactionNameWithSource(null, TransactionNameSource.CUSTOM);
73+
}
5274

53-
return new TransactionNameWithSource(null, TransactionNameSource.CUSTOM);
75+
private static void logCallbackError(final @NotNull Throwable throwable) {
76+
ScopesAdapter.getInstance()
77+
.getOptions()
78+
.getLogger()
79+
.log(
80+
SentryLevel.ERROR,
81+
"The TransactionNameProvider callback threw an exception.",
82+
throwable);
5483
}
5584
}

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

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
package io.sentry.spring7.tracing;
22

3+
import static io.sentry.TransactionContext.DEFAULT_TRANSACTION_NAME;
4+
35
import com.jakewharton.nopen.annotation.Open;
46
import io.sentry.BaggageHeader;
57
import io.sentry.CustomSamplingContext;
@@ -11,6 +13,7 @@
1113
import io.sentry.TransactionContext;
1214
import io.sentry.TransactionOptions;
1315
import io.sentry.protocol.TransactionNameSource;
16+
import io.sentry.spring7.TransactionNameProviderUtils;
1417
import io.sentry.util.Objects;
1518
import io.sentry.util.SpanUtils;
1619
import jakarta.servlet.FilterChain;
@@ -38,6 +41,8 @@ public class SentryTracingFilter extends OncePerRequestFilter {
3841

3942
private static final String TRACE_ORIGIN = "auto.http.spring7.webmvc";
4043
private static final String TRANSACTION_ATTR = "sentry.transaction";
44+
private static final @NotNull TransactionNameWithSource CALLBACK_ERROR_TRANSACTION_NAME =
45+
new TransactionNameWithSource(DEFAULT_TRANSACTION_NAME, TransactionNameSource.CUSTOM);
4146

4247
private final @NotNull TransactionNameProvider transactionNameProvider;
4348
private final @NotNull IScopes scopes;
@@ -146,7 +151,11 @@ private void doFilterWithTransaction(
146151
if (shouldFinishTransaction(httpRequest) && transaction != null) {
147152
// after all filters run, templated path pattern is available in request attribute
148153
final @NotNull TransactionNameWithSource transactionNameWithSource =
149-
transactionNameProvider.provideTransactionNameAndSource(httpRequest);
154+
TransactionNameProviderUtils.provideTransactionNameAndSource(
155+
transactionNameProvider,
156+
httpRequest,
157+
scopes.getOptions().getLogger(),
158+
CALLBACK_ERROR_TRANSACTION_NAME);
150159
final @Nullable String transactionName = transactionNameWithSource.getTransactionName();
151160
final @NotNull TransactionNameSource transactionNameSource =
152161
transactionNameWithSource.getTransactionNameSource();

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

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ import io.sentry.Hint
44
import io.sentry.IScopes
55
import io.sentry.SentryEvent
66
import io.sentry.SentryLevel
7+
import io.sentry.SentryOptions
8+
import io.sentry.TransactionContext.DEFAULT_TRANSACTION_NAME
79
import io.sentry.exception.ExceptionMechanismException
810
import io.sentry.spring7.tracing.TransactionNameProvider
911
import jakarta.servlet.http.HttpServletRequest
@@ -19,10 +21,15 @@ import org.mockito.kotlin.whenever
1921
class SentryExceptionResolverTest {
2022
private val scopes = mock<IScopes>()
2123
private val transactionNameProvider = mock<TransactionNameProvider>()
24+
private val options = SentryOptions()
2225

2326
private val request = mock<HttpServletRequest>()
2427
private val response = mock<HttpServletResponse>()
2528

29+
init {
30+
whenever(scopes.options).thenReturn(options)
31+
}
32+
2633
@Test
2734
fun `when handles exception, sets wrapped exception for event`() {
2835
val eventCaptor = argumentCaptor<SentryEvent>()
@@ -70,6 +77,19 @@ class SentryExceptionResolverTest {
7077
verify(transactionNameProvider).provideTransactionName(request)
7178
}
7279

80+
@Test
81+
fun `when transaction name provider fails, captures event with safe fallback`() {
82+
val failure = RuntimeException("provider failed")
83+
whenever(transactionNameProvider.provideTransactionName(any())).thenThrow(failure)
84+
val eventCaptor = argumentCaptor<SentryEvent>()
85+
whenever(scopes.captureEvent(eventCaptor.capture(), any<Hint>())).thenReturn(null)
86+
87+
SentryExceptionResolver(scopes, transactionNameProvider, 1)
88+
.resolveException(request, response, null, RuntimeException("request failed"))
89+
90+
assertThat(eventCaptor.firstValue.transaction).isEqualTo(DEFAULT_TRANSACTION_NAME)
91+
}
92+
7393
@Test
7494
fun `when handles exception, provides spring resolver hint`() {
7595
val hintCaptor = argumentCaptor<Hint>()

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

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,20 @@
11
package io.sentry.spring7
22

33
import io.sentry.Hint
4+
import io.sentry.ILogger
45
import io.sentry.IScopes
56
import io.sentry.SentryEvent
67
import io.sentry.SentryOptions
8+
import io.sentry.TransactionContext.DEFAULT_TRANSACTION_NAME
79
import io.sentry.spring7.tracing.SpringMvcTransactionNameProvider
10+
import io.sentry.spring7.tracing.TransactionNameProvider
811
import jakarta.servlet.http.HttpServletRequest
912
import java.net.URI
1013
import kotlin.test.Test
1114
import kotlin.test.assertEquals
15+
import kotlin.test.assertFailsWith
1216
import kotlin.test.assertNotNull
17+
import org.assertj.core.api.Assertions.assertThat
1318
import org.mockito.kotlin.mock
1419
import org.mockito.kotlin.whenever
1520
import org.springframework.mock.web.MockServletContext
@@ -19,13 +24,15 @@ import org.springframework.web.servlet.HandlerMapping
1924
class SentryRequestHttpServletRequestProcessorTest {
2025
private class Fixture {
2126
val scopes = mock<IScopes>()
27+
val logger = mock<ILogger>()
2228

2329
fun getSut(
2430
request: HttpServletRequest,
2531
options: SentryOptions = SentryOptions(),
32+
provider: TransactionNameProvider = SpringMvcTransactionNameProvider(),
2633
): SentryRequestHttpServletRequestProcessor {
2734
whenever(scopes.options).thenReturn(options)
28-
return SentryRequestHttpServletRequestProcessor(SpringMvcTransactionNameProvider(), request)
35+
return SentryRequestHttpServletRequestProcessor(provider, request, logger)
2936
}
3037
}
3138

@@ -46,6 +53,30 @@ class SentryRequestHttpServletRequestProcessorTest {
4653
assertEquals("GET /some-path", event.transaction)
4754
}
4855

56+
@Test
57+
fun `provider failure uses safe transaction name`() {
58+
val failure = RuntimeException("provider failed")
59+
val request = mock<HttpServletRequest>()
60+
val eventProcessor =
61+
fixture.getSut(request, provider = TransactionNameProvider { throw failure })
62+
val event = SentryEvent()
63+
64+
eventProcessor.process(event, Hint())
65+
66+
assertThat(event.transaction).isEqualTo(DEFAULT_TRANSACTION_NAME)
67+
}
68+
69+
@Test
70+
fun `fatal provider failure propagates`() {
71+
val failure = OutOfMemoryError("fatal")
72+
val request = mock<HttpServletRequest>()
73+
val eventProcessor =
74+
fixture.getSut(request, provider = TransactionNameProvider { throw failure })
75+
76+
assertThat(assertFailsWith<OutOfMemoryError> { eventProcessor.process(SentryEvent(), Hint()) })
77+
.isSameAs(failure)
78+
}
79+
4980
@Test
5081
fun `when event has transaction name set, does not overwrite transaction name with value from the current request`() {
5182
val request =

0 commit comments

Comments
 (0)