This is an automated email from the ASF dual-hosted git repository. reta pushed a commit to branch 3.3.x-fixes in repository https://gitbox.apache.org/repos/asf/cxf.git
commit 0665e4c6db56056ed40b04732eb24b20cd5c3ffa Author: Andriy Redko <[email protected]> AuthorDate: Fri Nov 20 08:55:44 2020 -0500 CXF-8367: Micrometer always reports outcome=UNKNOWN on success cases (#726) (cherry picked from commit c4e1d0ef71d4a86e5dc641c4a793cf294ac59509) --- .../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 3159d02..ca41d51 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 15de3bc..741c9a4 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);
