From ac729f141d0f2d7ea483a999cacc1cbfeb49e9bc Mon Sep 17 00:00:00 2001 From: Jason Hinch <594559+jhinch@users.noreply.github.com> Date: Sat, 8 May 2021 06:21:54 +1000 Subject: [PATCH] Modify WebClient tracing to include the uri template in generated span names (#1940) Spans names are in the form ' ' (e.g. 'GET /path/{id}'). This naming strategy matches the span names generated by TraceWebFilter. The improve naming strategy is not applied to RestTemplate as the uri template is not readily available to interceptors and RestTemplate is in maintenance mode --- .../client/TraceExchangeFilterFunction.java | 36 +++++++++++++++++-- ...FilterFunctionHttpClientResponseTests.java | 4 +-- .../integration/sampled/WebClientTests.java | 25 +++++++++++++ 3 files changed, 60 insertions(+), 5 deletions(-) diff --git a/spring-cloud-sleuth-instrumentation/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceExchangeFilterFunction.java b/spring-cloud-sleuth-instrumentation/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceExchangeFilterFunction.java index 487148e1e..9efc62798 100644 --- a/spring-cloud-sleuth-instrumentation/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceExchangeFilterFunction.java +++ b/spring-cloud-sleuth-instrumentation/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceExchangeFilterFunction.java @@ -38,10 +38,12 @@ import org.springframework.cloud.sleuth.http.HttpClientRequest; import org.springframework.cloud.sleuth.http.HttpClientResponse; import org.springframework.cloud.sleuth.instrument.reactor.TraceContextPropagator; import org.springframework.context.ConfigurableApplicationContext; +import org.springframework.http.HttpMethod; import org.springframework.web.reactive.function.client.ClientRequest; import org.springframework.web.reactive.function.client.ClientResponse; import org.springframework.web.reactive.function.client.ExchangeFilterFunction; import org.springframework.web.reactive.function.client.ExchangeFunction; +import org.springframework.web.reactive.function.client.WebClient; /** * Trace representation of {@link ExchangeFilterFunction}. @@ -53,6 +55,8 @@ public final class TraceExchangeFilterFunction implements ExchangeFilterFunction private static final Log log = LogFactory.getLog(TraceExchangeFilterFunction.class); + private static final String URI_TEMPLATE_ATTRIBUTE = WebClient.class.getName() + ".uriTemplate"; + final ConfigurableApplicationContext springContext; // Lazy initialized fields @@ -155,12 +159,18 @@ public final class TraceExchangeFilterFunction implements ExchangeFilterFunction final CurrentTraceContext currentTraceContext; + final HttpMethod method; + + final String httpRoute; + TraceWebClientSubscriber(CoreSubscriber actual, Context ctx, Span clientSpan, TraceContext parent, MonoWebClientTrace mono) { this.actual = actual; this.parent = parent; this.handler = mono.handler; this.currentTraceContext = mono.currentTraceContext; + this.method = mono.request.method(); + this.httpRoute = (String) mono.request.attribute(URI_TEMPLATE_ATTRIBUTE).orElse(null); this.context = this.parent != null && !this.parent.equals(ctx.getOrDefault(TraceContext.class, null)) ? ctx.put(TraceContext.class, this.parent) : ctx; set(clientSpan); @@ -186,8 +196,7 @@ public final class TraceExchangeFilterFunction implements ExchangeFilterFunction if (log.isTraceEnabled()) { log.trace("OnNext finally"); } - // TODO: is there a way to read the request at response time? - this.handler.handleReceive(new ClientResponseWrapper(response), span); + this.handler.handleReceive(new ClientResponseWrapper(response, this.method, this.httpRoute), span); } } } @@ -330,6 +339,11 @@ public final class TraceExchangeFilterFunction implements ExchangeFilterFunction return delegate.url().getPath(); } + @Override + public String route() { + return (String) delegate.attribute(URI_TEMPLATE_ATTRIBUTE).orElse(null); + } + @Override public String url() { return delegate.url().toString(); @@ -355,8 +369,24 @@ public final class TraceExchangeFilterFunction implements ExchangeFilterFunction final ClientResponse delegate; - ClientResponseWrapper(ClientResponse delegate) { + final HttpMethod method; + + final String httpRoute; + + ClientResponseWrapper(ClientResponse delegate, HttpMethod method, String httpRoute) { this.delegate = delegate; + this.method = method; + this.httpRoute = httpRoute; + } + + @Override + public String method() { + return method.name(); + } + + @Override + public String route() { + return httpRoute; } @Override diff --git a/spring-cloud-sleuth-instrumentation/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/TraceExchangeFilterFunctionHttpClientResponseTests.java b/spring-cloud-sleuth-instrumentation/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/TraceExchangeFilterFunctionHttpClientResponseTests.java index be159e3a4..04b85469f 100644 --- a/spring-cloud-sleuth-instrumentation/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/TraceExchangeFilterFunctionHttpClientResponseTests.java +++ b/spring-cloud-sleuth-instrumentation/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/TraceExchangeFilterFunctionHttpClientResponseTests.java @@ -29,7 +29,7 @@ public class TraceExchangeFilterFunctionHttpClientResponseTests { public void should_return_0_when_invalid_status_code_is_returned() { ClientResponse clientResponse = BDDMockito.mock(ClientResponse.class); BDDMockito.given(clientResponse.rawStatusCode()).willReturn(-1); - ClientResponseWrapper response = new ClientResponseWrapper(clientResponse); + ClientResponseWrapper response = new ClientResponseWrapper(clientResponse, null, null); Integer statusCode = response.statusCode(); @@ -40,7 +40,7 @@ public class TraceExchangeFilterFunctionHttpClientResponseTests { public void should_return_status_code_when_valid_status_code_is_returned() { ClientResponse clientResponse = BDDMockito.mock(ClientResponse.class); BDDMockito.given(clientResponse.rawStatusCode()).willReturn(200); - ClientResponseWrapper response = new ClientResponseWrapper(clientResponse); + ClientResponseWrapper response = new ClientResponseWrapper(clientResponse, null, null); Integer statusCode = response.statusCode(); diff --git a/tests/common/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/integration/sampled/WebClientTests.java b/tests/common/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/integration/sampled/WebClientTests.java index 89fad7d8f..6f9f9c9ab 100644 --- a/tests/common/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/integration/sampled/WebClientTests.java +++ b/tests/common/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/integration/sampled/WebClientTests.java @@ -68,6 +68,7 @@ import org.springframework.http.ResponseEntity; import org.springframework.test.annotation.DirtiesContext; import org.springframework.test.context.ContextConfiguration; import org.springframework.test.context.TestPropertySource; +import org.springframework.web.bind.annotation.PathVariable; import org.springframework.web.bind.annotation.RequestHeader; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RequestMethod; @@ -252,6 +253,25 @@ public abstract class WebClientTests { .collect(Collectors.toList())).isNotEmpty().contains("CLIENT"); } + @Test + @SuppressWarnings("unchecked") + public void shouldUseUriTemplateInSpanName() { + Span span = this.tracer.nextSpan().name("foo").start(); + + try (Tracer.SpanInScope ws = this.tracer.withSpan(span)) { + this.webClientBuilder.baseUrl("http://localhost:" + this.port).build().get() + .uri("/prefix/{variable}/suffix", "value").retrieve().bodyToMono(String.class) + .block(Duration.ofSeconds(5)); + } + finally { + span.end(); + } + + thenThereIsNoCurrentSpan(); + then(this.spans.reportedSpans().stream().filter(r -> r.getKind() == Span.Kind.CLIENT).map(r -> r.getName()) + .collect(Collectors.toList())).isNotEmpty().contains("GET /prefix/{variable}/suffix"); + } + /** * Cancel before {@link Subscription#request(long)} means a network request was never * sent @@ -549,6 +569,11 @@ public abstract class WebClientTests { return "ok"; } + @RequestMapping(value = "/prefix/{variable}/suffix", method = RequestMethod.GET) + String pathVariable(@PathVariable("variable") String variable) { + return "variable = " + variable; + } + } @Configuration(proxyBeanMethods = false)