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

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<Void> filter(ServerWebExchange exchange, Mono<Void> 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<Void>  error(ServerWebExchange exchange, long start, Throwable cause) {
 		Iterable<Tag> tags = this.tagsProvider.httpRequestTags(exchange, cause);
 		this.registry.timer(this.metricName, tags).record(System.nanoTime() - start,
 				TimeUnit.NANOSECONDS);
+		return Mono.empty();
 	}
 
 }

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);
+	}
+
+}