Polish 'Support @Timed annotation for WebFlux'

Extract common method logic to a `HandlerMethodTimedAnnotations`
utility class and also add some caching. Also move the test code
into a single class.

See gh-23112

spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/method/HandlerMethodTimedAnnotations.java

@@ -0,0 +1,64 @@
+/*
+ * Copyright 2012-2021 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.boot.actuate.metrics.web.method;
+
+import java.lang.reflect.AnnotatedElement;
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+
+import io.micrometer.core.annotation.Timed;
+
+import org.springframework.core.annotation.MergedAnnotationCollectors;
+import org.springframework.core.annotation.MergedAnnotations;
+import org.springframework.util.ConcurrentReferenceHashMap;
+import org.springframework.web.method.HandlerMethod;
+
+/**
+ * Utility used to obtain {@link Timed @Timed} annotations from a {@link HandlerMethod}.
+ *
+ * @author Phillip Webb
+ * @since 2.5.0
+ */
+public final class HandlerMethodTimedAnnotations {
+
+	private static Map<AnnotatedElement, Set<Timed>> cache = new ConcurrentReferenceHashMap<>();
+
+	private HandlerMethodTimedAnnotations() {
+	}
+
+	public static Set<Timed> get(HandlerMethod handler) {
+		Set<Timed> methodAnnotations = findTimedAnnotations(handler.getMethod());
+		if (!methodAnnotations.isEmpty()) {
+			return methodAnnotations;
+		}
+		return findTimedAnnotations(handler.getBeanType());
+	}
+
+	private static Set<Timed> findTimedAnnotations(AnnotatedElement element) {
+		Set<Timed> result = cache.get(element);
+		if (result != null) {
+			return result;
+		}
+		MergedAnnotations annotations = MergedAnnotations.from(element);
+		result = (!annotations.isPresent(Timed.class)) ? Collections.emptySet()
+				: annotations.stream(Timed.class).collect(MergedAnnotationCollectors.toAnnotationSet());
+		cache.put(element, result);
+		return result;
+	}
+
+}

spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/reactive/server/MetricsWebFilter.java

@@ -16,7 +16,6 @@
 
 package org.springframework.boot.actuate.metrics.web.reactive.server;
 
-import java.lang.reflect.AnnotatedElement;
 import java.util.Collections;
 import java.util.Set;
 import java.util.concurrent.TimeUnit;
@@ -25,14 +24,14 @@ import io.micrometer.core.annotation.Timed;
 import io.micrometer.core.instrument.MeterRegistry;
 import io.micrometer.core.instrument.Tag;
 import io.micrometer.core.instrument.Timer;
+import io.micrometer.core.instrument.Timer.Builder;
 import org.reactivestreams.Publisher;
 import reactor.core.publisher.Mono;
 
 import org.springframework.boot.actuate.metrics.AutoTimer;
+import org.springframework.boot.actuate.metrics.web.method.HandlerMethodTimedAnnotations;
 import org.springframework.boot.web.reactive.error.ErrorAttributes;
 import org.springframework.core.Ordered;
-import org.springframework.core.annotation.MergedAnnotationCollectors;
-import org.springframework.core.annotation.MergedAnnotations;
 import org.springframework.core.annotation.Order;
 import org.springframework.http.server.reactive.ServerHttpResponse;
 import org.springframework.web.method.HandlerMethod;
@@ -100,48 +99,24 @@ public class MetricsWebFilter implements WebFilter {
 	}
 
 	private void record(ServerWebExchange exchange, Throwable cause, long start) {
-		if (cause == null) {
-			cause = exchange.getAttribute(ErrorAttributes.ERROR_ATTRIBUTE);
-		}
+		cause = (cause != null) ? cause : exchange.getAttribute(ErrorAttributes.ERROR_ATTRIBUTE);
 		Object handler = exchange.getAttribute(HandlerMapping.BEST_MATCHING_HANDLER_ATTRIBUTE);
-		Set<Timed> annotations = getTimedAnnotations(handler);
+		Set<Timed> annotations = (handler instanceof HandlerMethod)
+				? HandlerMethodTimedAnnotations.get((HandlerMethod) handler) : Collections.emptySet();
 		Iterable<Tag> tags = this.tagsProvider.httpRequestTags(exchange, cause);
 		long duration = System.nanoTime() - start;
 		if (annotations.isEmpty()) {
 			if (this.autoTimer.isEnabled()) {
-				this.autoTimer.builder(this.metricName).tags(tags).register(this.registry).record(duration,
-						TimeUnit.NANOSECONDS);
+				Builder builder = this.autoTimer.builder(this.metricName);
+				builder.tags(tags).register(this.registry).record(duration, TimeUnit.NANOSECONDS);
 			}
 		}
 		else {
 			for (Timed annotation : annotations) {
-				Timer.builder(annotation, this.metricName).tags(tags).register(this.registry).record(duration,
-						TimeUnit.NANOSECONDS);
+				Builder builder = Timer.builder(annotation, this.metricName);
+				builder.tags(tags).register(this.registry).record(duration, TimeUnit.NANOSECONDS);
 			}
 		}
 	}
 
