diff --git a/CHANGELOG.md b/CHANGELOG.md index 25ed9a8fe3..4af70ac9f5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### Fixes - Base64 encode internal Apollo3 Headers ([#2707](https://github.com/getsentry/sentry-java/pull/2707)) +- Finish WebFlux transaction before popping scope ([#2724](https://github.com/getsentry/sentry-java/pull/2724)) - Fix `SentryTracer` crash when scheduling auto-finish of a transaction, but the timer has already been cancelled ([#2731](https://github.com/getsentry/sentry-java/pull/2731)) - Fix `AndroidTransactionProfiler` crash when finishing a profile that happened due to race condition ([#2731](https://github.com/getsentry/sentry-java/pull/2731)) diff --git a/sentry-spring-boot-starter-jakarta/api/sentry-spring-boot-starter-jakarta.api b/sentry-spring-boot-starter-jakarta/api/sentry-spring-boot-starter-jakarta.api index ce56e6da34..49141a82bb 100644 --- a/sentry-spring-boot-starter-jakarta/api/sentry-spring-boot-starter-jakarta.api +++ b/sentry-spring-boot-starter-jakarta/api/sentry-spring-boot-starter-jakarta.api @@ -53,6 +53,5 @@ public class io/sentry/spring/boot/jakarta/SentryProperties$Reactive { public class io/sentry/spring/boot/jakarta/SentryWebfluxAutoConfiguration { public fun ()V public fun sentryWebExceptionHandler (Lio/sentry/IHub;)Lio/sentry/spring/jakarta/webflux/SentryWebExceptionHandler; - public fun sentryWebTracingFilter ()Lio/sentry/spring/jakarta/webflux/SentryWebTracingFilter; } diff --git a/sentry-spring-boot-starter-jakarta/src/main/java/io/sentry/spring/boot/jakarta/SentryWebfluxAutoConfiguration.java b/sentry-spring-boot-starter-jakarta/src/main/java/io/sentry/spring/boot/jakarta/SentryWebfluxAutoConfiguration.java index 127de0a0ba..8b68d3435c 100644 --- a/sentry-spring-boot-starter-jakarta/src/main/java/io/sentry/spring/boot/jakarta/SentryWebfluxAutoConfiguration.java +++ b/sentry-spring-boot-starter-jakarta/src/main/java/io/sentry/spring/boot/jakarta/SentryWebfluxAutoConfiguration.java @@ -6,7 +6,6 @@ import io.sentry.spring.jakarta.webflux.SentryWebExceptionHandler; import io.sentry.spring.jakarta.webflux.SentryWebFilter; import io.sentry.spring.jakarta.webflux.SentryWebFilterWithThreadLocalAccessor; -import io.sentry.spring.jakarta.webflux.SentryWebTracingFilter; import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.NotNull; import org.springframework.boot.ApplicationRunner; @@ -14,7 +13,6 @@ import org.springframework.boot.autoconfigure.condition.AnyNestedCondition; import org.springframework.boot.autoconfigure.condition.ConditionalOnBean; import org.springframework.boot.autoconfigure.condition.ConditionalOnClass; -import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingClass; import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; import org.springframework.boot.autoconfigure.condition.ConditionalOnWebApplication; @@ -77,14 +75,6 @@ static class SentryWebfluxFilterConfiguration { } } - @Bean - @Order(SENTRY_SPRING_FILTER_PRECEDENCE + 1) - @Conditional(SentryAutoConfiguration.SentryTracingCondition.class) - @ConditionalOnMissingBean(name = "sentryWebTracingFilter") - public @NotNull SentryWebTracingFilter sentryWebTracingFilter() { - return new SentryWebTracingFilter(); - } - /** Configures exception handler that handles unhandled exceptions and sends them to Sentry. */ @Bean public @NotNull SentryWebExceptionHandler sentryWebExceptionHandler(final @NotNull IHub hub) { diff --git a/sentry-spring-boot-starter/api/sentry-spring-boot-starter.api b/sentry-spring-boot-starter/api/sentry-spring-boot-starter.api index c53a59646f..642f550e6b 100644 --- a/sentry-spring-boot-starter/api/sentry-spring-boot-starter.api +++ b/sentry-spring-boot-starter/api/sentry-spring-boot-starter.api @@ -47,6 +47,5 @@ public class io/sentry/spring/boot/SentryWebfluxAutoConfiguration { public fun sentryScheduleHookApplicationRunner ()Lorg/springframework/boot/ApplicationRunner; public fun sentryWebExceptionHandler (Lio/sentry/IHub;)Lio/sentry/spring/webflux/SentryWebExceptionHandler; public fun sentryWebFilter (Lio/sentry/IHub;)Lio/sentry/spring/webflux/SentryWebFilter; - public fun sentryWebTracingFilter ()Lio/sentry/spring/webflux/SentryWebTracingFilter; } diff --git a/sentry-spring-boot-starter/src/main/java/io/sentry/spring/boot/SentryWebfluxAutoConfiguration.java b/sentry-spring-boot-starter/src/main/java/io/sentry/spring/boot/SentryWebfluxAutoConfiguration.java index 5fb8ee0068..2d83b77635 100644 --- a/sentry-spring-boot-starter/src/main/java/io/sentry/spring/boot/SentryWebfluxAutoConfiguration.java +++ b/sentry-spring-boot-starter/src/main/java/io/sentry/spring/boot/SentryWebfluxAutoConfiguration.java @@ -5,16 +5,13 @@ import io.sentry.spring.webflux.SentryScheduleHook; import io.sentry.spring.webflux.SentryWebExceptionHandler; import io.sentry.spring.webflux.SentryWebFilter; -import io.sentry.spring.webflux.SentryWebTracingFilter; import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.NotNull; import org.springframework.boot.ApplicationRunner; import org.springframework.boot.autoconfigure.condition.ConditionalOnBean; import org.springframework.boot.autoconfigure.condition.ConditionalOnClass; -import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; import org.springframework.boot.autoconfigure.condition.ConditionalOnWebApplication; import org.springframework.context.annotation.Bean; -import org.springframework.context.annotation.Conditional; import org.springframework.context.annotation.Configuration; import org.springframework.core.Ordered; import org.springframework.core.annotation.Order; @@ -45,14 +42,6 @@ public class SentryWebfluxAutoConfiguration { return new SentryWebFilter(hub); } - @Bean - @Order(SENTRY_SPRING_FILTER_PRECEDENCE + 1) - @Conditional(SentryAutoConfiguration.SentryTracingCondition.class) - @ConditionalOnMissingBean(name = "sentryWebTracingFilter") - public @NotNull SentryWebTracingFilter sentryWebTracingFilter() { - return new SentryWebTracingFilter(); - } - /** Configures exception handler that handles unhandled exceptions and sends them to Sentry. */ @Bean public @NotNull SentryWebExceptionHandler sentryWebExceptionHandler(final @NotNull IHub hub) { diff --git a/sentry-spring-jakarta/api/sentry-spring-jakarta.api b/sentry-spring-jakarta/api/sentry-spring-jakarta.api index 1042acc17d..f4387ce93a 100644 --- a/sentry-spring-jakarta/api/sentry-spring-jakarta.api +++ b/sentry-spring-jakarta/api/sentry-spring-jakarta.api @@ -163,8 +163,12 @@ public abstract interface class io/sentry/spring/jakarta/tracing/TransactionName public abstract class io/sentry/spring/jakarta/webflux/AbstractSentryWebFilter : org/springframework/web/server/WebFilter { public static final field SENTRY_HUB_KEY Ljava/lang/String; public fun (Lio/sentry/IHub;)V - protected fun doFinally (Lio/sentry/IHub;)V + protected fun doFinally (Lorg/springframework/web/server/ServerWebExchange;Lio/sentry/IHub;Lio/sentry/ITransaction;)V protected fun doFirst (Lorg/springframework/web/server/ServerWebExchange;Lio/sentry/IHub;)V + protected fun doOnError (Lio/sentry/ITransaction;Ljava/lang/Throwable;)V + protected fun maybeStartTransaction (Lio/sentry/IHub;Lorg/springframework/http/server/reactive/ServerHttpRequest;)Lio/sentry/ITransaction; + protected fun shouldTraceRequest (Lio/sentry/IHub;Lorg/springframework/http/server/reactive/ServerHttpRequest;)Z + protected fun startTransaction (Lio/sentry/IHub;Lorg/springframework/http/server/reactive/ServerHttpRequest;)Lio/sentry/ITransaction; } public final class io/sentry/spring/jakarta/webflux/ReactorUtils { @@ -215,8 +219,3 @@ public final class io/sentry/spring/jakarta/webflux/SentryWebFilterWithThreadLoc public fun filter (Lorg/springframework/web/server/ServerWebExchange;Lorg/springframework/web/server/WebFilterChain;)Lreactor/core/publisher/Mono; } -public class io/sentry/spring/jakarta/webflux/SentryWebTracingFilter : org/springframework/web/server/WebFilter { - public fun ()V - public fun filter (Lorg/springframework/web/server/ServerWebExchange;Lorg/springframework/web/server/WebFilterChain;)Lreactor/core/publisher/Mono; -} - diff --git a/sentry-spring-jakarta/build.gradle.kts b/sentry-spring-jakarta/build.gradle.kts index 6ecb593c3a..1a063fc761 100644 --- a/sentry-spring-jakarta/build.gradle.kts +++ b/sentry-spring-jakarta/build.gradle.kts @@ -47,6 +47,7 @@ dependencies { testImplementation(kotlin(Config.kotlinStdLib)) testImplementation(Config.TestLibs.kotlinTestJunit) testImplementation(Config.TestLibs.mockitoKotlin) + testImplementation(Config.TestLibs.mockitoInline) testImplementation(Config.Libs.springBoot3StarterTest) testImplementation(Config.Libs.springBoot3StarterWeb) testImplementation(Config.Libs.springBoot3StarterWebflux) diff --git a/sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/webflux/AbstractSentryWebFilter.java b/sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/webflux/AbstractSentryWebFilter.java index 96ec9c7b80..cebfb74f96 100644 --- a/sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/webflux/AbstractSentryWebFilter.java +++ b/sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/webflux/AbstractSentryWebFilter.java @@ -3,12 +3,30 @@ import static io.sentry.TypeCheckHint.WEBFLUX_FILTER_REQUEST; import static io.sentry.TypeCheckHint.WEBFLUX_FILTER_RESPONSE; +import io.sentry.Baggage; +import io.sentry.BaggageHeader; import io.sentry.Breadcrumb; +import io.sentry.CustomSamplingContext; import io.sentry.Hint; import io.sentry.IHub; +import io.sentry.ITransaction; +import io.sentry.NoOpHub; +import io.sentry.Sentry; +import io.sentry.SentryLevel; +import io.sentry.SentryTraceHeader; +import io.sentry.SpanStatus; +import io.sentry.TransactionContext; +import io.sentry.TransactionOptions; +import io.sentry.exception.InvalidSentryTraceHeaderException; +import io.sentry.protocol.TransactionNameSource; import io.sentry.util.Objects; +import java.util.List; import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; +import org.springframework.http.HttpHeaders; +import org.springframework.http.HttpMethod; +import org.springframework.http.HttpStatusCode; import org.springframework.http.server.reactive.ServerHttpRequest; import org.springframework.http.server.reactive.ServerHttpResponse; import org.springframework.web.server.ServerWebExchange; @@ -19,29 +37,124 @@ public abstract class AbstractSentryWebFilter implements WebFilter { private final @NotNull SentryRequestResolver sentryRequestResolver; public static final String SENTRY_HUB_KEY = "sentry-hub"; + private static final String TRANSACTION_OP = "http.server"; public AbstractSentryWebFilter(final @NotNull IHub hub) { Objects.requireNonNull(hub, "hub is required"); this.sentryRequestResolver = new SentryRequestResolver(hub); } - protected void doFinally(final @NotNull IHub requestHub) { - requestHub.popScope(); + protected @Nullable ITransaction maybeStartTransaction( + final @NotNull IHub requestHub, final @NotNull ServerHttpRequest request) { + if (requestHub.isEnabled() + && requestHub.getOptions().isTracingEnabled() + && shouldTraceRequest(requestHub, request)) { + return startTransaction(requestHub, request); + } else { + return null; + } + } + + protected void doFinally( + final @NotNull ServerWebExchange serverWebExchange, + final @NotNull IHub requestHub, + final @Nullable ITransaction transaction) { + if (transaction != null) { + finishTransaction(serverWebExchange, transaction); + } + if (requestHub.isEnabled()) { + requestHub.popScope(); + } + Sentry.setCurrentHub(NoOpHub.getInstance()); } protected void doFirst( final @NotNull ServerWebExchange serverWebExchange, final @NotNull IHub requestHub) { - serverWebExchange.getAttributes().put(SENTRY_HUB_KEY, requestHub); - requestHub.pushScope(); - final ServerHttpRequest request = serverWebExchange.getRequest(); - final ServerHttpResponse response = serverWebExchange.getResponse(); - - final Hint hint = new Hint(); - hint.set(WEBFLUX_FILTER_REQUEST, request); - hint.set(WEBFLUX_FILTER_RESPONSE, response); - final String methodName = request.getMethod() != null ? request.getMethod().name() : "unknown"; - requestHub.addBreadcrumb(Breadcrumb.http(request.getURI().toString(), methodName), hint); - requestHub.configureScope( - scope -> scope.setRequest(sentryRequestResolver.resolveSentryRequest(request))); + if (requestHub.isEnabled()) { + serverWebExchange.getAttributes().put(SENTRY_HUB_KEY, requestHub); + requestHub.pushScope(); + final ServerHttpRequest request = serverWebExchange.getRequest(); + final ServerHttpResponse response = serverWebExchange.getResponse(); + + final Hint hint = new Hint(); + hint.set(WEBFLUX_FILTER_REQUEST, request); + hint.set(WEBFLUX_FILTER_RESPONSE, response); + final String methodName = + request.getMethod() != null ? request.getMethod().name() : "unknown"; + requestHub.addBreadcrumb(Breadcrumb.http(request.getURI().toString(), methodName), hint); + requestHub.configureScope( + scope -> scope.setRequest(sentryRequestResolver.resolveSentryRequest(request))); + } + } + + protected void doOnError(final @Nullable ITransaction transaction, final @NotNull Throwable e) { + if (transaction != null) { + transaction.setStatus(SpanStatus.INTERNAL_ERROR); + transaction.setThrowable(e); + } + } + + protected boolean shouldTraceRequest( + final @NotNull IHub hub, final @NotNull ServerHttpRequest request) { + return hub.getOptions().isTraceOptionsRequests() + || !HttpMethod.OPTIONS.equals(request.getMethod()); + } + + private void finishTransaction(ServerWebExchange exchange, ITransaction transaction) { + String transactionName = TransactionNameProvider.provideTransactionName(exchange); + if (transactionName != null) { + transaction.setName(transactionName, TransactionNameSource.ROUTE); + transaction.setOperation(TRANSACTION_OP); + } + if (transaction.getStatus() == null) { + final @Nullable ServerHttpResponse response = exchange.getResponse(); + if (response != null) { + final @Nullable HttpStatusCode statusCode = response.getStatusCode(); + if (statusCode != null) { + transaction.setStatus(SpanStatus.fromHttpStatusCode(statusCode.value())); + } + } + } + transaction.finish(); + } + + protected @NotNull ITransaction startTransaction( + final @NotNull IHub hub, final @NotNull ServerHttpRequest request) { + final @NotNull HttpHeaders headers = request.getHeaders(); + final @Nullable List sentryTraceHeaders = + headers.get(SentryTraceHeader.SENTRY_TRACE_HEADER); + final @Nullable List baggageHeaders = headers.get(BaggageHeader.BAGGAGE_HEADER); + final @NotNull String name = request.getMethod() + " " + request.getURI().getPath(); + final @NotNull CustomSamplingContext customSamplingContext = new CustomSamplingContext(); + customSamplingContext.set("request", request); + + final TransactionOptions transactionOptions = new TransactionOptions(); + transactionOptions.setCustomSamplingContext(customSamplingContext); + transactionOptions.setBindToScope(true); + + if (sentryTraceHeaders != null && sentryTraceHeaders.size() > 0) { + final @NotNull Baggage baggage = + Baggage.fromHeader(baggageHeaders, hub.getOptions().getLogger()); + try { + final @NotNull TransactionContext contexts = + TransactionContext.fromSentryTrace( + name, + TransactionNameSource.URL, + TRANSACTION_OP, + new SentryTraceHeader(sentryTraceHeaders.get(0)), + baggage, + null); + + return hub.startTransaction(contexts, transactionOptions); + } catch (InvalidSentryTraceHeaderException e) { + hub.getOptions() + .getLogger() + .log(SentryLevel.DEBUG, e, "Failed to parse Sentry trace header: %s", e.getMessage()); + } + } + + return hub.startTransaction( + new TransactionContext(name, TransactionNameSource.URL, TRANSACTION_OP), + transactionOptions); } } diff --git a/sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/webflux/SentryWebFilter.java b/sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/webflux/SentryWebFilter.java index 4d7365bf04..1e4cd9b550 100644 --- a/sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/webflux/SentryWebFilter.java +++ b/sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/webflux/SentryWebFilter.java @@ -2,10 +2,12 @@ import com.jakewharton.nopen.annotation.Open; import io.sentry.IHub; -import io.sentry.NoOpHub; +import io.sentry.ITransaction; import io.sentry.Sentry; import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; +import org.springframework.http.server.reactive.ServerHttpRequest; import org.springframework.web.server.ServerWebExchange; import org.springframework.web.server.WebFilterChain; import reactor.core.publisher.Mono; @@ -24,13 +26,12 @@ public Mono filter( final @NotNull ServerWebExchange serverWebExchange, final @NotNull WebFilterChain webFilterChain) { @NotNull IHub requestHub = Sentry.cloneMainHub(); + final ServerHttpRequest request = serverWebExchange.getRequest(); + final @Nullable ITransaction transaction = maybeStartTransaction(requestHub, request); return webFilterChain .filter(serverWebExchange) - .doFinally( - __ -> { - doFinally(requestHub); - Sentry.setCurrentHub(NoOpHub.getInstance()); - }) + .doFinally(__ -> doFinally(serverWebExchange, requestHub, transaction)) + .doOnError(e -> doOnError(transaction, e)) .doFirst( () -> { Sentry.setCurrentHub(requestHub); diff --git a/sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/webflux/SentryWebFilterWithThreadLocalAccessor.java b/sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/webflux/SentryWebFilterWithThreadLocalAccessor.java index 5f85b31efc..bd1b810fe0 100644 --- a/sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/webflux/SentryWebFilterWithThreadLocalAccessor.java +++ b/sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/webflux/SentryWebFilterWithThreadLocalAccessor.java @@ -1,10 +1,11 @@ package io.sentry.spring.jakarta.webflux; import io.sentry.IHub; -import io.sentry.NoOpHub; +import io.sentry.ITransaction; import io.sentry.Sentry; import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; import org.springframework.web.server.ServerWebExchange; import org.springframework.web.server.WebFilterChain; import reactor.core.publisher.Mono; @@ -21,17 +22,26 @@ public SentryWebFilterWithThreadLocalAccessor(final @NotNull IHub hub) { public Mono filter( final @NotNull ServerWebExchange serverWebExchange, final @NotNull WebFilterChain webFilterChain) { + final @NotNull TransactionContainer transactionContainer = new TransactionContainer(); return ReactorUtils.withSentryNewMainHubClone( webFilterChain .filter(serverWebExchange) .doFinally( - __ -> { - doFinally(Sentry.getCurrentHub()); - Sentry.setCurrentHub(NoOpHub.getInstance()); - }) + __ -> + doFinally( + serverWebExchange, + Sentry.getCurrentHub(), + transactionContainer.transaction)) + .doOnError(e -> doOnError(transactionContainer.transaction, e)) .doFirst( () -> { doFirst(serverWebExchange, Sentry.getCurrentHub()); + transactionContainer.transaction = + maybeStartTransaction(Sentry.getCurrentHub(), serverWebExchange.getRequest()); })); } + + private static class TransactionContainer { + private volatile @Nullable ITransaction transaction; + } } diff --git a/sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/webflux/SentryWebTracingFilter.java b/sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/webflux/SentryWebTracingFilter.java deleted file mode 100644 index a222e67a50..0000000000 --- a/sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/webflux/SentryWebTracingFilter.java +++ /dev/null @@ -1,123 +0,0 @@ -package io.sentry.spring.jakarta.webflux; - -import static io.sentry.spring.jakarta.webflux.AbstractSentryWebFilter.SENTRY_HUB_KEY; - -import com.jakewharton.nopen.annotation.Open; -import io.sentry.Baggage; -import io.sentry.BaggageHeader; -import io.sentry.CustomSamplingContext; -import io.sentry.IHub; -import io.sentry.ITransaction; -import io.sentry.Sentry; -import io.sentry.SentryLevel; -import io.sentry.SentryTraceHeader; -import io.sentry.SpanStatus; -import io.sentry.TransactionContext; -import io.sentry.TransactionOptions; -import io.sentry.exception.InvalidSentryTraceHeaderException; -import io.sentry.protocol.TransactionNameSource; -import java.util.List; -import org.jetbrains.annotations.ApiStatus; -import org.jetbrains.annotations.NotNull; -import org.jetbrains.annotations.Nullable; -import org.springframework.http.HttpHeaders; -import org.springframework.http.HttpMethod; -import org.springframework.http.HttpStatusCode; -import org.springframework.http.server.reactive.ServerHttpRequest; -import org.springframework.http.server.reactive.ServerHttpResponse; -import org.springframework.web.server.ServerWebExchange; -import org.springframework.web.server.WebFilter; -import org.springframework.web.server.WebFilterChain; -import reactor.core.publisher.Mono; - -@Open -@ApiStatus.Experimental -public class SentryWebTracingFilter implements WebFilter { - - private static final String TRANSACTION_OP = "http.server"; - - @Override - public Mono filter(ServerWebExchange exchange, WebFilterChain chain) { - final @Nullable Object hubObject = exchange.getAttributes().getOrDefault(SENTRY_HUB_KEY, null); - final @NotNull IHub hub = hubObject == null ? Sentry.getCurrentHub() : (IHub) hubObject; - final @NotNull ServerHttpRequest request = exchange.getRequest(); - - if (hub.isEnabled() && shouldTraceRequest(hub, request)) { - final @NotNull ITransaction transaction = startTransaction(hub, request); - - return chain - .filter(exchange) - .doFinally( - __ -> { - String transactionName = TransactionNameProvider.provideTransactionName(exchange); - if (transactionName != null) { - transaction.setName(transactionName, TransactionNameSource.ROUTE); - transaction.setOperation(TRANSACTION_OP); - } - if (transaction.getStatus() == null) { - final @Nullable ServerHttpResponse response = exchange.getResponse(); - if (response != null) { - final @Nullable HttpStatusCode statusCode = response.getStatusCode(); - if (statusCode != null) { - transaction.setStatus(SpanStatus.fromHttpStatusCode(statusCode.value())); - } - } - } - transaction.finish(); - }) - .doOnError( - e -> { - transaction.setStatus(SpanStatus.INTERNAL_ERROR); - transaction.setThrowable(e); - }); - } else { - return chain.filter(exchange); - } - } - - private boolean shouldTraceRequest( - final @NotNull IHub hub, final @NotNull ServerHttpRequest request) { - return hub.getOptions().isTraceOptionsRequests() - || !HttpMethod.OPTIONS.equals(request.getMethod()); - } - - private @NotNull ITransaction startTransaction( - final @NotNull IHub hub, final @NotNull ServerHttpRequest request) { - final @NotNull HttpHeaders headers = request.getHeaders(); - final @Nullable List sentryTraceHeaders = - headers.get(SentryTraceHeader.SENTRY_TRACE_HEADER); - final @Nullable List baggageHeaders = headers.get(BaggageHeader.BAGGAGE_HEADER); - final @NotNull String name = request.getMethod() + " " + request.getURI().getPath(); - final @NotNull CustomSamplingContext customSamplingContext = new CustomSamplingContext(); - customSamplingContext.set("request", request); - - final TransactionOptions transactionOptions = new TransactionOptions(); - transactionOptions.setCustomSamplingContext(customSamplingContext); - transactionOptions.setBindToScope(true); - - if (sentryTraceHeaders != null && sentryTraceHeaders.size() > 0) { - final @NotNull Baggage baggage = - Baggage.fromHeader(baggageHeaders, hub.getOptions().getLogger()); - try { - final @NotNull TransactionContext contexts = - TransactionContext.fromSentryTrace( - name, - TransactionNameSource.URL, - TRANSACTION_OP, - new SentryTraceHeader(sentryTraceHeaders.get(0)), - baggage, - null); - - return hub.startTransaction(contexts, transactionOptions); - } catch (InvalidSentryTraceHeaderException e) { - hub.getOptions() - .getLogger() - .log(SentryLevel.DEBUG, e, "Failed to parse Sentry trace header: %s", e.getMessage()); - } - } - - return hub.startTransaction( - new TransactionContext(name, TransactionNameSource.URL, TRANSACTION_OP), - transactionOptions); - } -} diff --git a/sentry-spring-jakarta/src/test/kotlin/io/sentry/spring/jakarta/webflux/SentryWebFluxTracingFilterTest.kt b/sentry-spring-jakarta/src/test/kotlin/io/sentry/spring/jakarta/webflux/SentryWebFluxTracingFilterTest.kt index 3944df69e7..8cb30dcb50 100644 --- a/sentry-spring-jakarta/src/test/kotlin/io/sentry/spring/jakarta/webflux/SentryWebFluxTracingFilterTest.kt +++ b/sentry-spring-jakarta/src/test/kotlin/io/sentry/spring/jakarta/webflux/SentryWebFluxTracingFilterTest.kt @@ -1,6 +1,10 @@ package io.sentry.spring.jakarta.webflux +import io.sentry.Breadcrumb +import io.sentry.Hint import io.sentry.IHub +import io.sentry.ScopeCallback +import io.sentry.Sentry import io.sentry.SentryOptions import io.sentry.SentryTracer import io.sentry.SpanId @@ -12,10 +16,12 @@ import io.sentry.protocol.SentryId import io.sentry.protocol.TransactionNameSource import io.sentry.spring.jakarta.webflux.AbstractSentryWebFilter.SENTRY_HUB_KEY 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.mock +import org.mockito.kotlin.times import org.mockito.kotlin.verify import org.mockito.kotlin.verifyNoMoreInteractions import org.mockito.kotlin.whenever @@ -49,7 +55,7 @@ class SentryWebFluxTracingFilterTest { whenever(hub.options).thenReturn(options) } - fun getSut(isEnabled: Boolean = true, status: HttpStatus = HttpStatus.OK, sentryTraceHeader: String? = null, method: HttpMethod = HttpMethod.POST): SentryWebTracingFilter { + fun getSut(isEnabled: Boolean = true, status: HttpStatus = HttpStatus.OK, sentryTraceHeader: String? = null, method: HttpMethod = HttpMethod.POST): SentryWebFilter { var requestBuilder = MockServerHttpRequest.method(method, "/product/{id}", 12) if (sentryTraceHeader != null) { requestBuilder = requestBuilder.header("sentry-trace", sentryTraceHeader) @@ -63,89 +69,101 @@ class SentryWebFluxTracingFilterTest { whenever(hub.startTransaction(any(), check { assertTrue(it.isBindToScope) })).thenAnswer { SentryTracer(it.arguments[0] as TransactionContext, hub) } whenever(hub.isEnabled).thenReturn(isEnabled) whenever(chain.filter(any())).thenReturn(Mono.create { s -> s.success() }) - return SentryWebTracingFilter() + return SentryWebFilter(hub) } } private val fixture = Fixture() + fun withMockHub(closure: () -> Unit) = Mockito.mockStatic(Sentry::class.java).use { + it.`when` { Sentry.cloneMainHub() }.thenReturn(fixture.hub) + closure.invoke() + } + @Test fun `creates transaction around the request`() { val filter = fixture.getSut() + withMockHub { + filter.filter(fixture.exchange, fixture.chain).block() - filter.filter(fixture.exchange, fixture.chain).block() - - verify(fixture.hub).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 ServerHttpRequest) - assertTrue(it.isBindToScope) - } - ) - verify(fixture.chain).filter(fixture.exchange) - verify(fixture.hub).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.hub).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 ServerHttpRequest) + assertTrue(it.isBindToScope) + } + ) + verify(fixture.chain).filter(fixture.exchange) + verify(fixture.hub).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() + ) + } } @Test fun `sets correct span status based on the response status`() { val filter = fixture.getSut(status = HttpStatus.INTERNAL_SERVER_ERROR) - filter.filter(fixture.exchange, fixture.chain).block() + withMockHub { + filter.filter(fixture.exchange, fixture.chain).block() - verify(fixture.hub).captureTransaction( - check { - assertThat(it.contexts.trace!!.status).isEqualTo(SpanStatus.INTERNAL_ERROR) - }, - anyOrNull(), - anyOrNull(), - anyOrNull() - ) + verify(fixture.hub).captureTransaction( + check { + assertThat(it.contexts.trace!!.status).isEqualTo(SpanStatus.INTERNAL_ERROR) + }, + anyOrNull(), + anyOrNull(), + anyOrNull() + ) + } } @Test fun `does not set span status for response status that dont match predefined span statuses`() { val filter = fixture.getSut(status = HttpStatus.FOUND) - filter.filter(fixture.exchange, fixture.chain).block() + withMockHub { + filter.filter(fixture.exchange, fixture.chain).block() - verify(fixture.hub).captureTransaction( - check { - assertThat(it.contexts.trace!!.status).isNull() - }, - anyOrNull(), - anyOrNull(), - anyOrNull() - ) + verify(fixture.hub).captureTransaction( + check { + assertThat(it.contexts.trace!!.status).isNull() + }, + anyOrNull(), + anyOrNull(), + anyOrNull() + ) + } } @Test fun `when sentry trace is not present, transaction does not have parentSpanId set`() { val filter = fixture.getSut() - filter.filter(fixture.exchange, fixture.chain).block() + withMockHub { + filter.filter(fixture.exchange, fixture.chain).block() - verify(fixture.hub).captureTransaction( - check { - assertThat(it.contexts.trace!!.parentSpanId).isNull() - }, - anyOrNull(), - anyOrNull(), - anyOrNull() - ) + verify(fixture.hub).captureTransaction( + check { + assertThat(it.contexts.trace!!.parentSpanId).isNull() + }, + anyOrNull(), + anyOrNull(), + anyOrNull() + ) + } } @Test @@ -153,99 +171,119 @@ class SentryWebFluxTracingFilterTest { val parentSpanId = SpanId() val filter = fixture.getSut(sentryTraceHeader = "${SentryId()}-$parentSpanId-1") - filter.filter(fixture.exchange, fixture.chain).block() + withMockHub { + filter.filter(fixture.exchange, fixture.chain).block() - verify(fixture.hub).captureTransaction( - check { - assertThat(it.contexts.trace!!.parentSpanId).isEqualTo(parentSpanId) - }, - anyOrNull(), - anyOrNull(), - anyOrNull() - ) + verify(fixture.hub).captureTransaction( + check { + assertThat(it.contexts.trace!!.parentSpanId).isEqualTo(parentSpanId) + }, + anyOrNull(), + anyOrNull(), + anyOrNull() + ) + } } @Test fun `when hub is disabled, components are not invoked`() { val filter = fixture.getSut(isEnabled = false) - filter.filter(fixture.exchange, fixture.chain).block() + withMockHub { + filter.filter(fixture.exchange, fixture.chain).block() - verify(fixture.chain).filter(fixture.exchange) + verify(fixture.chain).filter(fixture.exchange) - verify(fixture.hub).isEnabled - verifyNoMoreInteractions(fixture.hub) + verify(fixture.hub, times(3)).isEnabled + verifyNoMoreInteractions(fixture.hub) + } } @Test fun `sets status to internal server error when chain throws exception`() { val filter = fixture.getSut() - whenever(fixture.chain.filter(any())).thenReturn(Mono.error(RuntimeException("error"))) - try { - filter.filter(fixture.exchange, fixture.chain).block() - fail("filter is expected to rethrow exception") - } catch (_: Exception) { + withMockHub { + whenever(fixture.chain.filter(any())).thenReturn(Mono.error(RuntimeException("error"))) + + try { + filter.filter(fixture.exchange, fixture.chain).block() + fail("filter is expected to rethrow exception") + } catch (_: Exception) { + } + verify(fixture.hub).captureTransaction( + check { + assertThat(it.status).isEqualTo(SpanStatus.INTERNAL_ERROR) + }, + anyOrNull(), + anyOrNull(), + anyOrNull() + ) } - verify(fixture.hub).captureTransaction( - check { - assertThat(it.status).isEqualTo(SpanStatus.INTERNAL_ERROR) - }, - anyOrNull(), - anyOrNull(), - anyOrNull() - ) } @Test fun `does not track OPTIONS request with traceOptionsRequests=false`() { val filter = fixture.getSut(method = HttpMethod.OPTIONS) - fixture.options.isTraceOptionsRequests = false - filter.filter(fixture.exchange, fixture.chain).block() + withMockHub { + fixture.options.isTraceOptionsRequests = false - verify(fixture.chain).filter(fixture.exchange) + filter.filter(fixture.exchange, fixture.chain).block() + + verify(fixture.chain).filter(fixture.exchange) - verify(fixture.hub).isEnabled - verify(fixture.hub).options - verifyNoMoreInteractions(fixture.hub) + verify(fixture.hub, times(3)).isEnabled + verify(fixture.hub, times(2)).options + verify(fixture.hub).pushScope() + verify(fixture.hub).addBreadcrumb(any(), any()) + verify(fixture.hub).configureScope(any()) + verify(fixture.hub).popScope() + verifyNoMoreInteractions(fixture.hub) + } } @Test fun `tracks OPTIONS request with traceOptionsRequests=true`() { val filter = fixture.getSut(method = HttpMethod.OPTIONS) - fixture.options.isTraceOptionsRequests = true - filter.filter(fixture.exchange, fixture.chain).block() + withMockHub { + fixture.options.isTraceOptionsRequests = true - verify(fixture.chain).filter(fixture.exchange) + filter.filter(fixture.exchange, fixture.chain).block() + + verify(fixture.chain).filter(fixture.exchange) - verify(fixture.hub).captureTransaction( - check { - assertThat(it.contexts.trace!!.parentSpanId).isNull() - }, - anyOrNull(), - anyOrNull(), - anyOrNull() - ) + verify(fixture.hub).captureTransaction( + check { + assertThat(it.contexts.trace!!.parentSpanId).isNull() + }, + anyOrNull(), + anyOrNull(), + anyOrNull() + ) + } } @Test fun `tracks POST request with traceOptionsRequests=false`() { val filter = fixture.getSut(method = HttpMethod.POST) - fixture.options.isTraceOptionsRequests = false - filter.filter(fixture.exchange, fixture.chain).block() + withMockHub { + fixture.options.isTraceOptionsRequests = false - verify(fixture.chain).filter(fixture.exchange) + filter.filter(fixture.exchange, fixture.chain).block() + + verify(fixture.chain).filter(fixture.exchange) - verify(fixture.hub).captureTransaction( - check { - assertThat(it.contexts.trace!!.parentSpanId).isNull() - }, - anyOrNull(), - anyOrNull(), - anyOrNull() - ) + verify(fixture.hub).captureTransaction( + check { + assertThat(it.contexts.trace!!.parentSpanId).isNull() + }, + anyOrNull(), + anyOrNull(), + anyOrNull() + ) + } } } diff --git a/sentry-spring-jakarta/src/test/kotlin/io/sentry/spring/jakarta/webflux/SentryWebfluxIntegrationTest.kt b/sentry-spring-jakarta/src/test/kotlin/io/sentry/spring/jakarta/webflux/SentryWebfluxIntegrationTest.kt index fadcac27e7..8dbbd05ebd 100644 --- a/sentry-spring-jakarta/src/test/kotlin/io/sentry/spring/jakarta/webflux/SentryWebfluxIntegrationTest.kt +++ b/sentry-spring-jakarta/src/test/kotlin/io/sentry/spring/jakarta/webflux/SentryWebfluxIntegrationTest.kt @@ -166,9 +166,6 @@ open class App { @Bean open fun sentryWebExceptionHandler(hub: IHub) = SentryWebExceptionHandler(hub) - @Bean - open fun sentryTracingFilter(hub: IHub) = SentryWebTracingFilter() - @Bean open fun sentryScheduleHookRegistrar() = ApplicationRunner { Schedulers.onScheduleHook("sentry", SentryScheduleHook()) diff --git a/sentry-spring/api/sentry-spring.api b/sentry-spring/api/sentry-spring.api index 59b2819784..c7ee1d0c70 100644 --- a/sentry-spring/api/sentry-spring.api +++ b/sentry-spring/api/sentry-spring.api @@ -183,8 +183,3 @@ public final class io/sentry/spring/webflux/SentryWebFilter : org/springframewor public fun filter (Lorg/springframework/web/server/ServerWebExchange;Lorg/springframework/web/server/WebFilterChain;)Lreactor/core/publisher/Mono; } -public class io/sentry/spring/webflux/SentryWebTracingFilter : org/springframework/web/server/WebFilter { - public fun ()V - public fun filter (Lorg/springframework/web/server/ServerWebExchange;Lorg/springframework/web/server/WebFilterChain;)Lreactor/core/publisher/Mono; -} - diff --git a/sentry-spring/build.gradle.kts b/sentry-spring/build.gradle.kts index c314e8e5fe..cc23000561 100644 --- a/sentry-spring/build.gradle.kts +++ b/sentry-spring/build.gradle.kts @@ -47,6 +47,7 @@ dependencies { testImplementation(kotlin(Config.kotlinStdLib)) testImplementation(Config.TestLibs.kotlinTestJunit) testImplementation(Config.TestLibs.mockitoKotlin) + testImplementation(Config.TestLibs.mockitoInline) testImplementation(Config.Libs.springBootStarterTest) testImplementation(Config.Libs.springBootStarterWeb) testImplementation(Config.Libs.springBootStarterWebflux) diff --git a/sentry-spring/src/main/java/io/sentry/spring/webflux/SentryWebFilter.java b/sentry-spring/src/main/java/io/sentry/spring/webflux/SentryWebFilter.java index 06e4e133c2..6e29739282 100644 --- a/sentry-spring/src/main/java/io/sentry/spring/webflux/SentryWebFilter.java +++ b/sentry-spring/src/main/java/io/sentry/spring/webflux/SentryWebFilter.java @@ -3,14 +3,29 @@ import static io.sentry.TypeCheckHint.WEBFLUX_FILTER_REQUEST; import static io.sentry.TypeCheckHint.WEBFLUX_FILTER_RESPONSE; +import io.sentry.Baggage; +import io.sentry.BaggageHeader; import io.sentry.Breadcrumb; +import io.sentry.CustomSamplingContext; import io.sentry.Hint; import io.sentry.IHub; +import io.sentry.ITransaction; import io.sentry.NoOpHub; import io.sentry.Sentry; +import io.sentry.SentryLevel; +import io.sentry.SentryTraceHeader; +import io.sentry.SpanStatus; +import io.sentry.TransactionContext; +import io.sentry.TransactionOptions; +import io.sentry.exception.InvalidSentryTraceHeaderException; +import io.sentry.protocol.TransactionNameSource; import io.sentry.util.Objects; +import java.util.List; import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; +import org.springframework.http.HttpHeaders; +import org.springframework.http.HttpMethod; import org.springframework.http.server.reactive.ServerHttpRequest; import org.springframework.http.server.reactive.ServerHttpResponse; import org.springframework.web.server.ServerWebExchange; @@ -22,6 +37,7 @@ @ApiStatus.Experimental public final class SentryWebFilter implements WebFilter { public static final String SENTRY_HUB_KEY = "sentry-hub"; + private static final String TRANSACTION_OP = "http.server"; private final @NotNull SentryRequestResolver sentryRequestResolver; @@ -35,19 +51,39 @@ public Mono filter( final @NotNull ServerWebExchange serverWebExchange, final @NotNull WebFilterChain webFilterChain) { @NotNull IHub requestHub = Sentry.cloneMainHub(); + if (!requestHub.isEnabled()) { + return webFilterChain.filter(serverWebExchange); + } + + final boolean isTracingEnabled = requestHub.getOptions().isTracingEnabled(); + final @NotNull ServerHttpRequest request = serverWebExchange.getRequest(); + final @Nullable ITransaction transaction = + isTracingEnabled && shouldTraceRequest(requestHub, request) + ? startTransaction(requestHub, request) + : null; + return webFilterChain .filter(serverWebExchange) .doFinally( __ -> { + if (transaction != null) { + finishTransaction(serverWebExchange, transaction); + } requestHub.popScope(); Sentry.setCurrentHub(NoOpHub.getInstance()); }) + .doOnError( + e -> { + if (transaction != null) { + transaction.setStatus(SpanStatus.INTERNAL_ERROR); + transaction.setThrowable(e); + } + }) .doFirst( () -> { serverWebExchange.getAttributes().put(SENTRY_HUB_KEY, requestHub); Sentry.setCurrentHub(requestHub); requestHub.pushScope(); - final ServerHttpRequest request = serverWebExchange.getRequest(); final ServerHttpResponse response = serverWebExchange.getResponse(); final Hint hint = new Hint(); @@ -61,4 +97,68 @@ public Mono filter( scope -> scope.setRequest(sentryRequestResolver.resolveSentryRequest(request))); }); } + + private boolean shouldTraceRequest( + final @NotNull IHub hub, final @NotNull ServerHttpRequest request) { + return hub.getOptions().isTraceOptionsRequests() + || !HttpMethod.OPTIONS.equals(request.getMethod()); + } + + private @NotNull ITransaction startTransaction( + final @NotNull IHub hub, final @NotNull ServerHttpRequest request) { + final @NotNull HttpHeaders headers = request.getHeaders(); + final @Nullable List sentryTraceHeaders = + headers.get(SentryTraceHeader.SENTRY_TRACE_HEADER); + final @Nullable List baggageHeaders = headers.get(BaggageHeader.BAGGAGE_HEADER); + final @NotNull String name = request.getMethod() + " " + request.getURI().getPath(); + final @NotNull CustomSamplingContext customSamplingContext = new CustomSamplingContext(); + customSamplingContext.set("request", request); + + final TransactionOptions transactionOptions = new TransactionOptions(); + transactionOptions.setCustomSamplingContext(customSamplingContext); + transactionOptions.setBindToScope(true); + + if (sentryTraceHeaders != null && sentryTraceHeaders.size() > 0) { + final @NotNull Baggage baggage = + Baggage.fromHeader(baggageHeaders, hub.getOptions().getLogger()); + try { + final @NotNull TransactionContext contexts = + TransactionContext.fromSentryTrace( + name, + TransactionNameSource.URL, + TRANSACTION_OP, + new SentryTraceHeader(sentryTraceHeaders.get(0)), + baggage, + null); + + return hub.startTransaction(contexts, transactionOptions); + } catch (InvalidSentryTraceHeaderException e) { + hub.getOptions() + .getLogger() + .log(SentryLevel.DEBUG, e, "Failed to parse Sentry trace header: %s", e.getMessage()); + } + } + + return hub.startTransaction( + new TransactionContext(name, TransactionNameSource.URL, TRANSACTION_OP), + transactionOptions); + } + + private void finishTransaction(ServerWebExchange exchange, ITransaction transaction) { + String transactionName = TransactionNameProvider.provideTransactionName(exchange); + if (transactionName != null) { + transaction.setName(transactionName, TransactionNameSource.ROUTE); + transaction.setOperation(TRANSACTION_OP); + } + if (transaction.getStatus() == null) { + final @Nullable ServerHttpResponse response = exchange.getResponse(); + if (response != null) { + final @Nullable Integer rawStatusCode = response.getRawStatusCode(); + if (rawStatusCode != null) { + transaction.setStatus(SpanStatus.fromHttpStatusCode(rawStatusCode)); + } + } + } + transaction.finish(); + } } diff --git a/sentry-spring/src/main/java/io/sentry/spring/webflux/SentryWebTracingFilter.java b/sentry-spring/src/main/java/io/sentry/spring/webflux/SentryWebTracingFilter.java deleted file mode 100644 index 68384e879c..0000000000 --- a/sentry-spring/src/main/java/io/sentry/spring/webflux/SentryWebTracingFilter.java +++ /dev/null @@ -1,122 +0,0 @@ -package io.sentry.spring.webflux; - -import static io.sentry.spring.webflux.SentryWebFilter.SENTRY_HUB_KEY; - -import com.jakewharton.nopen.annotation.Open; -import io.sentry.Baggage; -import io.sentry.BaggageHeader; -import io.sentry.CustomSamplingContext; -import io.sentry.IHub; -import io.sentry.ITransaction; -import io.sentry.Sentry; -import io.sentry.SentryLevel; -import io.sentry.SentryTraceHeader; -import io.sentry.SpanStatus; -import io.sentry.TransactionContext; -import io.sentry.TransactionOptions; -import io.sentry.exception.InvalidSentryTraceHeaderException; -import io.sentry.protocol.TransactionNameSource; -import java.util.List; -import org.jetbrains.annotations.ApiStatus; -import org.jetbrains.annotations.NotNull; -import org.jetbrains.annotations.Nullable; -import org.springframework.http.HttpHeaders; -import org.springframework.http.HttpMethod; -import org.springframework.http.server.reactive.ServerHttpRequest; -import org.springframework.http.server.reactive.ServerHttpResponse; -import org.springframework.web.server.ServerWebExchange; -import org.springframework.web.server.WebFilter; -import org.springframework.web.server.WebFilterChain; -import reactor.core.publisher.Mono; - -@Open -@ApiStatus.Experimental -public class SentryWebTracingFilter implements WebFilter { - - private static final String TRANSACTION_OP = "http.server"; - - @Override - public Mono filter(ServerWebExchange exchange, WebFilterChain chain) { - final @Nullable Object hubObject = exchange.getAttributes().getOrDefault(SENTRY_HUB_KEY, null); - final @NotNull IHub hub = hubObject == null ? Sentry.getCurrentHub() : (IHub) hubObject; - final @NotNull ServerHttpRequest request = exchange.getRequest(); - - if (hub.isEnabled() && shouldTraceRequest(hub, request)) { - final @NotNull ITransaction transaction = startTransaction(hub, request); - - return chain - .filter(exchange) - .doFinally( - __ -> { - String transactionName = TransactionNameProvider.provideTransactionName(exchange); - if (transactionName != null) { - transaction.setName(transactionName, TransactionNameSource.ROUTE); - transaction.setOperation(TRANSACTION_OP); - } - if (transaction.getStatus() == null) { - final @Nullable ServerHttpResponse response = exchange.getResponse(); - if (response != null) { - final @Nullable Integer rawStatusCode = response.getRawStatusCode(); - if (rawStatusCode != null) { - transaction.setStatus(SpanStatus.fromHttpStatusCode(rawStatusCode)); - } - } - } - transaction.finish(); - }) - .doOnError( - e -> { - transaction.setStatus(SpanStatus.INTERNAL_ERROR); - transaction.setThrowable(e); - }); - } else { - return chain.filter(exchange); - } - } - - private boolean shouldTraceRequest( - final @NotNull IHub hub, final @NotNull ServerHttpRequest request) { - return hub.getOptions().isTraceOptionsRequests() - || !HttpMethod.OPTIONS.equals(request.getMethod()); - } - - private @NotNull ITransaction startTransaction( - final @NotNull IHub hub, final @NotNull ServerHttpRequest request) { - final @NotNull HttpHeaders headers = request.getHeaders(); - final @Nullable List sentryTraceHeaders = - headers.get(SentryTraceHeader.SENTRY_TRACE_HEADER); - final @Nullable List baggageHeaders = headers.get(BaggageHeader.BAGGAGE_HEADER); - final @NotNull String name = request.getMethod() + " " + request.getURI().getPath(); - final @NotNull CustomSamplingContext customSamplingContext = new CustomSamplingContext(); - customSamplingContext.set("request", request); - - final TransactionOptions transactionOptions = new TransactionOptions(); - transactionOptions.setCustomSamplingContext(customSamplingContext); - transactionOptions.setBindToScope(true); - - if (sentryTraceHeaders != null && sentryTraceHeaders.size() > 0) { - final @NotNull Baggage baggage = - Baggage.fromHeader(baggageHeaders, hub.getOptions().getLogger()); - try { - final @NotNull TransactionContext contexts = - TransactionContext.fromSentryTrace( - name, - TransactionNameSource.URL, - TRANSACTION_OP, - new SentryTraceHeader(sentryTraceHeaders.get(0)), - baggage, - null); - - return hub.startTransaction(contexts, transactionOptions); - } catch (InvalidSentryTraceHeaderException e) { - hub.getOptions() - .getLogger() - .log(SentryLevel.DEBUG, e, "Failed to parse Sentry trace header: %s", e.getMessage()); - } - } - - return hub.startTransaction( - new TransactionContext(name, TransactionNameSource.URL, TRANSACTION_OP), - transactionOptions); - } -} diff --git a/sentry-spring/src/test/kotlin/io/sentry/spring/webflux/SentryWebFluxTracingFilterTest.kt b/sentry-spring/src/test/kotlin/io/sentry/spring/webflux/SentryWebFluxTracingFilterTest.kt index 72f8d068b5..388dbd9446 100644 --- a/sentry-spring/src/test/kotlin/io/sentry/spring/webflux/SentryWebFluxTracingFilterTest.kt +++ b/sentry-spring/src/test/kotlin/io/sentry/spring/webflux/SentryWebFluxTracingFilterTest.kt @@ -1,6 +1,10 @@ package io.sentry.spring.webflux +import io.sentry.Breadcrumb +import io.sentry.Hint import io.sentry.IHub +import io.sentry.ScopeCallback +import io.sentry.Sentry import io.sentry.SentryOptions import io.sentry.SentryTracer import io.sentry.SpanId @@ -12,10 +16,12 @@ import io.sentry.protocol.SentryId import io.sentry.protocol.TransactionNameSource import io.sentry.spring.webflux.SentryWebFilter.SENTRY_HUB_KEY 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.mock +import org.mockito.kotlin.times import org.mockito.kotlin.verify import org.mockito.kotlin.verifyNoMoreInteractions import org.mockito.kotlin.whenever @@ -49,7 +55,7 @@ class SentryWebFluxTracingFilterTest { whenever(hub.options).thenReturn(options) } - fun getSut(isEnabled: Boolean = true, status: HttpStatus = HttpStatus.OK, sentryTraceHeader: String? = null, method: HttpMethod = HttpMethod.POST): SentryWebTracingFilter { + fun getSut(isEnabled: Boolean = true, status: HttpStatus = HttpStatus.OK, sentryTraceHeader: String? = null, method: HttpMethod = HttpMethod.POST): SentryWebFilter { var requestBuilder = MockServerHttpRequest.method(method, "/product/{id}", 12) if (sentryTraceHeader != null) { requestBuilder = requestBuilder.header("sentry-trace", sentryTraceHeader) @@ -63,89 +69,102 @@ class SentryWebFluxTracingFilterTest { whenever(hub.startTransaction(any(), check { assertTrue(it.isBindToScope) })).thenAnswer { SentryTracer(it.arguments[0] as TransactionContext, hub) } whenever(hub.isEnabled).thenReturn(isEnabled) whenever(chain.filter(any())).thenReturn(Mono.create { s -> s.success() }) - return SentryWebTracingFilter() + return SentryWebFilter(hub) } } private val fixture = Fixture() + fun withMockHub(closure: () -> Unit) = Mockito.mockStatic(Sentry::class.java).use { + it.`when` { Sentry.cloneMainHub() }.thenReturn(fixture.hub) + closure.invoke() + } + @Test fun `creates transaction around the request`() { val filter = fixture.getSut() - filter.filter(fixture.exchange, fixture.chain).block() - - verify(fixture.hub).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 ServerHttpRequest) - assertTrue(it.isBindToScope) - } - ) - verify(fixture.chain).filter(fixture.exchange) - verify(fixture.hub).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() - ) + withMockHub { + filter.filter(fixture.exchange, fixture.chain).block() + + verify(fixture.hub).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 ServerHttpRequest) + assertTrue(it.isBindToScope) + } + ) + verify(fixture.chain).filter(fixture.exchange) + verify(fixture.hub).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() + ) + } } @Test fun `sets correct span status based on the response status`() { val filter = fixture.getSut(status = HttpStatus.INTERNAL_SERVER_ERROR) - filter.filter(fixture.exchange, fixture.chain).block() + withMockHub { + filter.filter(fixture.exchange, fixture.chain).block() - verify(fixture.hub).captureTransaction( - check { - assertThat(it.contexts.trace!!.status).isEqualTo(SpanStatus.INTERNAL_ERROR) - }, - anyOrNull(), - anyOrNull(), - anyOrNull() - ) + verify(fixture.hub).captureTransaction( + check { + assertThat(it.contexts.trace!!.status).isEqualTo(SpanStatus.INTERNAL_ERROR) + }, + anyOrNull(), + anyOrNull(), + anyOrNull() + ) + } } @Test fun `does not set span status for response status that dont match predefined span statuses`() { val filter = fixture.getSut(status = HttpStatus.FOUND) - filter.filter(fixture.exchange, fixture.chain).block() + withMockHub { + filter.filter(fixture.exchange, fixture.chain).block() - verify(fixture.hub).captureTransaction( - check { - assertThat(it.contexts.trace!!.status).isNull() - }, - anyOrNull(), - anyOrNull(), - anyOrNull() - ) + verify(fixture.hub).captureTransaction( + check { + assertThat(it.contexts.trace!!.status).isNull() + }, + anyOrNull(), + anyOrNull(), + anyOrNull() + ) + } } @Test fun `when sentry trace is not present, transaction does not have parentSpanId set`() { val filter = fixture.getSut() - filter.filter(fixture.exchange, fixture.chain).block() + withMockHub { + filter.filter(fixture.exchange, fixture.chain).block() - verify(fixture.hub).captureTransaction( - check { - assertThat(it.contexts.trace!!.parentSpanId).isNull() - }, - anyOrNull(), - anyOrNull(), - anyOrNull() - ) + verify(fixture.hub).captureTransaction( + check { + assertThat(it.contexts.trace!!.parentSpanId).isNull() + }, + anyOrNull(), + anyOrNull(), + anyOrNull() + ) + } } @Test @@ -153,99 +172,119 @@ class SentryWebFluxTracingFilterTest { val parentSpanId = SpanId() val filter = fixture.getSut(sentryTraceHeader = "${SentryId()}-$parentSpanId-1") - filter.filter(fixture.exchange, fixture.chain).block() + withMockHub { + filter.filter(fixture.exchange, fixture.chain).block() - verify(fixture.hub).captureTransaction( - check { - assertThat(it.contexts.trace!!.parentSpanId).isEqualTo(parentSpanId) - }, - anyOrNull(), - anyOrNull(), - anyOrNull() - ) + verify(fixture.hub).captureTransaction( + check { + assertThat(it.contexts.trace!!.parentSpanId).isEqualTo(parentSpanId) + }, + anyOrNull(), + anyOrNull(), + anyOrNull() + ) + } } @Test fun `when hub is disabled, components are not invoked`() { val filter = fixture.getSut(isEnabled = false) - filter.filter(fixture.exchange, fixture.chain).block() + withMockHub { + filter.filter(fixture.exchange, fixture.chain).block() - verify(fixture.chain).filter(fixture.exchange) + verify(fixture.chain).filter(fixture.exchange) - verify(fixture.hub).isEnabled - verifyNoMoreInteractions(fixture.hub) + verify(fixture.hub).isEnabled + verifyNoMoreInteractions(fixture.hub) + } } @Test fun `sets status to internal server error when chain throws exception`() { val filter = fixture.getSut() - whenever(fixture.chain.filter(any())).thenReturn(Mono.error(RuntimeException("error"))) - try { - filter.filter(fixture.exchange, fixture.chain).block() - fail("filter is expected to rethrow exception") - } catch (_: Exception) { + withMockHub { + whenever(fixture.chain.filter(any())).thenReturn(Mono.error(RuntimeException("error"))) + + try { + filter.filter(fixture.exchange, fixture.chain).block() + fail("filter is expected to rethrow exception") + } catch (_: Exception) { + } + verify(fixture.hub).captureTransaction( + check { + assertThat(it.status).isEqualTo(SpanStatus.INTERNAL_ERROR) + }, + anyOrNull(), + anyOrNull(), + anyOrNull() + ) } - verify(fixture.hub).captureTransaction( - check { - assertThat(it.status).isEqualTo(SpanStatus.INTERNAL_ERROR) - }, - anyOrNull(), - anyOrNull(), - anyOrNull() - ) } @Test fun `does not track OPTIONS request with traceOptionsRequests=false`() { val filter = fixture.getSut(method = HttpMethod.OPTIONS) - fixture.options.isTraceOptionsRequests = false - filter.filter(fixture.exchange, fixture.chain).block() + withMockHub { + fixture.options.isTraceOptionsRequests = false + + filter.filter(fixture.exchange, fixture.chain).block() - verify(fixture.chain).filter(fixture.exchange) + verify(fixture.chain).filter(fixture.exchange) - verify(fixture.hub).isEnabled - verify(fixture.hub).options - verifyNoMoreInteractions(fixture.hub) + verify(fixture.hub).isEnabled + verify(fixture.hub, times(2)).options + verify(fixture.hub).pushScope() + verify(fixture.hub).addBreadcrumb(any(), any()) + verify(fixture.hub).configureScope(any()) + verify(fixture.hub).popScope() + verifyNoMoreInteractions(fixture.hub) + } } @Test fun `tracks OPTIONS request with traceOptionsRequests=true`() { val filter = fixture.getSut(method = HttpMethod.OPTIONS) - fixture.options.isTraceOptionsRequests = true - filter.filter(fixture.exchange, fixture.chain).block() + withMockHub { + fixture.options.isTraceOptionsRequests = true + + filter.filter(fixture.exchange, fixture.chain).block() - verify(fixture.chain).filter(fixture.exchange) + verify(fixture.chain).filter(fixture.exchange) - verify(fixture.hub).captureTransaction( - check { - assertThat(it.contexts.trace!!.parentSpanId).isNull() - }, - anyOrNull(), - anyOrNull(), - anyOrNull() - ) + verify(fixture.hub).captureTransaction( + check { + assertThat(it.contexts.trace!!.parentSpanId).isNull() + }, + anyOrNull(), + anyOrNull(), + anyOrNull() + ) + } } @Test fun `tracks POST request with traceOptionsRequests=false`() { val filter = fixture.getSut(method = HttpMethod.POST) - fixture.options.isTraceOptionsRequests = false - filter.filter(fixture.exchange, fixture.chain).block() + withMockHub { + fixture.options.isTraceOptionsRequests = false - verify(fixture.chain).filter(fixture.exchange) + filter.filter(fixture.exchange, fixture.chain).block() + + verify(fixture.chain).filter(fixture.exchange) - verify(fixture.hub).captureTransaction( - check { - assertThat(it.contexts.trace!!.parentSpanId).isNull() - }, - anyOrNull(), - anyOrNull(), - anyOrNull() - ) + verify(fixture.hub).captureTransaction( + check { + assertThat(it.contexts.trace!!.parentSpanId).isNull() + }, + anyOrNull(), + anyOrNull(), + anyOrNull() + ) + } } } diff --git a/sentry-spring/src/test/kotlin/io/sentry/spring/webflux/SentryWebfluxIntegrationTest.kt b/sentry-spring/src/test/kotlin/io/sentry/spring/webflux/SentryWebfluxIntegrationTest.kt index 7ec16372b0..94cd22f505 100644 --- a/sentry-spring/src/test/kotlin/io/sentry/spring/webflux/SentryWebfluxIntegrationTest.kt +++ b/sentry-spring/src/test/kotlin/io/sentry/spring/webflux/SentryWebfluxIntegrationTest.kt @@ -163,9 +163,6 @@ open class App { @Bean open fun sentryFilter(hub: IHub) = SentryWebFilter(hub) - @Bean - open fun sentryTracingFilter(hub: IHub) = SentryWebTracingFilter() - @Bean open fun sentryWebExceptionHandler(hub: IHub) = SentryWebExceptionHandler(hub)