Skip to content

Commit

Permalink
Avoid capturing URI template when interceptor won't use it
Browse files Browse the repository at this point in the history
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
  • Loading branch information
wilkinsona committed Jun 16, 2021
1 parent 93fd7c6 commit fe078c8
Show file tree
Hide file tree
Showing 2 changed files with 47 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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);
Expand All @@ -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;

Expand All @@ -123,13 +127,17 @@ private CapturingUriTemplateHandler(UriTemplateHandler delegate) {

@Override
public URI expand(String url, Map<String, ?> 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);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down

0 comments on commit fe078c8

Please sign in to comment.