-	private Set<Timed> getTimedAnnotations(Object handler) {
-		if (!(handler instanceof HandlerMethod)) {
-			return Collections.emptySet();
-		}
-		return getTimedAnnotations((HandlerMethod) handler);
-	}
-
-	private Set<Timed> getTimedAnnotations(HandlerMethod handler) {
-		Set<Timed> methodAnnotations = findTimedAnnotations(handler.getMethod());
-		if (!methodAnnotations.isEmpty()) {
-			return methodAnnotations;
-		}
-		return findTimedAnnotations(handler.getBeanType());
-	}
-
-	private Set<Timed> findTimedAnnotations(AnnotatedElement element) {
-		MergedAnnotations annotations = MergedAnnotations.from(element);
-		if (!annotations.isPresent(Timed.class)) {
-			return Collections.emptySet();
-		}
-		return annotations.stream(Timed.class).collect(MergedAnnotationCollectors.toAnnotationSet());
-	}
-
 }

spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/servlet/WebMvcMetricsFilter.java

@@ -17,7 +17,6 @@
 package org.springframework.boot.actuate.metrics.web.servlet;
 
 import java.io.IOException;
-import java.lang.reflect.AnnotatedElement;
 import java.util.Collections;
 import java.util.Set;
 
@@ -33,9 +32,8 @@ import io.micrometer.core.instrument.Timer.Builder;
 import io.micrometer.core.instrument.Timer.Sample;
 
 import org.springframework.boot.actuate.metrics.AutoTimer;
+import org.springframework.boot.actuate.metrics.web.method.HandlerMethodTimedAnnotations;
 import org.springframework.boot.web.servlet.error.ErrorAttributes;
-import org.springframework.core.annotation.MergedAnnotationCollectors;
-import org.springframework.core.annotation.MergedAnnotations;
 import org.springframework.http.HttpStatus;
 import org.springframework.web.filter.OncePerRequestFilter;
 import org.springframework.web.method.HandlerMethod;
