From fe078c8afc693c7817f046d0fdd57a6ed268fcd4 Mon Sep 17 00:00:00 2001 From: Andy Wilkinson Date: Wed, 16 Jun 2021 08:23:59 +0100 Subject: [PATCH] Avoid capturing URI template when interceptor won't use it Previously, the URI template handler installed by the client metrics interceptor would always capture the URI template and push it onto the deque, irrespective of whether auto timing was enabled. When auto-timing is disabled the deque is never polled so this led to its unrestricted growth. This commit updates the URI template handler so that a URI template is only pushed onto the deque when the auto timing configuration enables the interceptor. Fixes gh-26915 --- .../MetricsClientHttpRequestInterceptor.java | 16 ++++++--- .../MetricsRestTemplateCustomizerTests.java | 35 +++++++++++++++++++ 2 files changed, 47 insertions(+), 4 deletions(-) diff --git a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/client/MetricsClientHttpRequestInterceptor.java b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/client/MetricsClientHttpRequestInterceptor.java index a5e4a540dbfe..8da5d69a4847 100644 --- a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/client/MetricsClientHttpRequestInterceptor.java +++ b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/client/MetricsClientHttpRequestInterceptor.java @@ -77,7 +77,7 @@ class MetricsClientHttpRequestInterceptor implements ClientHttpRequestIntercepto @Override public ClientHttpResponse intercept(HttpRequest request, byte[] body, ClientHttpRequestExecution execution) throws IOException { - if (!this.autoTimer.isEnabled()) { + if (!enabled()) { return execution.execute(request, body); } long startTime = System.nanoTime(); @@ -100,6 +100,10 @@ public ClientHttpResponse intercept(HttpRequest request, byte[] body, ClientHttp } } + private boolean enabled() { + return this.autoTimer.isEnabled(); + } + UriTemplateHandler createUriTemplateHandler(UriTemplateHandler delegate) { if (delegate instanceof RootUriTemplateHandler) { return ((RootUriTemplateHandler) delegate).withHandlerWrapper(CapturingUriTemplateHandler::new); @@ -113,7 +117,7 @@ private Timer.Builder getTimeBuilder(HttpRequest request, ClientHttpResponse res .description("Timer of RestTemplate operation"); } - private static final class CapturingUriTemplateHandler implements UriTemplateHandler { + private final class CapturingUriTemplateHandler implements UriTemplateHandler { private final UriTemplateHandler delegate; @@ -123,13 +127,17 @@ private CapturingUriTemplateHandler(UriTemplateHandler delegate) { @Override public URI expand(String url, Map arguments) { - urlTemplate.get().push(url); + if (enabled()) { + urlTemplate.get().push(url); + } return this.delegate.expand(url, arguments); } @Override public URI expand(String url, Object... arguments) { - urlTemplate.get().push(url); + if (enabled()) { + urlTemplate.get().push(url); + } return this.delegate.expand(url, arguments); } diff --git a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/client/MetricsRestTemplateCustomizerTests.java b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/client/MetricsRestTemplateCustomizerTests.java index 693dfa2a4f7b..00c88bbe8a43 100644 --- a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/client/MetricsRestTemplateCustomizerTests.java +++ b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/client/MetricsRestTemplateCustomizerTests.java @@ -19,10 +19,12 @@ import java.io.IOException; import java.net.URI; import java.net.URISyntaxException; +import java.util.concurrent.atomic.AtomicBoolean; import io.micrometer.core.instrument.MeterRegistry; import io.micrometer.core.instrument.MockClock; import io.micrometer.core.instrument.Tag; +import io.micrometer.core.instrument.Timer.Builder; import io.micrometer.core.instrument.simple.SimpleConfig; import io.micrometer.core.instrument.simple.SimpleMeterRegistry; import org.assertj.core.api.InstanceOfAssertFactories; @@ -153,6 +155,39 @@ void whenCustomizerAndLocalHostUriTemplateHandlerAreUsedTogetherThenRestTemplate .extracting(RootUriTemplateHandler::getRootUri).isEqualTo("https://localhost:8443"); } + @Test + void whenAutoTimingIsDisabledUriTemplateHandlerDoesNotCaptureUris() { + AtomicBoolean enabled = new AtomicBoolean(false); + AutoTimer autoTimer = new AutoTimer() { + + @Override + public boolean isEnabled() { + return enabled.get(); + } + + @Override + public void apply(Builder builder) { + } + + }; + RestTemplate restTemplate = new RestTemplateBuilder(new MetricsRestTemplateCustomizer(this.registry, + new DefaultRestTemplateExchangeTagsProvider(), "http.client.requests", autoTimer)).build(); + MockRestServiceServer mockServer = MockRestServiceServer.createServer(restTemplate); + mockServer.expect(MockRestRequestMatchers.requestTo("/first/123")) + .andExpect(MockRestRequestMatchers.method(HttpMethod.GET)) + .andRespond(MockRestResponseCreators.withSuccess("OK", MediaType.APPLICATION_JSON)); + mockServer.expect(MockRestRequestMatchers.requestTo("/second/456")) + .andExpect(MockRestRequestMatchers.method(HttpMethod.GET)) + .andRespond(MockRestResponseCreators.withSuccess("OK", MediaType.APPLICATION_JSON)); + assertThat(restTemplate.getForObject("/first/{id}", String.class, 123)).isEqualTo("OK"); + assertThat(this.registry.find("http.client.requests").timer()).isNull(); + enabled.set(true); + assertThat(restTemplate.getForObject(URI.create("/second/456"), String.class)).isEqualTo("OK"); + this.registry.get("http.client.requests").tags("uri", "/second/456").timer(); + this.mockServer.verify(); + + } + private static final class TestInterceptor implements ClientHttpRequestInterceptor { private final RestTemplate restTemplate;