diff --git a/CHANGELOG.md b/CHANGELOG.md index 68eb9a2ec21..1fbd38a3f7b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,8 @@ - Added `enableTraceIdGeneration` to the AndroidOptions. This allows Hybrid SDKs to "freeze" and control the trace and connect errors on different layers of the application ([4188](https://github.com/getsentry/sentry-java/pull/4188)) - Move to a single NetworkCallback listener to reduce number of IPC calls on Android ([#4164](https://github.com/getsentry/sentry-java/pull/4164)) - Add GraphQL Apollo Kotlin 4 integration ([#4166](https://github.com/getsentry/sentry-java/pull/4166)) +- Add support for async dispatch requests to Spring Boot 2 and 3 ([#3983](https://github.com/getsentry/sentry-java/pull/3983)) + - To enable it, please set `sentry.keep-transactions-open-for-async-responses=true` in `application.properties` or `sentry.keepTransactionsOpenForAsyncResponses: true` in `application.yml` - Add constructor to JUL `SentryHandler` for disabling external config ([#4208](https://github.com/getsentry/sentry-java/pull/4208)) ### Fixes diff --git a/sentry-spring-boot-jakarta/api/sentry-spring-boot-jakarta.api b/sentry-spring-boot-jakarta/api/sentry-spring-boot-jakarta.api index ac89da6d379..cec2df120b5 100644 --- a/sentry-spring-boot-jakarta/api/sentry-spring-boot-jakarta.api +++ b/sentry-spring-boot-jakarta/api/sentry-spring-boot-jakarta.api @@ -32,10 +32,12 @@ public class io/sentry/spring/boot/jakarta/SentryProperties : io/sentry/SentryOp public fun getReactive ()Lio/sentry/spring/boot/jakarta/SentryProperties$Reactive; public fun getUserFilterOrder ()Ljava/lang/Integer; public fun isEnableAotCompatibility ()Z + public fun isKeepTransactionsOpenForAsyncResponses ()Z public fun isUseGitCommitIdAsRelease ()Z public fun setEnableAotCompatibility (Z)V public fun setExceptionResolverOrder (I)V public fun setGraphql (Lio/sentry/spring/boot/jakarta/SentryProperties$Graphql;)V + public fun setKeepTransactionsOpenForAsyncResponses (Z)V public fun setLogging (Lio/sentry/spring/boot/jakarta/SentryProperties$Logging;)V public fun setReactive (Lio/sentry/spring/boot/jakarta/SentryProperties$Reactive;)V public fun setUseGitCommitIdAsRelease (Z)V diff --git a/sentry-spring-boot-jakarta/src/main/java/io/sentry/spring/boot/jakarta/SentryAutoConfiguration.java b/sentry-spring-boot-jakarta/src/main/java/io/sentry/spring/boot/jakarta/SentryAutoConfiguration.java index 811b8449df9..4ed9acf1c07 100644 --- a/sentry-spring-boot-jakarta/src/main/java/io/sentry/spring/boot/jakarta/SentryAutoConfiguration.java +++ b/sentry-spring-boot-jakarta/src/main/java/io/sentry/spring/boot/jakarta/SentryAutoConfiguration.java @@ -312,9 +312,14 @@ static class SentrySecurityConfiguration { @ConditionalOnMissingBean(name = "sentryTracingFilter") public FilterRegistrationBean sentryTracingFilter( final @NotNull IScopes scopes, - final @NotNull TransactionNameProvider transactionNameProvider) { + final @NotNull TransactionNameProvider transactionNameProvider, + final @NotNull SentryProperties sentryProperties) { FilterRegistrationBean filter = - new FilterRegistrationBean<>(new SentryTracingFilter(scopes, transactionNameProvider)); + new FilterRegistrationBean<>( + new SentryTracingFilter( + scopes, + transactionNameProvider, + sentryProperties.isKeepTransactionsOpenForAsyncResponses())); filter.setOrder(SENTRY_SPRING_FILTER_PRECEDENCE + 1); // must run after SentrySpringFilter return filter; } diff --git a/sentry-spring-boot-jakarta/src/main/java/io/sentry/spring/boot/jakarta/SentryProperties.java b/sentry-spring-boot-jakarta/src/main/java/io/sentry/spring/boot/jakarta/SentryProperties.java index 80ea79932ca..b0778134bd4 100644 --- a/sentry-spring-boot-jakarta/src/main/java/io/sentry/spring/boot/jakarta/SentryProperties.java +++ b/sentry-spring-boot-jakarta/src/main/java/io/sentry/spring/boot/jakarta/SentryProperties.java @@ -5,6 +5,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.slf4j.event.Level; @@ -28,6 +29,8 @@ public class SentryProperties extends SentryOptions { */ private @Nullable Integer userFilterOrder; + @ApiStatus.Experimental private boolean keepTransactionsOpenForAsyncResponses = false; + /** Logging framework integration properties. */ private @NotNull Logging logging = new Logging(); @@ -104,6 +107,15 @@ public void setEnableAotCompatibility(boolean enableAotCompatibility) { this.enableAotCompatibility = enableAotCompatibility; } + public boolean isKeepTransactionsOpenForAsyncResponses() { + return keepTransactionsOpenForAsyncResponses; + } + + public void setKeepTransactionsOpenForAsyncResponses( + boolean keepTransactionsOpenForAsyncResponses) { + this.keepTransactionsOpenForAsyncResponses = keepTransactionsOpenForAsyncResponses; + } + public @NotNull Graphql getGraphql() { return graphql; } diff --git a/sentry-spring-boot/api/sentry-spring-boot.api b/sentry-spring-boot/api/sentry-spring-boot.api index 79b72bfb39f..b3ce0896e38 100644 --- a/sentry-spring-boot/api/sentry-spring-boot.api +++ b/sentry-spring-boot/api/sentry-spring-boot.api @@ -30,9 +30,11 @@ public class io/sentry/spring/boot/SentryProperties : io/sentry/SentryOptions { public fun getGraphql ()Lio/sentry/spring/boot/SentryProperties$Graphql; public fun getLogging ()Lio/sentry/spring/boot/SentryProperties$Logging; public fun getUserFilterOrder ()Ljava/lang/Integer; + public fun isKeepTransactionsOpenForAsyncResponses ()Z public fun isUseGitCommitIdAsRelease ()Z public fun setExceptionResolverOrder (I)V public fun setGraphql (Lio/sentry/spring/boot/SentryProperties$Graphql;)V + public fun setKeepTransactionsOpenForAsyncResponses (Z)V public fun setLogging (Lio/sentry/spring/boot/SentryProperties$Logging;)V public fun setUseGitCommitIdAsRelease (Z)V public fun setUserFilterOrder (Ljava/lang/Integer;)V diff --git a/sentry-spring-boot/src/main/java/io/sentry/spring/boot/SentryAutoConfiguration.java b/sentry-spring-boot/src/main/java/io/sentry/spring/boot/SentryAutoConfiguration.java index 9745c8a6e55..98cdc464877 100644 --- a/sentry-spring-boot/src/main/java/io/sentry/spring/boot/SentryAutoConfiguration.java +++ b/sentry-spring-boot/src/main/java/io/sentry/spring/boot/SentryAutoConfiguration.java @@ -297,9 +297,14 @@ static class SentrySecurityConfiguration { @ConditionalOnMissingBean(name = "sentryTracingFilter") public FilterRegistrationBean sentryTracingFilter( final @NotNull IScopes scopes, - final @NotNull TransactionNameProvider transactionNameProvider) { + final @NotNull TransactionNameProvider transactionNameProvider, + final @NotNull SentryProperties sentryProperties) { FilterRegistrationBean filter = - new FilterRegistrationBean<>(new SentryTracingFilter(scopes, transactionNameProvider)); + new FilterRegistrationBean<>( + new SentryTracingFilter( + scopes, + transactionNameProvider, + sentryProperties.isKeepTransactionsOpenForAsyncResponses())); filter.setOrder(SENTRY_SPRING_FILTER_PRECEDENCE + 1); // must run after SentrySpringFilter return filter; } diff --git a/sentry-spring-boot/src/main/java/io/sentry/spring/boot/SentryProperties.java b/sentry-spring-boot/src/main/java/io/sentry/spring/boot/SentryProperties.java index 334e36f4024..e40700b6b63 100644 --- a/sentry-spring-boot/src/main/java/io/sentry/spring/boot/SentryProperties.java +++ b/sentry-spring-boot/src/main/java/io/sentry/spring/boot/SentryProperties.java @@ -5,6 +5,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.slf4j.event.Level; @@ -28,6 +29,8 @@ public class SentryProperties extends SentryOptions { */ private @Nullable Integer userFilterOrder; + @ApiStatus.Experimental private boolean keepTransactionsOpenForAsyncResponses = false; + /** Logging framework integration properties. */ private @NotNull Logging logging = new Logging(); @@ -70,6 +73,15 @@ public void setUserFilterOrder(final @Nullable Integer userFilterOrder) { this.userFilterOrder = userFilterOrder; } + public boolean isKeepTransactionsOpenForAsyncResponses() { + return keepTransactionsOpenForAsyncResponses; + } + + public void setKeepTransactionsOpenForAsyncResponses( + boolean keepTransactionsOpenForAsyncResponses) { + this.keepTransactionsOpenForAsyncResponses = keepTransactionsOpenForAsyncResponses; + } + public @NotNull Logging getLogging() { return logging; } diff --git a/sentry-spring-jakarta/api/sentry-spring-jakarta.api b/sentry-spring-jakarta/api/sentry-spring-jakarta.api index 4b942b833d2..4140cd0a59d 100644 --- a/sentry-spring-jakarta/api/sentry-spring-jakarta.api +++ b/sentry-spring-jakarta/api/sentry-spring-jakarta.api @@ -264,7 +264,9 @@ public class io/sentry/spring/jakarta/tracing/SentryTracingFilter : org/springfr public fun ()V public fun (Lio/sentry/IScopes;)V public fun (Lio/sentry/IScopes;Lio/sentry/spring/jakarta/tracing/TransactionNameProvider;)V + public fun (Lio/sentry/IScopes;Lio/sentry/spring/jakarta/tracing/TransactionNameProvider;Z)V protected fun doFilterInternal (Ljakarta/servlet/http/HttpServletRequest;Ljakarta/servlet/http/HttpServletResponse;Ljakarta/servlet/FilterChain;)V + protected fun shouldNotFilterAsyncDispatch ()Z } public abstract interface annotation class io/sentry/spring/jakarta/tracing/SentryTransaction : java/lang/annotation/Annotation { diff --git a/sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/tracing/SentryTracingFilter.java b/sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/tracing/SentryTracingFilter.java index bd1ffbab812..f740ce4e612 100644 --- a/sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/tracing/SentryTracingFilter.java +++ b/sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/tracing/SentryTracingFilter.java @@ -37,9 +37,11 @@ public class SentryTracingFilter extends OncePerRequestFilter { private static final String TRANSACTION_OP = "http.server"; private static final String TRACE_ORIGIN = "auto.http.spring_jakarta.webmvc"; + private static final String TRANSACTION_ATTR = "sentry.transaction"; private final @NotNull TransactionNameProvider transactionNameProvider; private final @NotNull IScopes scopes; + private final boolean isAsyncSupportEnabled; /** * Creates filter that resolves transaction name using {@link SpringMvcTransactionNameProvider}. @@ -63,15 +65,37 @@ public SentryTracingFilter() { public SentryTracingFilter( final @NotNull IScopes scopes, final @NotNull TransactionNameProvider transactionNameProvider) { + this(scopes, transactionNameProvider, false); + } + + /** + * Creates filter that resolves transaction name using transaction name provider given by + * parameter. + * + * @param scopes - the scopes + * @param transactionNameProvider - transaction name provider. + * @param isAsyncSupportEnabled - whether transactions should be kept open until async handling is + * done + */ + public SentryTracingFilter( + final @NotNull IScopes scopes, + final @NotNull TransactionNameProvider transactionNameProvider, + final boolean isAsyncSupportEnabled) { this.scopes = Objects.requireNonNull(scopes, "Scopes are required"); this.transactionNameProvider = Objects.requireNonNull(transactionNameProvider, "transactionNameProvider is required"); + this.isAsyncSupportEnabled = isAsyncSupportEnabled; } public SentryTracingFilter(final @NotNull IScopes scopes) { this(scopes, new SpringMvcTransactionNameProvider()); } + @Override + protected boolean shouldNotFilterAsyncDispatch() { + return !isAsyncSupportEnabled; + } + @Override protected void doFilterInternal( final @NotNull HttpServletRequest httpRequest, @@ -79,12 +103,14 @@ protected void doFilterInternal( final @NotNull FilterChain filterChain) throws ServletException, IOException { if (scopes.isEnabled() && !isIgnored()) { - final @Nullable String sentryTraceHeader = - httpRequest.getHeader(SentryTraceHeader.SENTRY_TRACE_HEADER); - final @Nullable List baggageHeader = - Collections.list(httpRequest.getHeaders(BaggageHeader.BAGGAGE_HEADER)); - final @Nullable TransactionContext transactionContext = - scopes.continueTrace(sentryTraceHeader, baggageHeader); + @Nullable TransactionContext transactionContext = null; + if (shouldContinueTrace(httpRequest)) { + final @Nullable String sentryTraceHeader = + httpRequest.getHeader(SentryTraceHeader.SENTRY_TRACE_HEADER); + final @Nullable List baggageHeader = + Collections.list(httpRequest.getHeaders(BaggageHeader.BAGGAGE_HEADER)); + transactionContext = scopes.continueTrace(sentryTraceHeader, baggageHeader); + } if (scopes.getOptions().isTracingEnabled() && shouldTraceRequest(httpRequest)) { doFilterWithTransaction(httpRequest, httpResponse, filterChain, transactionContext); } else { @@ -105,35 +131,85 @@ private void doFilterWithTransaction( FilterChain filterChain, final @Nullable TransactionContext transactionContext) throws IOException, ServletException { - // at this stage we are not able to get real transaction name - final ITransaction transaction = startTransaction(httpRequest, transactionContext); + final @Nullable ITransaction transaction = + getOrStartTransaction(httpRequest, transactionContext); try { filterChain.doFilter(httpRequest, httpResponse); } catch (Throwable e) { - // exceptions that are not handled by Spring - transaction.setStatus(SpanStatus.INTERNAL_ERROR); + if (transaction != null) { + // exceptions that are not handled by Spring + transaction.setStatus(SpanStatus.INTERNAL_ERROR); + } throw e; } finally { - // after all filters run, templated path pattern is available in request attribute - final String transactionName = transactionNameProvider.provideTransactionName(httpRequest); - final TransactionNameSource transactionNameSource = - transactionNameProvider.provideTransactionSource(); - // if transaction name is not resolved, the request has not been processed by a controller - // and we should not report it to Sentry - if (transactionName != null) { - transaction.setName(transactionName, transactionNameSource); - transaction.setOperation(TRANSACTION_OP); - // if exception has been thrown, transaction status is already set to INTERNAL_ERROR, and - // httpResponse.getStatus() returns 200. - if (transaction.getStatus() == null) { - transaction.setStatus(SpanStatus.fromHttpStatusCode(httpResponse.getStatus())); + if (shouldFinishTransaction(httpRequest) && transaction != null) { + // after all filters run, templated path pattern is available in request attribute + final String transactionName = transactionNameProvider.provideTransactionName(httpRequest); + final TransactionNameSource transactionNameSource = + transactionNameProvider.provideTransactionSource(); + // if transaction name is not resolved, the request has not been processed by a controller + // and we should not report it to Sentry + if (transactionName != null) { + transaction.setName(transactionName, transactionNameSource); + transaction.setOperation(TRANSACTION_OP); + // if exception has been thrown, transaction status is already set to INTERNAL_ERROR, and + // httpResponse.getStatus() returns 200. + if (transaction.getStatus() == null) { + transaction.setStatus(SpanStatus.fromHttpStatusCode(httpResponse.getStatus())); + } + transaction.finish(); } - transaction.finish(); } } } + private ITransaction getOrStartTransaction( + final @NotNull HttpServletRequest httpRequest, + final @Nullable TransactionContext transactionContext) { + if (isAsyncDispatch(httpRequest)) { + // second invocation of this filter for the same async request already has the transaction + // in the attributes + return (ITransaction) httpRequest.getAttribute(TRANSACTION_ATTR); + } else { + // at this stage we are not able to get real transaction name + final @NotNull ITransaction transaction = startTransaction(httpRequest, transactionContext); + if (shouldStoreTransactionForAsyncProcessing()) { + httpRequest.setAttribute(TRANSACTION_ATTR, transaction); + } + return transaction; + } + } + + /** + * Returns false if an async request is being dispatched (second invocation of the filter for the + * same async request). + * + *

Returns true if not an async request or this is the first invocation of the filter for the + * same async request + */ + private boolean shouldContinueTrace(HttpServletRequest httpRequest) { + return !isAsyncSupportEnabled || !isAsyncDispatch(httpRequest); + } + + private boolean shouldStoreTransactionForAsyncProcessing() { + return isAsyncSupportEnabled; + } + + /** + * Returns false if async request handling has only been started but not yet finished (first + * invocation of this filter for the same async request). + * + *

Returns true if not an async request or async request handling has finished (second + * invocation of this filter for the same async request) + * + *

Note: isAsyncStarted changes its return value after filterChain.doFilter() of the first + * async invocation + */ + private boolean shouldFinishTransaction(HttpServletRequest httpRequest) { + return !isAsyncSupportEnabled || !isAsyncStarted(httpRequest); + } + private boolean shouldTraceRequest(final @NotNull HttpServletRequest request) { return scopes.getOptions().isTraceOptionsRequests() || !HttpMethod.OPTIONS.name().equals(request.getMethod()); diff --git a/sentry-spring-jakarta/src/test/kotlin/io/sentry/spring/jakarta/tracing/SentryTracingFilterTest.kt b/sentry-spring-jakarta/src/test/kotlin/io/sentry/spring/jakarta/tracing/SentryTracingFilterTest.kt index ff0020ddfeb..14bf0969035 100644 --- a/sentry-spring-jakarta/src/test/kotlin/io/sentry/spring/jakarta/tracing/SentryTracingFilterTest.kt +++ b/sentry-spring-jakarta/src/test/kotlin/io/sentry/spring/jakarta/tracing/SentryTracingFilterTest.kt @@ -13,12 +13,15 @@ import io.sentry.TransactionOptions import io.sentry.protocol.SentryId import io.sentry.protocol.SentryTransaction import io.sentry.protocol.TransactionNameSource +import jakarta.servlet.DispatcherType import jakarta.servlet.FilterChain import jakarta.servlet.http.HttpServletRequest import org.assertj.core.api.Assertions.assertThat +import org.mockito.Mockito import org.mockito.kotlin.any import org.mockito.kotlin.anyOrNull import org.mockito.kotlin.check +import org.mockito.kotlin.doAnswer import org.mockito.kotlin.eq import org.mockito.kotlin.mock import org.mockito.kotlin.never @@ -29,6 +32,8 @@ import org.mockito.kotlin.whenever import org.springframework.http.HttpMethod import org.springframework.mock.web.MockHttpServletRequest import org.springframework.mock.web.MockHttpServletResponse +import org.springframework.web.context.request.async.AsyncWebRequest +import org.springframework.web.context.request.async.WebAsyncUtils import org.springframework.web.servlet.HandlerMapping import kotlin.test.Test import kotlin.test.assertEquals @@ -47,13 +52,14 @@ class SentryTracingFilterTest { dsn = "https://key@sentry.io/proj" tracesSampleRate = 1.0 } + val asyncRequest = mock() val logger = mock() init { whenever(scopes.options).thenReturn(options) } - fun getSut(isEnabled: Boolean = true, status: Int = 200, sentryTraceHeader: String? = null, baggageHeaders: List? = null): SentryTracingFilter { + fun getSut(isEnabled: Boolean = true, status: Int = 200, sentryTraceHeader: String? = null, baggageHeaders: List? = null, isAsyncSupportEnabled: Boolean = false): SentryTracingFilter { request.requestURI = "/product/12" request.method = "POST" request.setAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE, "/product/{id}") @@ -70,7 +76,7 @@ class SentryTracingFilterTest { whenever(scopes.startTransaction(any(), check { assertTrue(it.isBindToScope) })).thenAnswer { SentryTracer(it.arguments[0] as TransactionContext, scopes) } whenever(scopes.isEnabled).thenReturn(isEnabled) whenever(scopes.continueTrace(any(), any())).thenAnswer { TransactionContext.fromPropagationContext(PropagationContext.fromHeaders(logger, it.arguments[0] as String?, it.arguments[1] as List?)) } - return SentryTracingFilter(scopes, transactionNameProvider) + return SentryTracingFilter(scopes, transactionNameProvider, isAsyncSupportEnabled) } } @@ -307,4 +313,101 @@ class SentryTracingFilterTest { anyOrNull() ) } + + @Test + fun `creates transaction around async request`() { + val sentryTrace = "f9118105af4a2d42b4124532cd1065ff-424cffc8f94feeee-1" + val baggage = listOf("baggage: sentry-environment=production,sentry-public_key=502f25099c204a2fbf4cb16edc5975d1,sentry-sample_rand=0.456789,sentry-sample_rate=0.5,sentry-sampled=true,sentry-trace_id=df71f5972f754b4c85af13ff5c07017d") + val filter = fixture.getSut(sentryTraceHeader = sentryTrace, baggageHeaders = baggage, isAsyncSupportEnabled = true) + + val asyncChain = mock() + doAnswer { + val request = it.arguments.first() as MockHttpServletRequest + whenever(fixture.asyncRequest.isAsyncStarted).thenReturn(true) + WebAsyncUtils.getAsyncManager(request).setAsyncWebRequest(fixture.asyncRequest) + }.whenever(asyncChain).doFilter(any(), any()) + + filter.doFilter(fixture.request, fixture.response, asyncChain) + + verify(fixture.scopes).continueTrace(eq(sentryTrace), eq(baggage)) + verify(fixture.scopes).startTransaction( + check { + assertEquals("POST /product/12", it.name) + assertEquals(TransactionNameSource.URL, it.transactionNameSource) + assertEquals("http.server", it.operation) + }, + check { + assertNotNull(it.customSamplingContext?.get("request")) + assertTrue(it.customSamplingContext?.get("request") is HttpServletRequest) + assertTrue(it.isBindToScope) + assertThat(it.origin).isEqualTo("auto.http.spring_jakarta.webmvc") + } + ) + verify(asyncChain).doFilter(fixture.request, fixture.response) + verify(fixture.scopes, never()).captureTransaction( + any(), + anyOrNull(), + anyOrNull(), + anyOrNull() + ) + + Mockito.clearInvocations(fixture.scopes) + + fixture.request.dispatcherType = DispatcherType.ASYNC + whenever(fixture.asyncRequest.isAsyncStarted).thenReturn(false) + + filter.doFilter(fixture.request, fixture.response, fixture.chain) + + verify(fixture.scopes, never()).startTransaction(anyOrNull(), anyOrNull()) + + verify(fixture.chain).doFilter(fixture.request, fixture.response) + + verify(fixture.scopes).captureTransaction( + check { + assertThat(it.transaction).isEqualTo("POST /product/{id}") + assertThat(it.contexts.trace!!.status).isEqualTo(SpanStatus.OK) + assertThat(it.contexts.trace!!.operation).isEqualTo("http.server") + }, + anyOrNull(), + anyOrNull(), + anyOrNull() + ) + verify(fixture.scopes, never()).continueTrace(anyOrNull(), anyOrNull()) + } + + @Test + fun `creates and finishes transaction immediately for async request if handling disabled`() { + val sentryTrace = "f9118105af4a2d42b4124532cd1065ff-424cffc8f94feeee-1" + val baggage = listOf("baggage: sentry-environment=production,sentry-public_key=502f25099c204a2fbf4cb16edc5975d1,sentry-sample_rand=0.456789,sentry-sample_rate=0.5,sentry-sampled=true,sentry-trace_id=df71f5972f754b4c85af13ff5c07017d") + val filter = fixture.getSut(sentryTraceHeader = sentryTrace, baggageHeaders = baggage, isAsyncSupportEnabled = false) + + filter.doFilter(fixture.request, fixture.response, fixture.chain) + + verify(fixture.scopes).startTransaction( + check { + assertEquals("POST /product/12", it.name) + assertEquals(TransactionNameSource.URL, it.transactionNameSource) + assertEquals("http.server", it.operation) + }, + check { + assertNotNull(it.customSamplingContext?.get("request")) + assertTrue(it.customSamplingContext?.get("request") is HttpServletRequest) + assertTrue(it.isBindToScope) + assertThat(it.origin).isEqualTo("auto.http.spring_jakarta.webmvc") + } + ) + verify(fixture.scopes).continueTrace(eq(sentryTrace), eq(baggage)) + verify(fixture.chain).doFilter(fixture.request, fixture.response) + + verify(fixture.scopes).captureTransaction( + check { + assertThat(it.transaction).isEqualTo("POST /product/{id}") + assertThat(it.contexts.trace!!.status).isEqualTo(SpanStatus.OK) + assertThat(it.contexts.trace!!.operation).isEqualTo("http.server") + }, + anyOrNull(), + anyOrNull(), + anyOrNull() + ) + } } diff --git a/sentry-spring/api/sentry-spring.api b/sentry-spring/api/sentry-spring.api index 67ad0c1c963..4743c32284b 100644 --- a/sentry-spring/api/sentry-spring.api +++ b/sentry-spring/api/sentry-spring.api @@ -255,7 +255,9 @@ public class io/sentry/spring/tracing/SentryTracingFilter : org/springframework/ public fun ()V public fun (Lio/sentry/IScopes;)V public fun (Lio/sentry/IScopes;Lio/sentry/spring/tracing/TransactionNameProvider;)V + public fun (Lio/sentry/IScopes;Lio/sentry/spring/tracing/TransactionNameProvider;Z)V protected fun doFilterInternal (Ljavax/servlet/http/HttpServletRequest;Ljavax/servlet/http/HttpServletResponse;Ljavax/servlet/FilterChain;)V + protected fun shouldNotFilterAsyncDispatch ()Z } public abstract interface annotation class io/sentry/spring/tracing/SentryTransaction : java/lang/annotation/Annotation { diff --git a/sentry-spring/src/main/java/io/sentry/spring/tracing/SentryTracingFilter.java b/sentry-spring/src/main/java/io/sentry/spring/tracing/SentryTracingFilter.java index 8f228f80b72..6c83eb2fb4f 100644 --- a/sentry-spring/src/main/java/io/sentry/spring/tracing/SentryTracingFilter.java +++ b/sentry-spring/src/main/java/io/sentry/spring/tracing/SentryTracingFilter.java @@ -34,9 +34,11 @@ public class SentryTracingFilter extends OncePerRequestFilter { private static final String TRANSACTION_OP = "http.server"; private static final String TRACE_ORIGIN = "auto.http.spring.webmvc"; + private static final String TRANSACTION_ATTR = "sentry.transaction"; private final @NotNull TransactionNameProvider transactionNameProvider; private final @NotNull IScopes scopes; + private final boolean isAsyncSupportEnabled; /** * Creates filter that resolves transaction name using {@link SpringMvcTransactionNameProvider}. @@ -60,15 +62,37 @@ public SentryTracingFilter() { public SentryTracingFilter( final @NotNull IScopes scopes, final @NotNull TransactionNameProvider transactionNameProvider) { + this(scopes, transactionNameProvider, false); + } + + /** + * Creates filter that resolves transaction name using transaction name provider given by + * parameter. + * + * @param scopes - the scopes + * @param transactionNameProvider - transaction name provider. + * @param isAsyncSupportEnabled - whether transactions should be kept open until async handling is + * done + */ + public SentryTracingFilter( + final @NotNull IScopes scopes, + final @NotNull TransactionNameProvider transactionNameProvider, + final boolean isAsyncSupportEnabled) { this.scopes = Objects.requireNonNull(scopes, "Scopes are required"); this.transactionNameProvider = Objects.requireNonNull(transactionNameProvider, "transactionNameProvider is required"); + this.isAsyncSupportEnabled = isAsyncSupportEnabled; } public SentryTracingFilter(final @NotNull IScopes scopes) { this(scopes, new SpringMvcTransactionNameProvider()); } + @Override + protected boolean shouldNotFilterAsyncDispatch() { + return !isAsyncSupportEnabled; + } + @Override protected void doFilterInternal( final @NotNull HttpServletRequest httpRequest, @@ -77,12 +101,14 @@ protected void doFilterInternal( throws ServletException, IOException { if (scopes.isEnabled() && !isIgnored()) { - final @Nullable String sentryTraceHeader = - httpRequest.getHeader(SentryTraceHeader.SENTRY_TRACE_HEADER); - final @Nullable List baggageHeader = - Collections.list(httpRequest.getHeaders(BaggageHeader.BAGGAGE_HEADER)); - final @Nullable TransactionContext transactionContext = - scopes.continueTrace(sentryTraceHeader, baggageHeader); + @Nullable TransactionContext transactionContext = null; + if (shouldContinueTrace(httpRequest)) { + final @Nullable String sentryTraceHeader = + httpRequest.getHeader(SentryTraceHeader.SENTRY_TRACE_HEADER); + final @Nullable List baggageHeader = + Collections.list(httpRequest.getHeaders(BaggageHeader.BAGGAGE_HEADER)); + transactionContext = scopes.continueTrace(sentryTraceHeader, baggageHeader); + } if (scopes.getOptions().isTracingEnabled() && shouldTraceRequest(httpRequest)) { doFilterWithTransaction(httpRequest, httpResponse, filterChain, transactionContext); @@ -104,35 +130,85 @@ private void doFilterWithTransaction( FilterChain filterChain, final @Nullable TransactionContext transactionContext) throws IOException, ServletException { - // at this stage we are not able to get real transaction name - final ITransaction transaction = startTransaction(httpRequest, transactionContext); + final @Nullable ITransaction transaction = + getOrStartTransaction(httpRequest, transactionContext); try { filterChain.doFilter(httpRequest, httpResponse); } catch (Throwable e) { - // exceptions that are not handled by Spring - transaction.setStatus(SpanStatus.INTERNAL_ERROR); + if (transaction != null) { + // exceptions that are not handled by Spring + transaction.setStatus(SpanStatus.INTERNAL_ERROR); + } throw e; } finally { - // after all filters run, templated path pattern is available in request attribute - final String transactionName = transactionNameProvider.provideTransactionName(httpRequest); - final TransactionNameSource transactionNameSource = - transactionNameProvider.provideTransactionSource(); - // if transaction name is not resolved, the request has not been processed by a controller - // and we should not report it to Sentry - if (transactionName != null) { - transaction.setName(transactionName, transactionNameSource); - transaction.setOperation(TRANSACTION_OP); - // if exception has been thrown, transaction status is already set to INTERNAL_ERROR, and - // httpResponse.getStatus() returns 200. - if (transaction.getStatus() == null) { - transaction.setStatus(SpanStatus.fromHttpStatusCode(httpResponse.getStatus())); + if (shouldFinishTransaction(httpRequest) && transaction != null) { + // after all filters run, templated path pattern is available in request attribute + final String transactionName = transactionNameProvider.provideTransactionName(httpRequest); + final TransactionNameSource transactionNameSource = + transactionNameProvider.provideTransactionSource(); + // if transaction name is not resolved, the request has not been processed by a controller + // and we should not report it to Sentry + if (transactionName != null) { + transaction.setName(transactionName, transactionNameSource); + transaction.setOperation(TRANSACTION_OP); + // if exception has been thrown, transaction status is already set to INTERNAL_ERROR, and + // httpResponse.getStatus() returns 200. + if (transaction.getStatus() == null) { + transaction.setStatus(SpanStatus.fromHttpStatusCode(httpResponse.getStatus())); + } + transaction.finish(); } - transaction.finish(); } } } + private ITransaction getOrStartTransaction( + final @NotNull HttpServletRequest httpRequest, + final @Nullable TransactionContext transactionContext) { + if (isAsyncDispatch(httpRequest)) { + // second invocation of this filter for the same async request already has the transaction + // in the attributes + return (ITransaction) httpRequest.getAttribute(TRANSACTION_ATTR); + } else { + // at this stage we are not able to get real transaction name + final @NotNull ITransaction transaction = startTransaction(httpRequest, transactionContext); + if (shouldStoreTransactionForAsyncProcessing()) { + httpRequest.setAttribute(TRANSACTION_ATTR, transaction); + } + return transaction; + } + } + + /** + * Returns false if an async request is being dispatched (second invocation of the filter for the + * same async request). + * + *

Returns true if not an async request or this is the first invocation of the filter for the + * same async request + */ + private boolean shouldContinueTrace(HttpServletRequest httpRequest) { + return !isAsyncSupportEnabled || !isAsyncDispatch(httpRequest); + } + + private boolean shouldStoreTransactionForAsyncProcessing() { + return isAsyncSupportEnabled; + } + + /** + * Returns false if async request handling has only been started but not yet finished (first + * invocation of this filter for the same async request). + * + *

Returns true if not an async request or async request handling has finished (second + * invocation of this filter for the same async request) + * + *

Note: isAsyncStarted changes its return value after filterChain.doFilter() of the first + * async invocation + */ + private boolean shouldFinishTransaction(HttpServletRequest httpRequest) { + return !isAsyncSupportEnabled || !isAsyncStarted(httpRequest); + } + private boolean shouldTraceRequest(final @NotNull HttpServletRequest request) { return scopes.getOptions().isTraceOptionsRequests() || !HttpMethod.OPTIONS.name().equals(request.getMethod()); diff --git a/sentry-spring/src/test/kotlin/io/sentry/spring/tracing/SentryTracingFilterTest.kt b/sentry-spring/src/test/kotlin/io/sentry/spring/tracing/SentryTracingFilterTest.kt index 538ac7c8cc6..9270d9b0c8a 100644 --- a/sentry-spring/src/test/kotlin/io/sentry/spring/tracing/SentryTracingFilterTest.kt +++ b/sentry-spring/src/test/kotlin/io/sentry/spring/tracing/SentryTracingFilterTest.kt @@ -14,9 +14,11 @@ import io.sentry.protocol.SentryId import io.sentry.protocol.SentryTransaction import io.sentry.protocol.TransactionNameSource import org.assertj.core.api.Assertions.assertThat +import org.mockito.Mockito import org.mockito.kotlin.any import org.mockito.kotlin.anyOrNull import org.mockito.kotlin.check +import org.mockito.kotlin.doAnswer import org.mockito.kotlin.eq import org.mockito.kotlin.mock import org.mockito.kotlin.never @@ -27,7 +29,10 @@ import org.mockito.kotlin.whenever import org.springframework.http.HttpMethod import org.springframework.mock.web.MockHttpServletRequest import org.springframework.mock.web.MockHttpServletResponse +import org.springframework.web.context.request.async.AsyncWebRequest +import org.springframework.web.context.request.async.WebAsyncUtils import org.springframework.web.servlet.HandlerMapping +import javax.servlet.DispatcherType import javax.servlet.FilterChain import javax.servlet.http.HttpServletRequest import kotlin.test.Test @@ -48,12 +53,13 @@ class SentryTracingFilterTest { tracesSampleRate = 1.0 } val logger = mock() + val asyncRequest = mock() init { whenever(scopes.options).thenReturn(options) } - fun getSut(isEnabled: Boolean = true, status: Int = 200, sentryTraceHeader: String? = null, baggageHeaders: List? = null): SentryTracingFilter { + fun getSut(isEnabled: Boolean = true, status: Int = 200, sentryTraceHeader: String? = null, baggageHeaders: List? = null, isAsyncSupportEnabled: Boolean = false): SentryTracingFilter { request.requestURI = "/product/12" request.method = "POST" request.setAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE, "/product/{id}") @@ -70,7 +76,7 @@ class SentryTracingFilterTest { whenever(scopes.startTransaction(any(), check { assertTrue(it.isBindToScope) })).thenAnswer { SentryTracer(it.arguments[0] as TransactionContext, scopes) } whenever(scopes.isEnabled).thenReturn(isEnabled) whenever(scopes.continueTrace(any(), any())).thenAnswer { TransactionContext.fromPropagationContext(PropagationContext.fromHeaders(logger, it.arguments[0] as String?, it.arguments[1] as List?)) } - return SentryTracingFilter(scopes, transactionNameProvider) + return SentryTracingFilter(scopes, transactionNameProvider, isAsyncSupportEnabled) } } @@ -307,4 +313,101 @@ class SentryTracingFilterTest { anyOrNull() ) } + + @Test + fun `creates transaction around async request`() { + val sentryTrace = "f9118105af4a2d42b4124532cd1065ff-424cffc8f94feeee-1" + val baggage = listOf("baggage: sentry-environment=production,sentry-public_key=502f25099c204a2fbf4cb16edc5975d1,sentry-sample_rand=0.456789,sentry-sample_rate=0.5,sentry-sampled=true,sentry-trace_id=df71f5972f754b4c85af13ff5c07017d") + val filter = fixture.getSut(sentryTraceHeader = sentryTrace, baggageHeaders = baggage, isAsyncSupportEnabled = true) + + val asyncChain = mock() + doAnswer { + val request = it.arguments.first() as MockHttpServletRequest + whenever(fixture.asyncRequest.isAsyncStarted).thenReturn(true) + WebAsyncUtils.getAsyncManager(request).setAsyncWebRequest(fixture.asyncRequest) + }.whenever(asyncChain).doFilter(any(), any()) + + filter.doFilter(fixture.request, fixture.response, asyncChain) + + verify(fixture.scopes).continueTrace(eq(sentryTrace), eq(baggage)) + verify(fixture.scopes).startTransaction( + check { + assertEquals("POST /product/12", it.name) + assertEquals(TransactionNameSource.URL, it.transactionNameSource) + assertEquals("http.server", it.operation) + }, + check { + assertNotNull(it.customSamplingContext?.get("request")) + assertTrue(it.customSamplingContext?.get("request") is HttpServletRequest) + assertTrue(it.isBindToScope) + assertThat(it.origin).isEqualTo("auto.http.spring.webmvc") + } + ) + verify(asyncChain).doFilter(fixture.request, fixture.response) + verify(fixture.scopes, never()).captureTransaction( + any(), + anyOrNull(), + anyOrNull(), + anyOrNull() + ) + + Mockito.clearInvocations(fixture.scopes) + + fixture.request.dispatcherType = DispatcherType.ASYNC + whenever(fixture.asyncRequest.isAsyncStarted).thenReturn(false) + + filter.doFilter(fixture.request, fixture.response, fixture.chain) + + verify(fixture.scopes, never()).startTransaction(anyOrNull(), anyOrNull()) + + verify(fixture.chain).doFilter(fixture.request, fixture.response) + + verify(fixture.scopes).captureTransaction( + check { + assertThat(it.transaction).isEqualTo("POST /product/{id}") + assertThat(it.contexts.trace!!.status).isEqualTo(SpanStatus.OK) + assertThat(it.contexts.trace!!.operation).isEqualTo("http.server") + }, + anyOrNull(), + anyOrNull(), + anyOrNull() + ) + verify(fixture.scopes, never()).continueTrace(anyOrNull(), anyOrNull()) + } + + @Test + fun `creates and finishes transaction immediately for async request if handling disabled`() { + val sentryTrace = "f9118105af4a2d42b4124532cd1065ff-424cffc8f94feeee-1" + val baggage = listOf("baggage: sentry-environment=production,sentry-public_key=502f25099c204a2fbf4cb16edc5975d1,sentry-sample_rand=0.456789,sentry-sample_rate=0.5,sentry-sampled=true,sentry-trace_id=df71f5972f754b4c85af13ff5c07017d") + val filter = fixture.getSut(sentryTraceHeader = sentryTrace, baggageHeaders = baggage, isAsyncSupportEnabled = false) + + filter.doFilter(fixture.request, fixture.response, fixture.chain) + + verify(fixture.scopes).startTransaction( + check { + assertEquals("POST /product/12", it.name) + assertEquals(TransactionNameSource.URL, it.transactionNameSource) + assertEquals("http.server", it.operation) + }, + check { + assertNotNull(it.customSamplingContext?.get("request")) + assertTrue(it.customSamplingContext?.get("request") is HttpServletRequest) + assertTrue(it.isBindToScope) + assertThat(it.origin).isEqualTo("auto.http.spring.webmvc") + } + ) + verify(fixture.scopes).continueTrace(eq(sentryTrace), eq(baggage)) + verify(fixture.chain).doFilter(fixture.request, fixture.response) + + verify(fixture.scopes).captureTransaction( + check { + assertThat(it.transaction).isEqualTo("POST /product/{id}") + assertThat(it.contexts.trace!!.status).isEqualTo(SpanStatus.OK) + assertThat(it.contexts.trace!!.operation).isEqualTo("http.server") + }, + anyOrNull(), + anyOrNull(), + anyOrNull() + ) + } }