From fed0f164ece6161d23563ede606408b9239d659c Mon Sep 17 00:00:00 2001 From: Brian Clozel Date: Tue, 14 Mar 2023 17:16:56 +0100 Subject: [PATCH] =?UTF-8?q?Move=20reactive=20server=20instrumentation?= =?UTF-8?q?=C2=A0out=20of=20WebFilter?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prior to this commit, the Observation instrumentation for Reactive server applications was implemented with a `WebFilter`. This allowed to record observations and set up a tracing context for the controller handlers. The limitation of this approach is that all processing happening at a lower level is not aware of any observation. Here, the `HttpWebHandlerAdapter` handles several interesting aspects: * logging of HTTP requests and responses at the TRACE level * logging of client disconnect errors * handling of unresolved errors With the current instrumentation, these logging statements will miss the tracing context information. As a result, this commit deprecates the `ServerHttpObservationFilter` in favor of a more direct instrumentation of the `HttpWebHandlerAdapter`. This enables a more precise instrumentattion and allows to set up the current observation earlier in the reactor context: log statements will now contain the relevant information. Fixes gh-30013 --- .../asciidoc/integration/observability.adoc | 11 +- .../reactive/HttpHandlerConfiguration.java | 42 +++++ .../httpserver/reactive/UserController.java | 4 +- .../ServerRequestObservationContext.java | 18 ++ .../reactive/ServerHttpObservationFilter.java | 5 +- .../server/adapter/HttpWebHandlerAdapter.java | 104 +++++++++++- .../server/adapter/WebHttpHandlerBuilder.java | 41 ++++- .../handler/ExceptionHandlingWebHandler.java | 12 +- .../ServerHttpObservationFilterTests.java | 3 +- ...tpWebHandlerAdapterObservabilityTests.java | 158 ++++++++++++++++++ .../ExceptionHandlingWebHandlerTests.java | 22 ++- .../server/support/RouterFunctionMapping.java | 7 +- .../handler/AbstractUrlHandlerMapping.java | 6 +- .../RequestMappingInfoHandlerMapping.java | 4 + .../support/RouterFunctionMappingTests.java | 10 +- ...RequestMappingInfoHandlerMappingTests.java | 4 +- 16 files changed, 420 insertions(+), 31 deletions(-) create mode 100644 framework-docs/src/main/java/org/springframework/docs/integration/observability/httpserver/reactive/HttpHandlerConfiguration.java create mode 100644 spring-web/src/test/java/org/springframework/web/server/adapter/HttpWebHandlerAdapterObservabilityTests.java diff --git a/framework-docs/src/docs/asciidoc/integration/observability.adoc b/framework-docs/src/docs/asciidoc/integration/observability.adoc index 735d6aeb3b7b..c54c85781289 100644 --- a/framework-docs/src/docs/asciidoc/integration/observability.adoc +++ b/framework-docs/src/docs/asciidoc/integration/observability.adoc @@ -121,12 +121,17 @@ By default, the following `KeyValues` are created: [[integration.observability.http-server.reactive]] === Reactive applications -Applications need to configure the `org.springframework.web.filter.reactive.ServerHttpObservationFilter` reactive `WebFilter` in their application. +Applications need to configure the `WebHttpHandlerBuilder` with a `MeterRegistry` to enable server instrumentation. +This can be done on the `WebHttpHandlerBuilder`, as follows: + +include::code:HttpHandlerConfiguration[] + It is using the `org.springframework.http.server.reactive.observation.DefaultServerRequestObservationConvention` by default, backed by the `ServerRequestObservationContext`. -This will only record an observation as an error if the `Exception` has not been handled by the web Framework and has bubbled up to the `WebFilter`. +This will only record an observation as an error if the `Exception` has not been handled by an application Controller. Typically, all exceptions handled by Spring WebFlux's `@ExceptionHandler` and <> will not be recorded with the observation. -You can, at any point during request processing, set the error field on the `ObservationContext` yourself: +Exceptions handled by configured `WebExceptionHandler` instances will be recorded as errors. +If you would like to customize this behavior, you can set, at any point during request processing, the error field on the `ObservationContext` yourself: include::code:UserController[] diff --git a/framework-docs/src/main/java/org/springframework/docs/integration/observability/httpserver/reactive/HttpHandlerConfiguration.java b/framework-docs/src/main/java/org/springframework/docs/integration/observability/httpserver/reactive/HttpHandlerConfiguration.java new file mode 100644 index 000000000000..dab8da25d5c0 --- /dev/null +++ b/framework-docs/src/main/java/org/springframework/docs/integration/observability/httpserver/reactive/HttpHandlerConfiguration.java @@ -0,0 +1,42 @@ +/* + * Copyright 2002-2023 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.docs.integration.observability.httpserver.reactive; + +import io.micrometer.observation.ObservationRegistry; + +import org.springframework.context.ApplicationContext; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import org.springframework.http.server.reactive.HttpHandler; +import org.springframework.web.server.adapter.WebHttpHandlerBuilder; + +@Configuration(proxyBeanMethods = false) +public class HttpHandlerConfiguration { + + private final ApplicationContext applicationContext; + + public HttpHandlerConfiguration(ApplicationContext applicationContext) { + this.applicationContext = applicationContext; + } + + @Bean + public HttpHandler httpHandler(ObservationRegistry registry) { + return WebHttpHandlerBuilder.applicationContext(this.applicationContext) + .observationRegistry(registry) + .build(); + } +} diff --git a/framework-docs/src/main/java/org/springframework/docs/integration/observability/httpserver/reactive/UserController.java b/framework-docs/src/main/java/org/springframework/docs/integration/observability/httpserver/reactive/UserController.java index 0f7d8b68a664..2cd35a755cc0 100644 --- a/framework-docs/src/main/java/org/springframework/docs/integration/observability/httpserver/reactive/UserController.java +++ b/framework-docs/src/main/java/org/springframework/docs/integration/observability/httpserver/reactive/UserController.java @@ -17,9 +17,9 @@ package org.springframework.docs.integration.observability.httpserver.reactive; import org.springframework.http.ResponseEntity; +import org.springframework.http.server.reactive.observation.ServerRequestObservationContext; import org.springframework.stereotype.Controller; import org.springframework.web.bind.annotation.ExceptionHandler; -import org.springframework.web.filter.reactive.ServerHttpObservationFilter; import org.springframework.web.server.ServerWebExchange; @Controller @@ -28,7 +28,7 @@ public class UserController { @ExceptionHandler(MissingUserException.class) ResponseEntity handleMissingUser(ServerWebExchange exchange, MissingUserException exception) { // We want to record this exception with the observation - ServerHttpObservationFilter.findObservationContext(exchange) + ServerRequestObservationContext.findCurrent(exchange) .ifPresent(context -> context.setError(exception)); return ResponseEntity.notFound().build(); } diff --git a/spring-web/src/main/java/org/springframework/http/server/reactive/observation/ServerRequestObservationContext.java b/spring-web/src/main/java/org/springframework/http/server/reactive/observation/ServerRequestObservationContext.java index 070d86373455..16db5ef5c806 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/observation/ServerRequestObservationContext.java +++ b/spring-web/src/main/java/org/springframework/http/server/reactive/observation/ServerRequestObservationContext.java @@ -18,12 +18,14 @@ import java.util.Collections; import java.util.Map; +import java.util.Optional; import io.micrometer.observation.transport.RequestReplyReceiverContext; import org.springframework.http.server.reactive.ServerHttpRequest; import org.springframework.http.server.reactive.ServerHttpResponse; import org.springframework.lang.Nullable; +import org.springframework.web.server.ServerWebExchange; /** * Context that holds information for metadata collection regarding @@ -36,6 +38,12 @@ */ public class ServerRequestObservationContext extends RequestReplyReceiverContext { + /** + * Name of the request attribute holding the {@link ServerRequestObservationContext context} for the current observation. + * @since 6.1.0 + */ + public static final String CURRENT_OBSERVATION_CONTEXT_ATTRIBUTE = ServerRequestObservationContext.class.getName() + ".context"; + private final Map attributes; @Nullable @@ -50,6 +58,16 @@ public ServerRequestObservationContext(ServerHttpRequest request, ServerHttpResp this.attributes = Collections.unmodifiableMap(attributes); } + /** + * Get the current {@link ServerRequestObservationContext observation context} from the given exchange, if available. + * @param exchange the current exchange + * @return the current observation context + * @since 6.1.0 + */ + public static Optional findCurrent(ServerWebExchange exchange) { + return Optional.ofNullable(exchange.getAttribute(CURRENT_OBSERVATION_CONTEXT_ATTRIBUTE)); + } + /** * Return an immutable map of the current request attributes. */ diff --git a/spring-web/src/main/java/org/springframework/web/filter/reactive/ServerHttpObservationFilter.java b/spring-web/src/main/java/org/springframework/web/filter/reactive/ServerHttpObservationFilter.java index ca75df7e7a15..c5c047080862 100644 --- a/spring-web/src/main/java/org/springframework/web/filter/reactive/ServerHttpObservationFilter.java +++ b/spring-web/src/main/java/org/springframework/web/filter/reactive/ServerHttpObservationFilter.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2022 the original author or authors. + * Copyright 2002-2023 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -23,6 +23,7 @@ import io.micrometer.observation.ObservationRegistry; import io.micrometer.observation.contextpropagation.ObservationThreadLocalAccessor; import org.reactivestreams.Publisher; +import org.springframework.web.server.adapter.WebHttpHandlerBuilder; import reactor.core.publisher.Mono; import org.springframework.http.server.reactive.ServerHttpResponse; @@ -47,7 +48,9 @@ * * @author Brian Clozel * @since 6.0 + * @deprecated since 6.1.0 in favor of {@link WebHttpHandlerBuilder}. */ +@Deprecated(since = "6.1.0", forRemoval = true) public class ServerHttpObservationFilter implements WebFilter { /** diff --git a/spring-web/src/main/java/org/springframework/web/server/adapter/HttpWebHandlerAdapter.java b/spring-web/src/main/java/org/springframework/web/server/adapter/HttpWebHandlerAdapter.java index 6c8405a56513..2ca930100dc9 100644 --- a/spring-web/src/main/java/org/springframework/web/server/adapter/HttpWebHandlerAdapter.java +++ b/spring-web/src/main/java/org/springframework/web/server/adapter/HttpWebHandlerAdapter.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2022 the original author or authors. + * Copyright 2002-2023 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -20,8 +20,12 @@ import java.util.Set; import java.util.function.Function; +import io.micrometer.observation.Observation; +import io.micrometer.observation.ObservationRegistry; +import io.micrometer.observation.contextpropagation.ObservationThreadLocalAccessor; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.reactivestreams.Publisher; import reactor.core.publisher.Mono; import org.springframework.context.ApplicationContext; @@ -36,11 +40,16 @@ import org.springframework.http.server.reactive.HttpHandler; import org.springframework.http.server.reactive.ServerHttpRequest; import org.springframework.http.server.reactive.ServerHttpResponse; +import org.springframework.http.server.reactive.observation.DefaultServerRequestObservationConvention; +import org.springframework.http.server.reactive.observation.ServerHttpObservationDocumentation; +import org.springframework.http.server.reactive.observation.ServerRequestObservationContext; +import org.springframework.http.server.reactive.observation.ServerRequestObservationConvention; import org.springframework.lang.Nullable; import org.springframework.util.Assert; import org.springframework.util.StringUtils; import org.springframework.web.server.ServerWebExchange; import org.springframework.web.server.WebHandler; +import org.springframework.web.server.handler.ExceptionHandlingWebHandler; import org.springframework.web.server.handler.WebHandlerDecorator; import org.springframework.web.server.i18n.AcceptHeaderLocaleContextResolver; import org.springframework.web.server.i18n.LocaleContextResolver; @@ -55,6 +64,7 @@ * * @author Rossen Stoyanchev * @author Sebastien Deleuze + * @author Brian Clozel * @since 5.0 */ public class HttpWebHandlerAdapter extends WebHandlerDecorator implements HttpHandler { @@ -75,6 +85,8 @@ public class HttpWebHandlerAdapter extends WebHandlerDecorator implements HttpHa private static final Set DISCONNECTED_CLIENT_EXCEPTIONS = Set.of("AbortedException", "ClientAbortException", "EOFException", "EofException"); + private static final ServerRequestObservationConvention DEFAULT_OBSERVATION_CONVENTION = new DefaultServerRequestObservationConvention(); + private static final Log logger = LogFactory.getLog(HttpWebHandlerAdapter.class); @@ -91,6 +103,12 @@ public class HttpWebHandlerAdapter extends WebHandlerDecorator implements HttpHa @Nullable private ForwardedHeaderTransformer forwardedHeaderTransformer; + @Nullable + private ObservationRegistry observationRegistry; + + @Nullable + private ServerRequestObservationConvention observationConvention; + @Nullable private ApplicationContext applicationContext; @@ -192,6 +210,44 @@ public ForwardedHeaderTransformer getForwardedHeaderTransformer() { return this.forwardedHeaderTransformer; } + /** + * Configure a {@link ObservationRegistry} for recording server exchange observations. + * By default, a {@link ObservationRegistry#NOOP no-op} instance will be used. + * @param observationRegistry the observation registry to use + * @since 6.1.0 + */ + public void setObservationRegistry(ObservationRegistry observationRegistry) { + this.observationRegistry = observationRegistry; + } + + /** + * Return the configured {@link ObservationRegistry}. + * @since 6.1.0 + */ + @Nullable + public ObservationRegistry getObservationRegistry() { + return this.observationRegistry; + } + + /** + * Configure a {@link ServerRequestObservationConvention} for server exchanges observations. + * By default, a {@link DefaultServerRequestObservationConvention} instance will be used. + * @param observationConvention the observation convention to use + * @since 6.1.0 + */ + public void setObservationConvention(ServerRequestObservationConvention observationConvention) { + this.observationConvention = observationConvention; + } + + /** + * Return the Observation convention configured for server exchanges observations. + * @since 6.1.0 + */ + @Nullable + public ServerRequestObservationConvention getObservationConvention() { + return this.observationConvention; + } + /** * Configure the {@code ApplicationContext} associated with the web application, * if it was initialized with one via @@ -247,9 +303,12 @@ public Mono handle(ServerHttpRequest request, ServerHttpResponse response) exchange.getLogPrefix() + formatRequest(exchange.getRequest()) + (traceOn ? ", headers=" + formatHeaders(exchange.getRequest().getHeaders()) : "")); + ServerRequestObservationContext observationContext = new ServerRequestObservationContext(exchange.getRequest(), + exchange.getResponse(), exchange.getAttributes()); + exchange.getAttributes().put(ServerRequestObservationContext.CURRENT_OBSERVATION_CONTEXT_ATTRIBUTE, observationContext); + return getDelegate().handle(exchange) - .doOnSuccess(aVoid -> logResponse(exchange)) - .onErrorResume(ex -> handleUnresolvedError(exchange, ex)) + .transformDeferred(call -> transform(exchange, observationContext, call)) .then(cleanupMultipart(exchange)) .then(Mono.defer(response::setComplete)); } @@ -271,6 +330,42 @@ protected String formatRequest(ServerHttpRequest request) { return "HTTP " + request.getMethod() + " \"" + request.getPath() + query + "\""; } + private Publisher transform(ServerWebExchange exchange, ServerRequestObservationContext observationContext, Mono call) { + Observation observation = ServerHttpObservationDocumentation.HTTP_REACTIVE_SERVER_REQUESTS.observation(this.observationConvention, + DEFAULT_OBSERVATION_CONVENTION, () -> observationContext, this.observationRegistry); + observation.start(); + return call + .doOnSuccess(aVoid -> { + logResponse(exchange); + stopObservation(observation, exchange); + }) + .onErrorResume(ex -> handleUnresolvedError(exchange, observationContext, ex)) + .doOnCancel(() -> cancelObservation(observationContext, observation)) + .contextWrite(context -> context.put(ObservationThreadLocalAccessor.KEY, observation)); + } + + private void stopObservation(Observation observation, ServerWebExchange exchange) { + Throwable throwable = exchange.getAttribute(ExceptionHandlingWebHandler.HANDLED_WEB_EXCEPTION); + if (throwable != null) { + observation.error(throwable); + } + ServerHttpResponse response = exchange.getResponse(); + if (response.isCommitted()) { + observation.stop(); + } + else { + response.beforeCommit(() -> { + observation.stop(); + return Mono.empty(); + }); + } + } + + private void cancelObservation(ServerRequestObservationContext observationContext, Observation observation) { + observationContext.setConnectionAborted(true); + observation.stop(); + } + private void logResponse(ServerWebExchange exchange) { LogFormatUtils.traceDebug(logger, traceOn -> { HttpStatusCode status = exchange.getResponse().getStatusCode(); @@ -284,7 +379,7 @@ private String formatHeaders(HttpHeaders responseHeaders) { responseHeaders.toString() : responseHeaders.isEmpty() ? "{}" : "{masked}"; } - private Mono handleUnresolvedError(ServerWebExchange exchange, Throwable ex) { + private Mono handleUnresolvedError(ServerWebExchange exchange, ServerRequestObservationContext observationContext, Throwable ex) { ServerHttpRequest request = exchange.getRequest(); ServerHttpResponse response = exchange.getResponse(); String logPrefix = exchange.getLogPrefix(); @@ -304,6 +399,7 @@ else if (lostClientLogger.isDebugEnabled()) { lostClientLogger.debug(logPrefix + "Client went away: " + ex + " (stacktrace at TRACE level for '" + DISCONNECTED_CLIENT_LOG_CATEGORY + "')"); } + observationContext.setConnectionAborted(true); return Mono.empty(); } else { diff --git a/spring-web/src/main/java/org/springframework/web/server/adapter/WebHttpHandlerBuilder.java b/spring-web/src/main/java/org/springframework/web/server/adapter/WebHttpHandlerBuilder.java index 0a6a87bada2d..d64f0e11cb92 100644 --- a/spring-web/src/main/java/org/springframework/web/server/adapter/WebHttpHandlerBuilder.java +++ b/spring-web/src/main/java/org/springframework/web/server/adapter/WebHttpHandlerBuilder.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2022 the original author or authors. + * Copyright 2002-2023 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -22,6 +22,7 @@ import java.util.function.Consumer; import java.util.function.Function; +import io.micrometer.observation.ObservationRegistry; import reactor.blockhound.BlockHound; import reactor.blockhound.integration.BlockHoundIntegration; @@ -31,6 +32,8 @@ import org.springframework.http.codec.ServerCodecConfigurer; import org.springframework.http.server.reactive.HttpHandler; import org.springframework.http.server.reactive.HttpHandlerDecoratorFactory; +import org.springframework.http.server.reactive.observation.DefaultServerRequestObservationConvention; +import org.springframework.http.server.reactive.observation.ServerRequestObservationConvention; import org.springframework.lang.Nullable; import org.springframework.util.Assert; import org.springframework.util.ObjectUtils; @@ -104,6 +107,12 @@ public final class WebHttpHandlerBuilder { @Nullable private ForwardedHeaderTransformer forwardedHeaderTransformer; + @Nullable + private ObservationRegistry observationRegistry; + + @Nullable + private ServerRequestObservationConvention observationConvention; + /** * Private constructor to use when initialized from an ApplicationContext. @@ -126,6 +135,8 @@ private WebHttpHandlerBuilder(WebHttpHandlerBuilder other) { this.codecConfigurer = other.codecConfigurer; this.localeContextResolver = other.localeContextResolver; this.forwardedHeaderTransformer = other.forwardedHeaderTransformer; + this.observationRegistry = other.observationRegistry; + this.observationConvention = other.observationConvention; this.httpHandlerDecorator = other.httpHandlerDecorator; } @@ -359,6 +370,28 @@ public boolean hasForwardedHeaderTransformer() { return (this.forwardedHeaderTransformer != null); } + /** + * Configure a {@link ObservationRegistry} for recording server exchange observations. + * By default, a {@link ObservationRegistry#NOOP no-op} registry will be configured. + * @param observationRegistry the observation registry + * @since 6.1.0 + */ + public WebHttpHandlerBuilder observationRegistry(ObservationRegistry observationRegistry) { + this.observationRegistry = observationRegistry; + return this; + } + + /** + * Configure a {@link ServerRequestObservationConvention} to use for server observations. + * By default, a {@link DefaultServerRequestObservationConvention} will be used. + * @param observationConvention the convention to use for all recorded observations + * @since 6.1.0 + */ + public WebHttpHandlerBuilder observationConvention(ServerRequestObservationConvention observationConvention) { + this.observationConvention = observationConvention; + return this; + } + /** * Configure a {@link Function} to decorate the {@link HttpHandler} returned * by this builder which effectively wraps the entire @@ -404,6 +437,12 @@ public HttpHandler build() { if (this.forwardedHeaderTransformer != null) { adapted.setForwardedHeaderTransformer(this.forwardedHeaderTransformer); } + if (this.observationRegistry != null) { + adapted.setObservationRegistry(this.observationRegistry); + } + if (this.observationConvention != null) { + adapted.setObservationConvention(this.observationConvention); + } if (this.applicationContext != null) { adapted.setApplicationContext(this.applicationContext); } diff --git a/spring-web/src/main/java/org/springframework/web/server/handler/ExceptionHandlingWebHandler.java b/spring-web/src/main/java/org/springframework/web/server/handler/ExceptionHandlingWebHandler.java index a1e61e1eea63..b8b01951b3d1 100644 --- a/spring-web/src/main/java/org/springframework/web/server/handler/ExceptionHandlingWebHandler.java +++ b/spring-web/src/main/java/org/springframework/web/server/handler/ExceptionHandlingWebHandler.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2020 the original author or authors. + * Copyright 2002-2023 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -38,6 +38,13 @@ */ public class ExceptionHandlingWebHandler extends WebHandlerDecorator { + /** + * Name of the {@link ServerWebExchange#getAttributes() attribute} that + * contains the exception handled by {@link WebExceptionHandler WebExceptionHandlers}. + * @since 6.0.8 + */ + public static String HANDLED_WEB_EXCEPTION = ExceptionHandlingWebHandler.class.getSimpleName() + ".handledException"; + private final List exceptionHandlers; @@ -74,7 +81,8 @@ public Mono handle(ServerWebExchange exchange) { } for (WebExceptionHandler handler : this.exceptionHandlers) { - completion = completion.onErrorResume(ex -> handler.handle(exchange, ex)); + completion = completion.doOnError(error -> exchange.getAttributes().put(HANDLED_WEB_EXCEPTION, error)) + .onErrorResume(ex -> handler.handle(exchange, ex)); } return completion; } diff --git a/spring-web/src/test/java/org/springframework/web/filter/reactive/ServerHttpObservationFilterTests.java b/spring-web/src/test/java/org/springframework/web/filter/reactive/ServerHttpObservationFilterTests.java index 4bef48c1149d..b6a6d98cce0a 100644 --- a/spring-web/src/test/java/org/springframework/web/filter/reactive/ServerHttpObservationFilterTests.java +++ b/spring-web/src/test/java/org/springframework/web/filter/reactive/ServerHttpObservationFilterTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2022 the original author or authors. + * Copyright 2002-2023 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -40,6 +40,7 @@ * * @author Brian Clozel */ +@SuppressWarnings("removal") class ServerHttpObservationFilterTests { private final TestObservationRegistry observationRegistry = TestObservationRegistry.create(); diff --git a/spring-web/src/test/java/org/springframework/web/server/adapter/HttpWebHandlerAdapterObservabilityTests.java b/spring-web/src/test/java/org/springframework/web/server/adapter/HttpWebHandlerAdapterObservabilityTests.java new file mode 100644 index 000000000000..94d8c8b46d2d --- /dev/null +++ b/spring-web/src/test/java/org/springframework/web/server/adapter/HttpWebHandlerAdapterObservabilityTests.java @@ -0,0 +1,158 @@ +/* + * Copyright 2002-2023 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.web.server.adapter; + + +import java.util.List; +import java.util.Optional; + +import io.micrometer.observation.contextpropagation.ObservationThreadLocalAccessor; +import io.micrometer.observation.tck.TestObservationRegistry; +import io.micrometer.observation.tck.TestObservationRegistryAssert; +import org.junit.jupiter.api.Test; +import reactor.core.publisher.Mono; +import reactor.test.StepVerifier; +import reactor.util.context.ContextView; + +import org.springframework.http.HttpStatus; +import org.springframework.http.server.reactive.observation.ServerRequestObservationContext; +import org.springframework.web.server.ServerWebExchange; +import org.springframework.web.server.WebExceptionHandler; +import org.springframework.web.server.WebHandler; +import org.springframework.web.server.handler.ExceptionHandlingWebHandler; +import org.springframework.web.testfixture.http.server.reactive.MockServerHttpRequest; +import org.springframework.web.testfixture.http.server.reactive.MockServerHttpResponse; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Observability-related tests for {@link HttpWebHandlerAdapter}. + * @author Brian Clozel + */ +class HttpWebHandlerAdapterObservabilityTests { + + private final TestObservationRegistry observationRegistry = TestObservationRegistry.create(); + + private final MockServerHttpRequest request = MockServerHttpRequest.post("/test/resource").build(); + + private final MockServerHttpResponse response = new MockServerHttpResponse(); + + @Test + void handlerShouldSetObservationContextOnExchange() { + HttpStatusSuccessStubWebHandler targetHandler = new HttpStatusSuccessStubWebHandler(HttpStatus.OK); + createWebHandler(targetHandler).handle(this.request, this.response).block(); + assertThat(targetHandler.observationContext).isPresent(); + assertThat(targetHandler.observationContext.get().getCarrier()).isEqualTo(this.request); + assertThat(targetHandler.observationContext.get().getResponse()).isEqualTo(this.response); + assertThatHttpObservation().hasLowCardinalityKeyValue("outcome", "SUCCESS"); + } + + @Test + void handlerShouldSetCurrentObservationInReactorContext() { + ReactorContextWebHandler targetHandler = new ReactorContextWebHandler(); + createWebHandler(targetHandler).handle(this.request, this.response).block(); + assertThat(targetHandler.contextView.getOrEmpty(ObservationThreadLocalAccessor.KEY)).isPresent(); + assertThatHttpObservation().hasLowCardinalityKeyValue("outcome", "SUCCESS"); + } + + @Test + void handlerShouldSeeHandledException() { + ThrowingExceptionWebHandler throwingHandler = new ThrowingExceptionWebHandler(new IllegalStateException("testing error")); + ExceptionHandlingWebHandler targetHandler = new ExceptionHandlingWebHandler(throwingHandler, List.of(new BadRequestExceptionHandler())); + createWebHandler(targetHandler).handle(this.request, this.response).block(); + assertThat(throwingHandler.observationContext).isPresent(); + assertThat(throwingHandler.observationContext.get().getError()).isInstanceOf(IllegalStateException.class); + assertThatHttpObservation().hasLowCardinalityKeyValue("outcome", "CLIENT_ERROR"); + } + + @Test + void handlerShouldRecordObservationWhenCancelled() { + HttpStatusSuccessStubWebHandler targetHandler = new HttpStatusSuccessStubWebHandler(HttpStatus.OK); + StepVerifier.create(createWebHandler(targetHandler).handle(this.request, this.response)).thenCancel().verify(); + assertThatHttpObservation().hasLowCardinalityKeyValue("outcome", "UNKNOWN"); + } + + private HttpWebHandlerAdapter createWebHandler(WebHandler targetHandler) { + HttpWebHandlerAdapter handlerAdapter = new HttpWebHandlerAdapter(targetHandler); + handlerAdapter.setObservationRegistry(this.observationRegistry); + return handlerAdapter; + } + + private TestObservationRegistryAssert.TestObservationRegistryAssertReturningObservationContextAssert assertThatHttpObservation() { + return TestObservationRegistryAssert.assertThat(this.observationRegistry) + .hasObservationWithNameEqualTo("http.server.requests").that(); + } + + private static class HttpStatusSuccessStubWebHandler implements WebHandler { + + private final HttpStatus responseStatus; + + private Optional observationContext; + + public HttpStatusSuccessStubWebHandler(HttpStatus responseStatus) { + this.responseStatus = responseStatus; + } + + @Override + public Mono handle(ServerWebExchange exchange) { + this.observationContext = ServerRequestObservationContext.findCurrent(exchange); + exchange.getResponse().setStatusCode(this.responseStatus); + return Mono.empty(); + } + } + + private static class ReactorContextWebHandler implements WebHandler { + + ContextView contextView; + + @Override + public Mono handle(ServerWebExchange exchange) { + exchange.getResponse().setStatusCode(HttpStatus.OK); + return Mono.deferContextual(contextView -> { + this.contextView = contextView; + return Mono.empty(); + }); + } + } + + private static class ThrowingExceptionWebHandler implements WebHandler { + + private final Throwable exception; + + private Optional observationContext; + + private ThrowingExceptionWebHandler(Throwable exception) { + this.exception = exception; + } + + @Override + public Mono handle(ServerWebExchange exchange) { + this.observationContext = ServerRequestObservationContext.findCurrent(exchange); + return Mono.error(this.exception); + } + } + + private static class BadRequestExceptionHandler implements WebExceptionHandler { + + @Override + public Mono handle(ServerWebExchange exchange, Throwable ex) { + exchange.getResponse().setStatusCode(HttpStatus.BAD_REQUEST); + return exchange.getResponse().setComplete(); + } + } + +} diff --git a/spring-web/src/test/java/org/springframework/web/server/handler/ExceptionHandlingWebHandlerTests.java b/spring-web/src/test/java/org/springframework/web/server/handler/ExceptionHandlingWebHandlerTests.java index 4e27848f5f15..412263cd96d2 100644 --- a/spring-web/src/test/java/org/springframework/web/server/handler/ExceptionHandlingWebHandlerTests.java +++ b/spring-web/src/test/java/org/springframework/web/server/handler/ExceptionHandlingWebHandlerTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2019 the original author or authors. + * Copyright 2002-2023 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -45,13 +45,13 @@ public class ExceptionHandlingWebHandlerTests { @Test - public void handleErrorSignal() throws Exception { + void handleErrorSignal() { createWebHandler(new BadRequestExceptionHandler()).handle(this.exchange).block(); assertThat(this.exchange.getResponse().getStatusCode()).isEqualTo(HttpStatus.BAD_REQUEST); } @Test - public void handleErrorSignalWithMultipleHttpErrorHandlers() throws Exception { + void handleErrorSignalWithMultipleHttpErrorHandlers() { createWebHandler( new UnresolvedExceptionHandler(), new UnresolvedExceptionHandler(), @@ -62,17 +62,14 @@ public void handleErrorSignalWithMultipleHttpErrorHandlers() throws Exception { } @Test - public void unresolvedException() throws Exception { + void unresolvedException() { Mono mono = createWebHandler(new UnresolvedExceptionHandler()).handle(this.exchange); StepVerifier.create(mono).expectErrorMessage("boo").verify(); assertThat(this.exchange.getResponse().getStatusCode()).isNull(); } @Test - public void unresolvedExceptionWithWebHttpHandlerAdapter() throws Exception { - - // HttpWebHandlerAdapter handles unresolved errors - + void unresolvedExceptionWithWebHttpHandlerAdapter() { new HttpWebHandlerAdapter(createWebHandler(new UnresolvedExceptionHandler())) .handle(this.exchange.getRequest(), this.exchange.getResponse()).block(); @@ -80,11 +77,18 @@ public void unresolvedExceptionWithWebHttpHandlerAdapter() throws Exception { } @Test - public void thrownExceptionBecomesErrorSignal() throws Exception { + void thrownExceptionBecomesErrorSignal() { createWebHandler(new BadRequestExceptionHandler()).handle(this.exchange).block(); assertThat(this.exchange.getResponse().getStatusCode()).isEqualTo(HttpStatus.BAD_REQUEST); } + @Test + void thrownExceptionIsStoredAsExchangeAttribute() { + createWebHandler(new BadRequestExceptionHandler()).handle(this.exchange).block(); + Exception exceptionAttribute = this.exchange.getAttribute(ExceptionHandlingWebHandler.HANDLED_WEB_EXCEPTION); + assertThat(exceptionAttribute).isInstanceOf(IllegalStateException.class); + } + private WebHandler createWebHandler(WebExceptionHandler... handlers) { return new ExceptionHandlingWebHandler(this.targetHandler, Arrays.asList(handlers)); } diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/function/server/support/RouterFunctionMapping.java b/spring-webflux/src/main/java/org/springframework/web/reactive/function/server/support/RouterFunctionMapping.java index 2dd4a8142d95..1d44e772747f 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/function/server/support/RouterFunctionMapping.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/function/server/support/RouterFunctionMapping.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2022 the original author or authors. + * Copyright 2002-2023 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -26,6 +26,7 @@ import org.springframework.beans.factory.InitializingBean; import org.springframework.http.codec.HttpMessageReader; import org.springframework.http.codec.ServerCodecConfigurer; +import org.springframework.http.server.reactive.observation.ServerRequestObservationContext; import org.springframework.lang.Nullable; import org.springframework.util.CollectionUtils; import org.springframework.web.filter.reactive.ServerHttpObservationFilter; @@ -161,7 +162,7 @@ protected Mono getHandlerInternal(ServerWebExchange exchange) { } } - @SuppressWarnings("unchecked") + @SuppressWarnings({"unchecked", "removal"}) private void setAttributes( Map attributes, ServerRequest serverRequest, HandlerFunction handlerFunction) { @@ -173,6 +174,8 @@ private void setAttributes( attributes.put(BEST_MATCHING_PATTERN_ATTRIBUTE, matchingPattern); ServerHttpObservationFilter.findObservationContext(serverRequest.exchange()) .ifPresent(context -> context.setPathPattern(matchingPattern.toString())); + ServerRequestObservationContext.findCurrent(serverRequest.exchange()) + .ifPresent(context -> context.setPathPattern(matchingPattern.toString())); } Map uriVariables = (Map) attributes.get(RouterFunctions.URI_TEMPLATE_VARIABLES_ATTRIBUTE); diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/handler/AbstractUrlHandlerMapping.java b/spring-webflux/src/main/java/org/springframework/web/reactive/handler/AbstractUrlHandlerMapping.java index a4997b3ab8f7..0d9285640435 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/handler/AbstractUrlHandlerMapping.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/handler/AbstractUrlHandlerMapping.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2022 the original author or authors. + * Copyright 2002-2023 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -27,6 +27,7 @@ import org.springframework.beans.BeansException; import org.springframework.http.server.PathContainer; +import org.springframework.http.server.reactive.observation.ServerRequestObservationContext; import org.springframework.lang.Nullable; import org.springframework.util.Assert; import org.springframework.util.StringUtils; @@ -128,6 +129,7 @@ public Mono getHandlerInternal(ServerWebExchange exchange) { * @see org.springframework.web.util.pattern.PathPattern */ @Nullable + @SuppressWarnings("removal") protected Object lookupHandler(PathContainer lookupPath, ServerWebExchange exchange) throws Exception { List matches = null; for (PathPattern pattern : this.handlerMap.keySet()) { @@ -168,6 +170,8 @@ protected Object lookupHandler(PathContainer lookupPath, ServerWebExchange excha exchange.getAttributes().put(BEST_MATCHING_PATTERN_ATTRIBUTE, pattern); ServerHttpObservationFilter.findObservationContext(exchange) .ifPresent(context -> context.setPathPattern(pattern.toString())); + ServerRequestObservationContext.findCurrent(exchange) + .ifPresent(context -> context.setPathPattern(pattern.toString())); exchange.getAttributes().put(PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE, pathWithinMapping); exchange.getAttributes().put(URI_TEMPLATE_VARIABLES_ATTRIBUTE, matchInfo.getUriVariables()); diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/RequestMappingInfoHandlerMapping.java b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/RequestMappingInfoHandlerMapping.java index 3494b8bb9a82..5e1be8dc622a 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/RequestMappingInfoHandlerMapping.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/RequestMappingInfoHandlerMapping.java @@ -35,6 +35,7 @@ import org.springframework.http.MediaType; import org.springframework.http.server.PathContainer; import org.springframework.http.server.reactive.ServerHttpRequest; +import org.springframework.http.server.reactive.observation.ServerRequestObservationContext; import org.springframework.util.Assert; import org.springframework.util.CollectionUtils; import org.springframework.util.MultiValueMap; @@ -113,6 +114,7 @@ public Mono getHandlerInternal(ServerWebExchange exchange) { * @see HandlerMapping#PRODUCIBLE_MEDIA_TYPES_ATTRIBUTE */ @Override + @SuppressWarnings("removal") protected void handleMatch(RequestMappingInfo info, HandlerMethod handlerMethod, ServerWebExchange exchange) { @@ -143,6 +145,8 @@ protected void handleMatch(RequestMappingInfo info, HandlerMethod handlerMethod, exchange.getAttributes().put(BEST_MATCHING_PATTERN_ATTRIBUTE, bestPattern); ServerHttpObservationFilter.findObservationContext(exchange) .ifPresent(context -> context.setPathPattern(bestPattern.toString())); + ServerRequestObservationContext.findCurrent(exchange) + .ifPresent(context -> context.setPathPattern(bestPattern.toString())); exchange.getAttributes().put(URI_TEMPLATE_VARIABLES_ATTRIBUTE, uriVariables); exchange.getAttributes().put(MATRIX_VARIABLES_ATTRIBUTE, matrixVariables); diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/function/server/support/RouterFunctionMappingTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/function/server/support/RouterFunctionMappingTests.java index c8615618357e..f27974eb58dd 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/function/server/support/RouterFunctionMappingTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/function/server/support/RouterFunctionMappingTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2022 the original author or authors. + * Copyright 2002-2023 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -36,7 +36,6 @@ import org.springframework.web.util.pattern.PathPattern; import static org.assertj.core.api.Assertions.assertThat; -import static org.springframework.web.filter.reactive.ServerHttpObservationFilter.CURRENT_OBSERVATION_CONTEXT_ATTRIBUTE; /** * Tests for {@link RouterFunctionMapping}. @@ -117,6 +116,7 @@ void changeParser() throws Exception { } @Test + @SuppressWarnings("removal") void mappedRequestShouldHoldAttributes() { HandlerFunction handlerFunction = request -> ServerResponse.ok().build(); RouterFunction routerFunction = RouterFunctions.route() @@ -138,6 +138,8 @@ void mappedRequestShouldHoldAttributes() { assertThat(matchingPattern.getPatternString()).isEqualTo("/match"); assertThat(ServerHttpObservationFilter.findObservationContext(exchange)) .hasValueSatisfying(context -> assertThat(context.getPathPattern()).isEqualTo(matchingPattern.getPatternString())); + assertThat(ServerRequestObservationContext.findCurrent(exchange)) + .hasValueSatisfying(context -> assertThat(context.getPathPattern()).isEqualTo(matchingPattern.getPatternString())); ServerRequest serverRequest = exchange.getAttribute(RouterFunctions.REQUEST_ATTRIBUTE); assertThat(serverRequest).isNotNull(); @@ -146,10 +148,12 @@ void mappedRequestShouldHoldAttributes() { assertThat(handler).isEqualTo(handlerFunction); } + @SuppressWarnings("removal") private ServerWebExchange createExchange(String urlTemplate) { MockServerWebExchange exchange = MockServerWebExchange.from(MockServerHttpRequest.get(urlTemplate)); ServerRequestObservationContext observationContext = new ServerRequestObservationContext(exchange.getRequest(), exchange.getResponse(), exchange.getAttributes()); - exchange.getAttributes().put(CURRENT_OBSERVATION_CONTEXT_ATTRIBUTE, observationContext); + exchange.getAttributes().put(ServerHttpObservationFilter.CURRENT_OBSERVATION_CONTEXT_ATTRIBUTE, observationContext); + exchange.getAttributes().put(ServerRequestObservationContext.CURRENT_OBSERVATION_CONTEXT_ATTRIBUTE, observationContext); return exchange; } diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/RequestMappingInfoHandlerMappingTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/RequestMappingInfoHandlerMappingTests.java index bcdeefc7c351..b4d739408991 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/RequestMappingInfoHandlerMappingTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/RequestMappingInfoHandlerMappingTests.java @@ -65,7 +65,6 @@ import static org.springframework.web.bind.annotation.RequestMethod.GET; import static org.springframework.web.bind.annotation.RequestMethod.HEAD; import static org.springframework.web.bind.annotation.RequestMethod.OPTIONS; -import static org.springframework.web.filter.reactive.ServerHttpObservationFilter.CURRENT_OBSERVATION_CONTEXT_ATTRIBUTE; import static org.springframework.web.reactive.HandlerMapping.BEST_MATCHING_HANDLER_ATTRIBUTE; import static org.springframework.web.reactive.HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE; import static org.springframework.web.reactive.result.method.RequestMappingInfo.paths; @@ -261,11 +260,12 @@ public void handleMatchBestMatchingPatternAttribute() { } @Test + @SuppressWarnings("removal") public void handleMatchBestMatchingPatternAttributeInObservationContext() { RequestMappingInfo key = paths("/{path1}/2", "/**").build(); ServerWebExchange exchange = MockServerWebExchange.from(get("/1/2")); ServerRequestObservationContext observationContext = new ServerRequestObservationContext(exchange.getRequest(), exchange.getResponse(), exchange.getAttributes()); - exchange.getAttributes().put(CURRENT_OBSERVATION_CONTEXT_ATTRIBUTE, observationContext); + exchange.getAttributes().put(ServerRequestObservationContext.CURRENT_OBSERVATION_CONTEXT_ATTRIBUTE, observationContext); this.handlerMapping.handleMatch(key, handlerMethod, exchange); assertThat(observationContext.getPathPattern()).isEqualTo("/{path1}/2");