Fixed feign-micrometer exception handling.

OpenFeign · Nov 22, 2024 · b9dc9a7 · b9dc9a7
1 parent 4f6cff2
commit b9dc9a7
Show file tree

Hide file tree

Showing 2 changed files with 51 additions and 18 deletions.
diff --git a/micrometer/src/main/java/feign/micrometer/MicrometerObservationCapability.java b/micrometer/src/main/java/feign/micrometer/MicrometerObservationCapability.java
@@ -21,6 +21,7 @@
 import feign.FeignException;
 import feign.Request;
 import feign.Response;
+import feign.Util;
 import io.micrometer.observation.Observation;
 import io.micrometer.observation.ObservationRegistry;
 
@@ -59,9 +60,18 @@ public Client enrich(Client client) {
       try {
         Response response = client.execute(request, options);
 
-        throwExceptionOnErrorStatusCode(response, request);
+        if (response.body() != null) {
+          response =
+              response.toBuilder().body(Util.toByteArray(response.body().asInputStream())).build();
+        }
 
-        finalizeObservation(feignContext, observation, null, response);
+        FeignException exception = null;
+
+        if (responseContainsError(response)) {
+          exception = getFeignExceptionFromStatusCode(response, request);
+        }
+
+        finalizeObservation(feignContext, observation, exception, response);
         return response;
       } catch (Exception ex) {
         finalizeObservation(feignContext, observation, ex, null);
@@ -89,12 +99,13 @@ public AsyncClient<Object> enrich(AsyncClient<Object> client) {
             .execute(feignContext.getCarrier(), options, context)
             .whenComplete(
                 (r, ex) -> {
-                  try {
-                    throwExceptionOnErrorStatusCode(r, request);
-                    finalizeObservation(feignContext, observation, ex, r);
-                  } catch (FeignException statusCodeException) {
-                    finalizeObservation(feignContext, observation, statusCodeException, null);
+                  FeignException exception = null;
+
+                  if (responseContainsError(r)) {
+                    exception = getFeignExceptionFromStatusCode(r, request);
                   }
+
+                  finalizeObservation(feignContext, observation, exception, r);
                 });
       } catch (Exception ex) {
         finalizeObservation(feignContext, observation, ex, null);
@@ -113,9 +124,14 @@ private void finalizeObservation(
     observation.stop();
   }
 
-  private void throwExceptionOnErrorStatusCode(Response response, Request request) {
-    if (response.status() >= 400 && response.status() < 600) {
-      throw FeignException.errorStatus(request.requestTemplate().method(), response);
+  private FeignException getFeignExceptionFromStatusCode(Response response, Request request) {
+    if (responseContainsError(response)) {
+      return FeignException.errorStatus(request.requestTemplate().method(), response);
     }
+    return null;
+  }
+
+  private boolean responseContainsError(Response response) {
+    return response.status() >= 400 && response.status() < 600;
   }
 }
diff --git a/micrometer/src/test/java/feign/micrometer/FeignHeaderInstrumentationTest.java b/micrometer/src/test/java/feign/micrometer/FeignHeaderInstrumentationTest.java
@@ -20,6 +20,7 @@
 import static com.github.tomakehurst.wiremock.client.WireMock.equalTo;
 import static com.github.tomakehurst.wiremock.client.WireMock.get;
 import static com.github.tomakehurst.wiremock.client.WireMock.getRequestedFor;
+import static com.github.tomakehurst.wiremock.client.WireMock.noContent;
 import static com.github.tomakehurst.wiremock.client.WireMock.ok;
 import static com.github.tomakehurst.wiremock.client.WireMock.stubFor;
 import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo;
@@ -99,6 +100,21 @@ void getTemplatedPathForUriWithException(WireMockRuntimeInfo wmRuntimeInfo) {
         .isEqualTo("BadRequest");
   }
 
+  @Test
+  void getTemplatedPathForUriWithNoContent(WireMockRuntimeInfo wmRuntimeInfo) {
+    stubFor(get(anyUrl()).willReturn(noContent()));
+
+    TestClient testClient = clientInstrumentedWithObservations(wmRuntimeInfo.getHttpBaseUrl());
+
+    try {
+      testClient.templated("1", "2");
+    } catch (FeignException e) {
+      assertThat(e).isInstanceOf(FeignException.BadRequest.class);
+    }
+
+    assertThat(meterRegistry.get(METER_NAME).meter().getId().getTag("error")).isEqualTo("none");
+  }
+
   @Test
   void getTemplatedPathForUriForAsync(WireMockRuntimeInfo wmRuntimeInfo)
       throws ExecutionException, InterruptedException {
@@ -117,18 +133,19 @@ void getTemplatedPathForUriForAsync(WireMockRuntimeInfo wmRuntimeInfo)
   }
 
   @Test
-  void getTemplatedPathForUriForAsyncWithException(WireMockRuntimeInfo wmRuntimeInfo) {
+  void getTemplatedPathForUriForAsyncWithException(WireMockRuntimeInfo wmRuntimeInfo)
+      throws InterruptedException {
     stubFor(get(anyUrl()).willReturn(badRequest()));
 
     AsyncTestClient testClient =
         asyncClientInstrumentedWithObservations(wmRuntimeInfo.getHttpBaseUrl());
-    testClient
-        .templated("1", "2")
-        .whenComplete(
-            (s, throwable) -> {
-              assertThat(meterRegistry.get(METER_NAME).meter().getId().getTag("error"))
-                  .isEqualTo("BadRequest");
-            });
+
+    try {
+      testClient.templated("1", "2").get();
+    } catch (ExecutionException e) {
+      assertThat(meterRegistry.get(METER_NAME).meter().getId().getTag("error"))
+          .isEqualTo("BadRequest");
+    }
   }
 
   private void assertTags() {