@@ -130,7 +128,8 @@ public class WebMvcMetricsFilter extends OncePerRequestFilter {
 	private void record(TimingContext timingContext, HttpServletRequest request, HttpServletResponse response,
 			Throwable exception) {
 		Object handler = getHandler(request);
-		Set<Timed> annotations = getTimedAnnotations(handler);
+		Set<Timed> annotations = (handler instanceof HandlerMethod)
+				? HandlerMethodTimedAnnotations.get((HandlerMethod) handler) : Collections.emptySet();
 		Timer.Sample timerSample = timingContext.getTimerSample();
 		if (annotations.isEmpty()) {
 			if (this.autoTimer.isEnabled()) {
@@ -150,29 +149,6 @@ public class WebMvcMetricsFilter extends OncePerRequestFilter {
 		return request.getAttribute(HandlerMapping.BEST_MATCHING_HANDLER_ATTRIBUTE);
 	}
 
-	private Set<Timed> getTimedAnnotations(Object handler) {
-		if (!(handler instanceof HandlerMethod)) {
-			return Collections.emptySet();
-		}
-		return getTimedAnnotations((HandlerMethod) handler);
-	}
-
-	private Set<Timed> getTimedAnnotations(HandlerMethod handler) {
-		Set<Timed> methodAnnotations = findTimedAnnotations(handler.getMethod());
-		if (!methodAnnotations.isEmpty()) {
-			return methodAnnotations;
-		}
-		return findTimedAnnotations(handler.getBeanType());
-	}
-
-	private Set<Timed> findTimedAnnotations(AnnotatedElement element) {
-		MergedAnnotations annotations = MergedAnnotations.from(element);
-		if (!annotations.isPresent(Timed.class)) {
-			return Collections.emptySet();
-		}
-		return annotations.stream(Timed.class).collect(MergedAnnotationCollectors.toAnnotationSet());
-	}
-
 	private Timer getTimer(Builder builder, Object handler, HttpServletRequest request, HttpServletResponse response,
 			Throwable exception) {
 		return builder.tags(this.tagsProvider.getTags(request, response, handler, exception)).register(this.registry);

spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/method/HandlerMethodTimedAnnotationsTests.java

@@ -0,0 +1,87 @@
+/*
+ * Copyright 2012-2021 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.boot.actuate.metrics.web.method;
+
+import java.lang.reflect.Method;
+import java.util.Set;
+
+import io.micrometer.core.annotation.Timed;
+import org.junit.jupiter.api.Test;
+
+import org.springframework.util.ReflectionUtils;
+import org.springframework.web.method.HandlerMethod;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+/**
+ * Tests for {@link HandlerMethodTimedAnnotations}.
+ *
+ * @author Phillip Webb
+ */
+class HandlerMethodTimedAnnotationsTests {
+
+	@Test
+	void getWhenNoneReturnsEmptySet() {
+		Object bean = new None();
+		Method method = ReflectionUtils.findMethod(bean.getClass(), "handle");
+		Set<Timed> annotations = HandlerMethodTimedAnnotations.get(new HandlerMethod(bean, method));
+		assertThat(annotations).isEmpty();
+	}
+
+	@Test
+	void getWhenOnMethodReturnsMethodAnnotations() {
+		Object bean = new OnMethod();
+		Method method = ReflectionUtils.findMethod(bean.getClass(), "handle");
+		Set<Timed> annotations = HandlerMethodTimedAnnotations.get(new HandlerMethod(bean, method));
+		assertThat(annotations).extracting(Timed::value).containsOnly("y", "z");
+	}
+
+	@Test
+	void getWhenNonOnMethodReturnsBeanAnnotations() {
+		Object bean = new OnBean();
+		Method method = ReflectionUtils.findMethod(bean.getClass(), "handle");
+		Set<Timed> annotations = HandlerMethodTimedAnnotations.get(new HandlerMethod(bean, method));
+		assertThat(annotations).extracting(Timed::value).containsOnly("y", "z");
+	}
+
+	static class None {
+
+		void handle() {
+		}
+
+	}
+
+	@Timed("x")
+	static class OnMethod {
+
+		@Timed("y")
+		@Timed("z")
+		void handle() {
+		}
+
+	}
+
+	@Timed("y")
+	@Timed("z")
+	static class OnBean {
+
+		void handle() {
+		}
+
+	}
+
+}

spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/reactive/server/MetricsWebFilterTests.java

@@ -19,6 +19,7 @@ package org.springframework.boot.actuate.metrics.web.reactive.server;
 import java.io.EOFException;
 import java.time.Duration;
 
+import io.micrometer.core.annotation.Timed;
 import io.micrometer.core.instrument.MockClock;
 import io.micrometer.core.instrument.simple.SimpleConfig;
 import io.micrometer.core.instrument.simple.SimpleMeterRegistry;
@@ -31,6 +32,8 @@ import org.springframework.boot.actuate.metrics.AutoTimer;
 import org.springframework.boot.web.reactive.error.ErrorAttributes;
 import org.springframework.mock.http.server.reactive.MockServerHttpRequest;
 import org.springframework.mock.web.server.MockServerWebExchange;
+import org.springframework.util.ReflectionUtils;
+import org.springframework.web.method.HandlerMethod;
 import org.springframework.web.reactive.HandlerMapping;
 import org.springframework.web.util.pattern.PathPatternParser;
 
@@ -46,6 +49,8 @@ class MetricsWebFilterTests {
 
 	private static final String REQUEST_METRICS_NAME = "http.server.requests";
 
+	private static final String REQUEST_METRICS_NAME_PERCENTILE = REQUEST_METRICS_NAME + ".percentile";
+
 	private SimpleMeterRegistry registry;
 
 	private MetricsWebFilter webFilter;
@@ -157,6 +162,59 @@ class MetricsWebFilterTests {
 		assertMetricsContainsTag("outcome", "UNKNOWN");
 	}
 
+	@Test
+	void filterAddsStandardTags() {
+		MockServerWebExchange exchange = createTimedHandlerMethodExchange("timed");
+		this.webFilter.filter(exchange, (serverWebExchange) -> exchange.getResponse().setComplete())
+				.block(Duration.ofSeconds(30));
+		assertMetricsContainsTag("uri", "/projects/{project}");
+		assertMetricsContainsTag("status", "200");
+	}
+
+	@Test
+	void filterAddsExtraTags() {
+		MockServerWebExchange exchange = createTimedHandlerMethodExchange("timedExtraTags");
+		this.webFilter.filter(exchange, (serverWebExchange) -> exchange.getResponse().setComplete())
+				.block(Duration.ofSeconds(30));
+		assertMetricsContainsTag("uri", "/projects/{project}");
+		assertMetricsContainsTag("status", "200");
+		assertMetricsContainsTag("tag1", "value1");
+		assertMetricsContainsTag("tag2", "value2");
+	}
+
+	@Test
+	void filterAddsExtraTagsAndException() {
+		MockServerWebExchange exchange = createTimedHandlerMethodExchange("timedExtraTags");
+		this.webFilter.filter(exchange, (serverWebExchange) -> Mono.error(new IllegalStateException("test error")))
+				.onErrorResume((ex) -> {
+					exchange.getResponse().setRawStatusCode(500);
+					return exchange.getResponse().setComplete();
+				}).block(Duration.ofSeconds(30));
+		assertMetricsContainsTag("uri", "/projects/{project}");
+		assertMetricsContainsTag("status", "500");
+		assertMetricsContainsTag("exception", "IllegalStateException");
+		assertMetricsContainsTag("tag1", "value1");
+		assertMetricsContainsTag("tag2", "value2");
+	}
+
+	@Test
+	void filterAddsPercentileMeters() {
+		MockServerWebExchange exchange = createTimedHandlerMethodExchange("timedPercentiles");
+		this.webFilter.filter(exchange, (serverWebExchange) -> exchange.getResponse().setComplete())
+				.block(Duration.ofSeconds(30));
+		assertMetricsContainsTag("uri", "/projects/{project}");
+		assertMetricsContainsTag("status", "200");
+		assertThat(this.registry.get(REQUEST_METRICS_NAME_PERCENTILE).tag("phi", "0.95").gauge().value()).isNotZero();
+		assertThat(this.registry.get(REQUEST_METRICS_NAME_PERCENTILE).tag("phi", "0.5").gauge().value()).isNotZero();
+	}
+
+	private MockServerWebExchange createTimedHandlerMethodExchange(String methodName) {
+		MockServerWebExchange exchange = createExchange("/projects/spring-boot", "/projects/{project}");
+		exchange.getAttributes().put(HandlerMapping.BEST_MATCHING_HANDLER_ATTRIBUTE,
+				new HandlerMethod(this, ReflectionUtils.findMethod(Handlers.class, methodName)));
+		return exchange;
+	}
+
 	private MockServerWebExchange createExchange(String path, String pathPattern) {
 		PathPatternParser parser = new PathPatternParser();
 		MockServerWebExchange exchange = MockServerWebExchange.from(MockServerHttpRequest.get(path).build());
@@ -168,4 +226,23 @@ class MetricsWebFilterTests {
 		assertThat(this.registry.get(REQUEST_METRICS_NAME).tag(tagKey, tagValue).timer().count()).isEqualTo(1);
 	}
 
+	static class Handlers {
+
+		@Timed
+		Mono<String> timed() {
+			return Mono.just("test");
+		}
+
+		@Timed(extraTags = { "tag1", "value1", "tag2", "value2" })
+		Mono<String> timedExtraTags() {
+			return Mono.just("test");
+		}
+
+		@Timed(percentiles = { 0.5, 0.95 })
+		Mono<String> timedPercentiles() {
+			return Mono.just("test");
+		}
+
+	}
+
 }

spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/reactive/server/MetricsWebFilterTimedAnnotationTests.java

@@ -1,138 +0,0 @@
-/*
- * Copyright 2012-2020 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.boot.actuate.metrics.web.reactive.server;
-
-import java.time.Duration;
-
-import io.micrometer.core.annotation.Timed;
-import io.micrometer.core.instrument.MockClock;
-import io.micrometer.core.instrument.simple.SimpleConfig;
-import io.micrometer.core.instrument.simple.SimpleMeterRegistry;
-import org.junit.jupiter.api.BeforeEach;
-import org.junit.jupiter.api.Test;
-import reactor.core.publisher.Mono;
-
-import org.springframework.boot.actuate.metrics.AutoTimer;
-import org.springframework.mock.http.server.reactive.MockServerHttpRequest;
-import org.springframework.mock.web.server.MockServerWebExchange;
-import org.springframework.util.ReflectionUtils;
-import org.springframework.web.method.HandlerMethod;
-import org.springframework.web.reactive.HandlerMapping;
-import org.springframework.web.util.pattern.PathPatternParser;
-
-import static org.assertj.core.api.Assertions.assertThat;
-
-/**
- * Unit tests for {@link MetricsWebFilter} for handlers with {@link Timed} annotation.
- *
- * @author Andrey Shlykov
- */
-class MetricsWebFilterTimedAnnotationTests {
-
-	private static final String REQUEST_METRICS_NAME = "http.server.requests";
-
-	private static final String REQUEST_METRICS_NAME_PERCENTILE = REQUEST_METRICS_NAME + ".percentile";
-
-	private SimpleMeterRegistry registry;
-
-	private MetricsWebFilter webFilter;
-
-	@BeforeEach
-	void setup() {
-		MockClock clock = new MockClock();
-		this.registry = new SimpleMeterRegistry(SimpleConfig.DEFAULT, clock);
-		this.webFilter = new MetricsWebFilter(this.registry, new DefaultWebFluxTagsProvider(true), REQUEST_METRICS_NAME,
-				AutoTimer.ENABLED);
-	}
-
-	@Test
-	void filterAddsStandardTags() {
-		MockServerWebExchange exchange = createExchange("timedHandler");
-		this.webFilter.filter(exchange, (serverWebExchange) -> exchange.getResponse().setComplete())
-				.block(Duration.ofSeconds(30));
-		assertMetricsContainsTag("uri", "/projects/{project}");
-		assertMetricsContainsTag("status", "200");
-	}
-
-	@Test
-	void filterAddsExtraTags() {
-		MockServerWebExchange exchange = createExchange("timedExtraTagsHandler");
-		this.webFilter.filter(exchange, (serverWebExchange) -> exchange.getResponse().setComplete())
-				.block(Duration.ofSeconds(30));
-		assertMetricsContainsTag("uri", "/projects/{project}");
-		assertMetricsContainsTag("status", "200");
-		assertMetricsContainsTag("tag1", "value1");
-		assertMetricsContainsTag("tag2", "value2");
-	}
-
-	@Test
-	void filterAddsExtraTagsAndException() {
-		MockServerWebExchange exchange = createExchange("timedExtraTagsHandler");
-		this.webFilter.filter(exchange, (serverWebExchange) -> Mono.error(new IllegalStateException("test error")))
-				.onErrorResume((t) -> {
-					exchange.getResponse().setRawStatusCode(500);
-					return exchange.getResponse().setComplete();
-				}).block(Duration.ofSeconds(30));
-		assertMetricsContainsTag("uri", "/projects/{project}");
-		assertMetricsContainsTag("status", "500");
-		assertMetricsContainsTag("exception", "IllegalStateException");
-		assertMetricsContainsTag("tag1", "value1");
-		assertMetricsContainsTag("tag2", "value2");
-	}
-
-	@Test
-	void filterAddsPercentileMeters() {
-		MockServerWebExchange exchange = createExchange("timedPercentilesHandler");
-		this.webFilter.filter(exchange, (serverWebExchange) -> exchange.getResponse().setComplete())
-				.block(Duration.ofSeconds(30));
-		assertMetricsContainsTag("uri", "/projects/{project}");
-		assertMetricsContainsTag("status", "200");
-		assertThat(this.registry.get(REQUEST_METRICS_NAME_PERCENTILE).tag("phi", "0.95").gauge().value()).isNotZero();
-		assertThat(this.registry.get(REQUEST_METRICS_NAME_PERCENTILE).tag("phi", "0.5").gauge().value()).isNotZero();
-	}
-
-	private MockServerWebExchange createExchange(String handlerName) {
-		PathPatternParser parser = new PathPatternParser();
-		HandlerMethod handlerMethod = new HandlerMethod(this, ReflectionUtils.findMethod(this.getClass(), handlerName));
-		MockServerWebExchange exchange = MockServerWebExchange
-				.from(MockServerHttpRequest.get("/projects/spring-boot").build());
-		exchange.getAttributes().put(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE,
-				parser.parse("/projects/{project}"));
-		exchange.getAttributes().put(HandlerMapping.BEST_MATCHING_HANDLER_ATTRIBUTE, handlerMethod);
-		return exchange;
-	}
-
-	private void assertMetricsContainsTag(String tagKey, String tagValue) {
-		assertThat(this.registry.get(REQUEST_METRICS_NAME).tag(tagKey, tagValue).timer().count()).isEqualTo(1);
-	}
-
-	@Timed
-	Mono<String> timedHandler() {
-		return Mono.just("test");
-	}
-
-	@Timed(extraTags = { "tag1", "value1", "tag2", "value2" })
-	Mono<String> timedExtraTagsHandler() {
-		return Mono.just("test");
-	}
-
-	@Timed(percentiles = { 0.5, 0.95 })
-	Mono<String> timedPercentilesHandler() {
-		return Mono.just("test");
-	}
-
-}