Webflux instrumentation fix (#7251)

When a webflux filter is added which throws an exception, the
instrumentation does not currently capture the `http.status_code`.

The fix is to move `WebClientTracingFilter` from the first to the last
filter in the chain, which I think(?) is the general strategy we've
taken for other client instrumentation, e.g. so that if a filter makes
another http call it won't be suppressed.

I don't love the test coverage I added, so let me know if you have any
better suggestions?

EDIT: btw, I did archaeology to confirm that behavior (adding to the
beginning of the chain) has been in place since the webflux
instrumentation was added originally
6f472a62a0 (diff-493ad89b5bde807c90387aa2bb67eb10d3bcef6b6a388bd31e11796a6d01ac38R36)
This commit is contained in:
Trask Stalnaker 2022-11-22 15:09:22 -08:00 committed by GitHub
parent d8251d1fea
commit 05471b053b
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 31 additions and 10 deletions

View File

@ -14,7 +14,9 @@ import io.opentelemetry.instrumentation.testing.junit.http.SingleConnection
import io.opentelemetry.semconv.trace.attributes.SemanticAttributes
import org.springframework.http.HttpMethod
import org.springframework.http.client.reactive.ReactorClientHttpConnector
import org.springframework.web.reactive.function.client.ExchangeFilterFunction
import org.springframework.web.reactive.function.client.WebClient
import reactor.core.publisher.Mono
class SpringWebfluxHttpClientTest extends HttpClientTest<WebClient.RequestBodySpec> implements AgentTestTrait {
@ -31,7 +33,12 @@ class SpringWebfluxHttpClientTest extends HttpClientTest<WebClient.RequestBodySp
})
connector = new ReactorClientHttpConnector(httpClient)
}
return WebClient.builder().clientConnector(connector).build().method(HttpMethod.resolve(method))
return WebClient.builder()
.filter(ExchangeFilterFunction.ofResponseProcessor(
clientResponse -> {
return Mono.error(new TestException(clientResponse.statusCode().value()))
}))
.clientConnector(connector).build().method(HttpMethod.resolve(method))
.uri(uri)
.headers { h -> headers.forEach({ key, value -> h.add(key, value) }) }
}
@ -47,7 +54,11 @@ class SpringWebfluxHttpClientTest extends HttpClientTest<WebClient.RequestBodySp
@Override
int sendRequest(WebClient.RequestBodySpec request, String method, URI uri, Map<String, String> headers) {
return request.exchange().block().statusCode().value()
try {
return request.exchange().block().statusCode().value()
} catch (TestException e) {
return e.getStatusCode()
}
}
@Override
@ -80,11 +91,6 @@ class SpringWebfluxHttpClientTest extends HttpClientTest<WebClient.RequestBodySp
false
}
@Override
boolean testCallbackWithImplicitParent() {
true
}
@Override
Set<AttributeKey<?>> httpAttributes(URI uri) {
def attributes = super.httpAttributes(uri)
@ -96,4 +102,13 @@ class SpringWebfluxHttpClientTest extends HttpClientTest<WebClient.RequestBodySp
SingleConnection createSingleConnection(String host, int port) {
return new SpringWebFluxSingleConnection(isOldVersion(), host, port)
}
static class TestException extends RuntimeException {
int statusCode
TestException(int statusCode) {
this.statusCode = statusCode
}
}
}

View File

@ -46,6 +46,6 @@ public final class SpringWebfluxTelemetry {
return;
}
}
exchangeFilterFunctions.add(0, new WebClientTracingFilter(instrumenter, propagators));
exchangeFilterFunctions.add(new WebClientTracingFilter(instrumenter, propagators));
}
}

View File

@ -285,14 +285,20 @@ abstract class HttpClientTest<REQUEST> extends InstrumentationSpecification {
assumeTrue(testCallback())
assumeTrue(testCallbackWithParent())
expect:
junitTest.requestWithCallbackAndParent()
try {
junitTest.requestWithCallbackAndParent()
} catch (Exception ignored) {
}
}
def "trace request with callback and no parent"() {
assumeTrue(testCallback())
assumeFalse(testCallbackWithImplicitParent())
expect:
junitTest.requestWithCallbackAndNoParent()
try {
junitTest.requestWithCallbackAndNoParent()
} catch (Exception ignored) {
}
}
def "trace request with callback and implicit parent"() {