Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Finish WebFlux transaction before popping scope #2724

Merged
merged 4 commits into from
May 30, 2023
Merged

Finish WebFlux transaction before popping scope #2724

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8324fd8
Merge SentryWebTracingFilter into SentryWebFilter to avoid ordering p…
adinauer May 22, 2023
bbb1440
Fix tests; add changelog
adinauer May 22, 2023
504a195
Merge branch 'main' into fix/finish-webflux-transaction-before-scope-pop
adinauer May 22, 2023
d832b5b
Merge branch 'main' into fix/finish-webflux-transaction-before-scope-pop
adinauer May 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))

Expand Down
1 change: 0 additions & 1 deletion sentry-spring-boot-starter-jakarta/api/sentry-spring-boot-starter-jakarta.api
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,5 @@ public class io/sentry/spring/boot/jakarta/SentryProperties$Reactive {
public class io/sentry/spring/boot/jakarta/SentryWebfluxAutoConfiguration {
public fun <init> ()V
public fun sentryWebExceptionHandler (Lio/sentry/IHub;)Lio/sentry/spring/jakarta/webflux/SentryWebExceptionHandler;
public fun sentryWebTracingFilter ()Lio/sentry/spring/jakarta/webflux/SentryWebTracingFilter;
}

10 changes: 0 additions & 10 deletions ...r-jakarta/src/main/java/io/sentry/spring/boot/jakarta/SentryWebfluxAutoConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,13 @@
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;
import org.springframework.boot.autoconfigure.condition.AllNestedConditions;
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;
Expand Down Expand Up @@ -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) {
Expand Down
1 change: 0 additions & 1 deletion sentry-spring-boot-starter/api/sentry-spring-boot-starter.api
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

11 changes: 0 additions & 11 deletions ...ring-boot-starter/src/main/java/io/sentry/spring/boot/SentryWebfluxAutoConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down
11 changes: 5 additions & 6 deletions sentry-spring-jakarta/api/sentry-spring-jakarta.api
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 <init> (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 {
Expand Down Expand Up @@ -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 <init> ()V
public fun filter (Lorg/springframework/web/server/ServerWebExchange;Lorg/springframework/web/server/WebFilterChain;)Lreactor/core/publisher/Mono;
}

1 change: 1 addition & 0 deletions sentry-spring-jakarta/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
141 changes: 127 additions & 14 deletions ...pring-jakarta/src/main/java/io/sentry/spring/jakarta/webflux/AbstractSentryWebFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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<String> sentryTraceHeaders =
headers.get(SentryTraceHeader.SENTRY_TRACE_HEADER);
final @Nullable List<String> 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);
}
}
13 changes: 7 additions & 6 deletions sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/webflux/SentryWebFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -24,13 +26,12 @@ public Mono<Void> 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);
Expand Down
20 changes: 15 additions & 5 deletions ...rc/main/java/io/sentry/spring/jakarta/webflux/SentryWebFilterWithThreadLocalAccessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -21,17 +22,26 @@ public SentryWebFilterWithThreadLocalAccessor(final @NotNull IHub hub) {
public Mono<Void> 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(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does Sentry.getCurrentHub() work correctly in this filter method? ReactorUtils.withSentryNewMainHubClone does not set the current hub, so shouldn't we get the hub from the context?
Or is this somehow handled by reactor behind the scenes?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ThreadLocal is set via ThreadLocalAccessor and propagated via reactor context.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tested again with debugger, works as expected (i.e. uses the correct hub).

transactionContainer.transaction))
.doOnError(e -> doOnError(transactionContainer.transaction, e))
.doFirst(
() -> {
doFirst(serverWebExchange, Sentry.getCurrentHub());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment about Sentry.getCurrentHub()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

transactionContainer.transaction =
maybeStartTransaction(Sentry.getCurrentHub(), serverWebExchange.getRequest());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment about Sentry.getCurrentHub()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

}));
}

private static class TransactionContainer {
private volatile @Nullable ITransaction transaction;
}
}
Loading