From 77be10e7bcdf7c934295c9aba539067cc1fd0fbb Mon Sep 17 00:00:00 2001 From: Brian Clozel Date: Thu, 12 Apr 2018 11:30:43 +0200 Subject: [PATCH] Fix "status" metrics tag for error responses Prior to this commit, the metrics `WebFilter` would handle exceptions flowing through the pipeline and extract tag information right away. Since error handling turns the exception information into error HTTP responses later in the chain, the information extracted from the response earlier is invalid. In this case, the "status" information could be "200" whereas error handlers would later set that status to "500". This commit delays the tags extraction later in the process, right before the response is comitted. The happy path is not changed, as handlers signal that the response is fully taken care of at that point. Fixes gh-11514 --- .../web/reactive/server/MetricsWebFilter.java | 12 ++- .../server/MetricsWebFilterTests.java | 92 +++++++++++++++++++ 2 files changed, 100 insertions(+), 4 deletions(-) create mode 100644 spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/reactive/server/MetricsWebFilterTests.java diff --git a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/reactive/server/MetricsWebFilter.java b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/reactive/server/MetricsWebFilter.java index fad2e703477..8250b09a072 100644 --- a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/reactive/server/MetricsWebFilter.java +++ b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/reactive/server/MetricsWebFilter.java @@ -25,15 +25,16 @@ import reactor.core.publisher.Mono; import org.springframework.core.Ordered; import org.springframework.core.annotation.Order; +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; /** - * Intercepts incoming HTTP requests modeled with the WebFlux annotation-based programming - * model. + * Intercepts incoming HTTP requests handled by Spring WebFlux handlers. * * @author Jon Schneider + * @author Brian Clozel * @since 2.0.0 */ @Order(Ordered.HIGHEST_PRECEDENCE + 1) @@ -59,8 +60,10 @@ public class MetricsWebFilter implements WebFilter { private Publisher filter(ServerWebExchange exchange, Mono call) { long start = System.nanoTime(); + ServerHttpResponse response = exchange.getResponse(); return call.doOnSuccess((done) -> success(exchange, start)) - .doOnError((cause) -> error(exchange, start, cause)); + .doOnError((cause) -> response + .beforeCommit(() -> error(exchange, start, cause))); } private void success(ServerWebExchange exchange, long start) { @@ -69,10 +72,11 @@ public class MetricsWebFilter implements WebFilter { TimeUnit.NANOSECONDS); } - private void error(ServerWebExchange exchange, long start, Throwable cause) { + private Mono error(ServerWebExchange exchange, long start, Throwable cause) { Iterable tags = this.tagsProvider.httpRequestTags(exchange, cause); this.registry.timer(this.metricName, tags).record(System.nanoTime() - start, TimeUnit.NANOSECONDS); + return Mono.empty(); } } diff --git a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/reactive/server/MetricsWebFilterTests.java b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/reactive/server/MetricsWebFilterTests.java new file mode 100644 index 00000000000..82cf4e6afd4 --- /dev/null +++ b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/reactive/server/MetricsWebFilterTests.java @@ -0,0 +1,92 @@ +/* + * Copyright 2012-2018 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 + * + * http://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.boot.actuate.metrics.web.reactive.server; + +import io.micrometer.core.instrument.MockClock; +import io.micrometer.core.instrument.simple.SimpleConfig; +import io.micrometer.core.instrument.simple.SimpleMeterRegistry; +import org.junit.Before; +import org.junit.Test; +import reactor.core.publisher.Mono; + +import org.springframework.mock.http.server.reactive.MockServerHttpRequest; +import org.springframework.mock.web.server.MockServerWebExchange; +import org.springframework.web.reactive.HandlerMapping; +import org.springframework.web.util.pattern.PathPatternParser; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Tests for {@link MetricsWebFilter} + * @author Brian Clozel + */ +public class MetricsWebFilterTests { + + private static final String REQUEST_METRICS_NAME = "http.server.requests"; + + private SimpleMeterRegistry registry; + + private MetricsWebFilter webFilter; + + @Before + public void setup() { + MockClock clock = new MockClock(); + this.registry = new SimpleMeterRegistry(SimpleConfig.DEFAULT, clock); + this.webFilter = new MetricsWebFilter(this.registry, + new DefaultWebFluxTagsProvider(), REQUEST_METRICS_NAME); + } + + @Test + public void filterAddsTagsToRegistry() { + MockServerWebExchange exchange = createExchange("/projects/spring-boot", + "/projects/{project}"); + this.webFilter.filter(exchange, + serverWebExchange -> exchange.getResponse().setComplete()).block(); + assertMetricsContainsTag("uri", "/projects/{project}"); + assertMetricsContainsTag("status", "200"); + } + + @Test + public void filterAddsTagsToRegistryForExceptions() { + MockServerWebExchange exchange = createExchange("/projects/spring-boot", + "/projects/{project}"); + this.webFilter.filter(exchange, + serverWebExchange -> Mono.error(new IllegalStateException("test error"))) + .onErrorResume(t -> { + exchange.getResponse().setStatusCodeValue(500); + return exchange.getResponse().setComplete(); + }).block(); + assertMetricsContainsTag("uri", "/projects/{project}"); + assertMetricsContainsTag("status", "500"); + } + + private MockServerWebExchange createExchange(String path, String pathPattern) { + PathPatternParser parser = new PathPatternParser(); + MockServerWebExchange exchange = MockServerWebExchange + .from(MockServerHttpRequest.get(path).build()); + exchange.getAttributes() + .put(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE, parser.parse(pathPattern)); + return exchange; + } + + private void assertMetricsContainsTag(String tagKey, String tagValue) { + assertThat(this.registry.get(REQUEST_METRICS_NAME) + .tag(tagKey, tagValue).timer().count()) + .isEqualTo(1); + } + +}