[cxf] branch master updated: CXF-8367: Micrometer always reports outcome=UNKNOWN on success cases (#726)

classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|

[cxf] branch master updated: CXF-8367: Micrometer always reports outcome=UNKNOWN on success cases (#726)

reta
This is an automated email from the ASF dual-hosted git repository.

reta pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/cxf.git


The following commit(s) were added to refs/heads/master by this push:
     new c4e1d0e  CXF-8367: Micrometer always reports outcome=UNKNOWN on success cases (#726)
c4e1d0e is described below

commit c4e1d0ef71d4a86e5dc641c4a793cf294ac59509
Author: Andriy Redko <[hidden email]>
AuthorDate: Fri Nov 20 08:55:44 2020 -0500

    CXF-8367: Micrometer always reports outcome=UNKNOWN on success cases (#726)
---
 .../java/org/apache/cxf/message/MessageUtils.java  | 51 ++++++++++++++++++++++
 .../metrics/micrometer/provider/StandardTags.java  | 14 +++---
 .../provider/jaxws/JaxwsFaultCodeProvider.java     |  9 ++--
 .../micrometer/provider/StandardTagsTest.java      | 25 +++--------
 .../provider/jaxws/JaxwsFaultCodeProviderTest.java | 34 ++++++++++++++-
 .../transport/http/AbstractHTTPDestination.java    | 40 ++---------------
 6 files changed, 101 insertions(+), 72 deletions(-)

diff --git a/core/src/main/java/org/apache/cxf/message/MessageUtils.java b/core/src/main/java/org/apache/cxf/message/MessageUtils.java
index 34937f9..043c6af 100644
--- a/core/src/main/java/org/apache/cxf/message/MessageUtils.java
+++ b/core/src/main/java/org/apache/cxf/message/MessageUtils.java
@@ -20,6 +20,7 @@
 package org.apache.cxf.message;
 
 import java.lang.reflect.Method;
+import java.net.HttpURLConnection;
 import java.util.Optional;
 import java.util.logging.Logger;
 
@@ -205,4 +206,54 @@ public final class MessageUtils {
         }
         return Optional.ofNullable(method);
     }
+    
+    /**
+     * Gets the response code from the message and tries to deduct one if it
+     * is not set yet.
+     * @param message message to get response code from
+     * @return response code (or deducted value assuming success)
+     */
+    public static int getReponseCodeFromMessage(Message message) {
+        Integer i = (Integer)message.get(Message.RESPONSE_CODE);
+        if (i != null) {
+            return i.intValue();
+        }
+        int code = hasNoResponseContent(message) ? HttpURLConnection.HTTP_ACCEPTED : HttpURLConnection.HTTP_OK;
+        // put the code in the message so that others can get it
+        message.put(Message.RESPONSE_CODE, code);
+        return code;
+    }
+
+    /**
+     * Determines if the current message has no response content.
+     * The message has no response content if either:
+     *  - the request is oneway and the current message is no partial
+     *    response or an empty partial response.
+     *  - the request is not oneway but the current message is an empty partial
+     *    response.
+     * @param message
+     * @return
+     */
+    public static boolean hasNoResponseContent(Message message) {
+        final boolean ow = isOneWay(message);
+        final boolean pr = MessageUtils.isPartialResponse(message);
+        final boolean epr = MessageUtils.isEmptyPartialResponse(message);
+
+        //REVISIT may need to provide an option to choose other behavior?
+        // old behavior not suppressing any responses  => ow && !pr
+        // suppress empty responses for oneway calls   => ow && (!pr || epr)
+        // suppress additionally empty responses for decoupled twoway calls =>
+        return (ow && !pr) || epr;
+    }
+    
+    /**
+     * Checks if the message is oneway or not
+     * @param message the message under consideration
+     * @return true if the message has been marked as oneway
+     */
+    public static boolean isOneWay(Message message) {
+        final Exchange ex = message.getExchange();
+        return ex != null && ex.isOneWay();
+    }
+
 }
diff --git a/rt/features/metrics/src/main/java/org/apache/cxf/metrics/micrometer/provider/StandardTags.java b/rt/features/metrics/src/main/java/org/apache/cxf/metrics/micrometer/provider/StandardTags.java
index 07247a5..f62d0ce 100644
--- a/rt/features/metrics/src/main/java/org/apache/cxf/metrics/micrometer/provider/StandardTags.java
+++ b/rt/features/metrics/src/main/java/org/apache/cxf/metrics/micrometer/provider/StandardTags.java
@@ -24,6 +24,7 @@ import java.util.Optional;
 
 import org.apache.cxf.common.util.StringUtils;
 import org.apache.cxf.message.Message;
+import org.apache.cxf.message.MessageUtils;
 
 import io.micrometer.core.instrument.Tag;
 
@@ -65,10 +66,8 @@ public class StandardTags {
 
     public Tag status(Message response) {
         return ofNullable(response)
-                .map(e -> e.get(Message.RESPONSE_CODE))
-                .filter(e -> e instanceof Integer)
-                .map(e -> (Integer) e)
-                .map(String::valueOf)
+                .map(e -> MessageUtils.getReponseCodeFromMessage(response))
+                .map(code -> Integer.toString(code))
                 .map(status -> Tag.of("status", status))
                 .orElse(STATUS_UNKNOWN);
     }
@@ -90,11 +89,8 @@ public class StandardTags {
     }
 
     public Tag outcome(Message response) {
-        Optional<Integer> statusCode =
-                ofNullable(response)
-                        .map(e -> e.get(Message.RESPONSE_CODE))
-                        .filter(e -> e instanceof Integer)
-                        .map(e -> (Integer) e);
+        Optional<Integer> statusCode = ofNullable(response)
+           .map(e -> MessageUtils.getReponseCodeFromMessage(response));
         if (!statusCode.isPresent()) {
             return OUTCOME_UNKNOWN;
         }
diff --git a/rt/features/metrics/src/main/java/org/apache/cxf/metrics/micrometer/provider/jaxws/JaxwsFaultCodeProvider.java b/rt/features/metrics/src/main/java/org/apache/cxf/metrics/micrometer/provider/jaxws/JaxwsFaultCodeProvider.java
index 7b9f7f0..9e11c50 100644
--- a/rt/features/metrics/src/main/java/org/apache/cxf/metrics/micrometer/provider/jaxws/JaxwsFaultCodeProvider.java
+++ b/rt/features/metrics/src/main/java/org/apache/cxf/metrics/micrometer/provider/jaxws/JaxwsFaultCodeProvider.java
@@ -26,20 +26,17 @@ public class JaxwsFaultCodeProvider {
     
     public String getFaultCode(Exchange ex, boolean client) {
         FaultMode fm = ex.get(FaultMode.class);
+        // We check OutFaultMessage/InFaultMessage only because some features propagate the
+        // fault mode using InMessage/OutMessage (which may not end-up with a fault), for
+        // example check MAPAggregatorImpl.
         if (client) {
             if (fm == null && ex.getInFaultMessage() != null) {
                 fm = ex.getInFaultMessage().get(FaultMode.class);
             }
-            if (fm == null && ex.getOutMessage() != null) {
-                fm = ex.getOutMessage().get(FaultMode.class);
-            }
         } else {
             if (fm == null && ex.getOutFaultMessage() != null) {
                 fm = ex.getOutFaultMessage().get(FaultMode.class);
             }
-            if (fm == null && ex.getInMessage() != null) {
-                fm = ex.getInMessage().get(FaultMode.class);
-            }
         }
         
         if (fm == null) {
diff --git a/rt/features/metrics/src/test/java/org/apache/cxf/metrics/micrometer/provider/StandardTagsTest.java b/rt/features/metrics/src/test/java/org/apache/cxf/metrics/micrometer/provider/StandardTagsTest.java
index 92a13b3..c5e25ba 100644
--- a/rt/features/metrics/src/test/java/org/apache/cxf/metrics/micrometer/provider/StandardTagsTest.java
+++ b/rt/features/metrics/src/test/java/org/apache/cxf/metrics/micrometer/provider/StandardTagsTest.java
@@ -192,26 +192,27 @@ public class StandardTagsTest {
     }
 
     @Test
-    public void testStatusReturnWithUnknownWhenStatusIsNull() {
+    public void testStatusReturnWith200WhenResponseCodeIsNotSet() {
         // given
 
         // when
         Tag actual = underTest.status(response);
 
         // then
-        assertThat(actual, is(Tag.of(STATUS_METRIC_NAME, "UNKNOWN")));
+        assertThat(actual, is(Tag.of(STATUS_METRIC_NAME, "200")));
     }
 
     @Test
-    public void testStatusReturnWithUnknownWhenResponseCodeIsNotInteger() {
+    public void testStatusReturnWith202WhenResponseCodeIsNullAndResponseIsPartial() {
         // given
-        doReturn("").when(request).get(Message.RESPONSE_CODE);
+        doReturn(null).when(request).get(Message.RESPONSE_CODE);
+        doReturn(true).when(request).get(Message.EMPTY_PARTIAL_RESPONSE_MESSAGE);
 
         // when
         Tag actual = underTest.status(request);
 
         // then
-        assertThat(actual, is(Tag.of(STATUS_METRIC_NAME, "UNKNOWN")));
+        assertThat(actual, is(Tag.of(STATUS_METRIC_NAME, "202")));
     }
 
     @Test
@@ -327,18 +328,6 @@ public class StandardTagsTest {
         Tag actual = underTest.outcome(response);
 
         // then
-        assertThat(actual, is(Tag.of(OUTCOME_METRIC_NAME, "UNKNOWN")));
-    }
-
-    @Test
-    public void testOutcomeReturnWithUnknownWhenMethodIsNotInteger() {
-        // given
-        doReturn(new Object()).when(response).get(Message.BASE_PATH);
-
-        // when
-        Tag actual = underTest.outcome(response);
-
-        // then
-        assertThat(actual, is(Tag.of(OUTCOME_METRIC_NAME, "UNKNOWN")));
+        assertThat(actual, is(Tag.of(OUTCOME_METRIC_NAME, "SUCCESS")));
     }
 }
diff --git a/rt/features/metrics/src/test/java/org/apache/cxf/metrics/micrometer/provider/jaxws/JaxwsFaultCodeProviderTest.java b/rt/features/metrics/src/test/java/org/apache/cxf/metrics/micrometer/provider/jaxws/JaxwsFaultCodeProviderTest.java
index 0dae669..326ab34 100644
--- a/rt/features/metrics/src/test/java/org/apache/cxf/metrics/micrometer/provider/jaxws/JaxwsFaultCodeProviderTest.java
+++ b/rt/features/metrics/src/test/java/org/apache/cxf/metrics/micrometer/provider/jaxws/JaxwsFaultCodeProviderTest.java
@@ -77,6 +77,21 @@ public class JaxwsFaultCodeProviderTest {
         // then
         assertThat(actual, equalTo(RUNTIME_FAULT_STRING));
     }
+    
+    @Test
+    public void testFaultModeIsNotPresentButInFaultModeIsPresentThenShouldReturnThat() {
+        // given
+        doReturn(null).when(ex).get(FaultMode.class);
+        doReturn(message).when(ex).getInFaultMessage();
+        doReturn(RUNTIME_FAULT).when(message).get(FaultMode.class);
+
+        // when
+        String actual = underTest.getFaultCode(ex, true);
+
+        // then
+        assertThat(actual, equalTo(RUNTIME_FAULT_STRING));
+    }
+
 
     @Test
     public void testFaultModeIsNotPresentButOutFaultModeIsMissingThenShouldReturnNull() {
@@ -92,7 +107,7 @@ public class JaxwsFaultCodeProviderTest {
     }
 
     @Test
-    public void testNeitherFaultModeNorOutFaultModePresentsThenShouldReturnInMessagesFaultMode() {
+    public void testNeitherFaultModeNorOutFaultModePresentsThenShouldNotReturnInMessageFaultMode() {
         // given
         doReturn(null).when(ex).get(FaultMode.class);
         doReturn(null).when(ex).getOutFaultMessage();
@@ -103,7 +118,22 @@ public class JaxwsFaultCodeProviderTest {
         String actual = underTest.getFaultCode(ex, false);
 
         // then
-        assertThat(actual, equalTo(RUNTIME_FAULT_STRING));
+        assertThat(actual, is(nullValue()));
+    }
+    
+    @Test
+    public void testNeitherFaultModeNorOutFaultModePresentsThenShouldNotReturnOutMessageFaultMode() {
+        // given
+        doReturn(null).when(ex).get(FaultMode.class);
+        doReturn(null).when(ex).getInFaultMessage();
+        doReturn(message).when(ex).getOutMessage();
+        doReturn(RUNTIME_FAULT).when(message).get(FaultMode.class);
+
+        // when
+        String actual = underTest.getFaultCode(ex, true);
+
+        // then
+        assertThat(actual, is(nullValue()));
     }
 
     @Test
diff --git a/rt/transports/http/src/main/java/org/apache/cxf/transport/http/AbstractHTTPDestination.java b/rt/transports/http/src/main/java/org/apache/cxf/transport/http/AbstractHTTPDestination.java
index 2beaf93..2be88b3 100644
--- a/rt/transports/http/src/main/java/org/apache/cxf/transport/http/AbstractHTTPDestination.java
+++ b/rt/transports/http/src/main/java/org/apache/cxf/transport/http/AbstractHTTPDestination.java
@@ -22,7 +22,6 @@ package org.apache.cxf.transport.http;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
-import java.net.HttpURLConnection;
 import java.net.ServerSocket;
 import java.net.URL;
 import java.nio.charset.StandardCharsets;
@@ -235,8 +234,7 @@ public abstract class AbstractHTTPDestination
      * @return true iff the message has been marked as oneway
      */
     protected final boolean isOneWay(Message message) {
-        Exchange ex = message.getExchange();
-        return ex != null && ex.isOneWay();
+        return MessageUtils.isOneWay(message);
     }
 
     public void invoke(final ServletConfig config,
@@ -632,7 +630,7 @@ public abstract class AbstractHTTPDestination
 
         HttpServletResponse response = getHttpResponseFromMessage(outMessage);
 
-        int responseCode = getReponseCodeFromMessage(outMessage);
+        int responseCode = MessageUtils.getReponseCodeFromMessage(outMessage);
         if (responseCode >= 300) {
             String ec = (String)outMessage.get(Message.ERROR_MESSAGE);
             if (!StringUtils.isEmpty(ec)) {
@@ -645,7 +643,7 @@ public abstract class AbstractHTTPDestination
 
         outMessage.put(RESPONSE_HEADERS_COPIED, "true");
 
-        if (hasNoResponseContent(outMessage)) {
+        if (MessageUtils.hasNoResponseContent(outMessage)) {
             response.setContentLength(0);
             response.flushBuffer();
             closeResponseOutputStream(response);
@@ -669,38 +667,6 @@ public abstract class AbstractHTTPDestination
         }
     }
 
-    private int getReponseCodeFromMessage(Message message) {
-        Integer i = (Integer)message.get(Message.RESPONSE_CODE);
-        if (i != null) {
-            return i.intValue();
-        }
-        int code = hasNoResponseContent(message) ? HttpURLConnection.HTTP_ACCEPTED : HttpURLConnection.HTTP_OK;
-        // put the code in the message so that others can get it
-        message.put(Message.RESPONSE_CODE, code);
-        return code;
-    }
-
-    /**
-     * Determines if the current message has no response content.
-     * The message has no response content if either:
-     *  - the request is oneway and the current message is no partial
-     *    response or an empty partial response.
-     *  - the request is not oneway but the current message is an empty partial
-     *    response.
-     * @param message
-     * @return
-     */
-    private boolean hasNoResponseContent(Message message) {
-        final boolean ow = isOneWay(message);
-        final boolean pr = MessageUtils.isPartialResponse(message);
-        final boolean epr = MessageUtils.isEmptyPartialResponse(message);
-
-        //REVISIT may need to provide an option to choose other behavior?
-        // old behavior not suppressing any responses  => ow && !pr
-        // suppress empty responses for oneway calls   => ow && (!pr || epr)
-        // suppress additionally empty responses for decoupled twoway calls =>
-        return (ow && !pr) || epr;
-    }
 
     private HttpServletResponse getHttpResponseFromMessage(Message message) throws IOException {
         Object responseObj = message.get(HTTP_RESPONSE);