From 05471b053bbb4e21eef57171a51272678981954d Mon Sep 17 00:00:00 2001
From: Trask Stalnaker <trask.stalnaker@gmail.com>
Date: Tue, 22 Nov 2022 15:09:22 -0800
Subject: [PATCH] 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
https://github.com/open-telemetry/opentelemetry-java-instrumentation/commit/6f472a62a0eea340608c9b275f7333ef22945ff6#diff-493ad89b5bde807c90387aa2bb67eb10d3bcef6b6a388bd31e11796a6d01ac38R36
---
 .../client/SpringWebfluxHttpClientTest.groovy | 29 ++++++++++++++-----
 .../client/SpringWebfluxTelemetry.java        |  2 +-
 .../test/base/HttpClientTest.groovy           | 10 +++++--
 3 files changed, 31 insertions(+), 10 deletions(-)

diff --git a/instrumentation/spring/spring-webflux-5.0/javaagent/src/test/groovy/client/SpringWebfluxHttpClientTest.groovy b/instrumentation/spring/spring-webflux-5.0/javaagent/src/test/groovy/client/SpringWebfluxHttpClientTest.groovy
index 674a5da83c..13329cd874 100644
--- a/instrumentation/spring/spring-webflux-5.0/javaagent/src/test/groovy/client/SpringWebfluxHttpClientTest.groovy
+++ b/instrumentation/spring/spring-webflux-5.0/javaagent/src/test/groovy/client/SpringWebfluxHttpClientTest.groovy
@@ -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
+    }
+  }
 }
diff --git a/instrumentation/spring/spring-webflux-5.0/library/src/main/java/io/opentelemetry/instrumentation/spring/webflux/client/SpringWebfluxTelemetry.java b/instrumentation/spring/spring-webflux-5.0/library/src/main/java/io/opentelemetry/instrumentation/spring/webflux/client/SpringWebfluxTelemetry.java
index a330bc4449..d80d5f70b1 100644
--- a/instrumentation/spring/spring-webflux-5.0/library/src/main/java/io/opentelemetry/instrumentation/spring/webflux/client/SpringWebfluxTelemetry.java
+++ b/instrumentation/spring/spring-webflux-5.0/library/src/main/java/io/opentelemetry/instrumentation/spring/webflux/client/SpringWebfluxTelemetry.java
@@ -46,6 +46,6 @@ public final class SpringWebfluxTelemetry {
         return;
       }
     }
-    exchangeFilterFunctions.add(0, new WebClientTracingFilter(instrumenter, propagators));
+    exchangeFilterFunctions.add(new WebClientTracingFilter(instrumenter, propagators));
   }
 }
diff --git a/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpClientTest.groovy b/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpClientTest.groovy
index 3749889afd..54c4441c70 100644
--- a/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpClientTest.groovy
+++ b/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpClientTest.groovy
@@ -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"() {