[GitHub] [cxf] reta opened a new pull request #726: CXF-8367: Micrometer always reports outcome=UNKNOWN on success cases

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] reta opened a new pull request #726: CXF-8367: Micrometer always reports outcome=UNKNOWN on success cases

GitBox

reta opened a new pull request #726:
URL: https://github.com/apache/cxf/pull/726


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] reta commented on a change in pull request #726: CXF-8367: Micrometer always reports outcome=UNKNOWN on success cases

GitBox

reta commented on a change in pull request #726:
URL: https://github.com/apache/cxf/pull/726#discussion_r524817976



##########
File path: rt/features/metrics/src/main/java/org/apache/cxf/metrics/micrometer/provider/jaxws/JaxwsFaultCodeProvider.java
##########
@@ -26,20 +26,17 @@
     
     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) {

Review comment:
       @shark300 sorry for pinging, I've removed In Message (and Out Message) from the fault code resolution. It seems like it is used by some features and interceptors, however the fault logic only relies on In Fault / Out Fault.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] reta commented on a change in pull request #726: CXF-8367: Micrometer always reports outcome=UNKNOWN on success cases

GitBox
In reply to this post by GitBox

reta commented on a change in pull request #726:
URL: https://github.com/apache/cxf/pull/726#discussion_r524817976



##########
File path: rt/features/metrics/src/main/java/org/apache/cxf/metrics/micrometer/provider/jaxws/JaxwsFaultCodeProvider.java
##########
@@ -26,20 +26,17 @@
     
     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) {

Review comment:
       @shark300 sorry for pinging, I've removed In Message (and Out Message) from the fault code resolution. It seems like it is used by some features and interceptors, however the fault logic only relies on In Fault / Out Fault. Do you have any concerns with this change? Thank you.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] shark300 commented on a change in pull request #726: CXF-8367: Micrometer always reports outcome=UNKNOWN on success cases

GitBox
In reply to this post by GitBox

shark300 commented on a change in pull request #726:
URL: https://github.com/apache/cxf/pull/726#discussion_r524975642



##########
File path: rt/features/metrics/src/main/java/org/apache/cxf/metrics/micrometer/provider/jaxws/JaxwsFaultCodeProvider.java
##########
@@ -26,20 +26,17 @@
     
     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) {

Review comment:
       No problem, thank you for that :)
   "Adjust the StandardTags.status() method to treat null values as 200" Is it true in this case? I mean what is the HTTP status code in this case? As I see, the logic should be similar to this one: https://github.com/spring-projects/spring-boot/blob/master/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/http/Outcome.java
   
   Could you add a new integration test for this case?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] reta commented on a change in pull request #726: CXF-8367: Micrometer always reports outcome=UNKNOWN on success cases

GitBox
In reply to this post by GitBox

reta commented on a change in pull request #726:
URL: https://github.com/apache/cxf/pull/726#discussion_r525620400



##########
File path: rt/features/metrics/src/main/java/org/apache/cxf/metrics/micrometer/provider/jaxws/JaxwsFaultCodeProvider.java
##########
@@ -26,20 +26,17 @@
     
     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) {

Review comment:
       Thanks @shark300 , if for any reasons the response code is not set, we reuse what logic from `AbstractHttpDestination` (https://github.com/apache/cxf/pull/726/files#diff-d9de014df7f37e23ad17d9091884dd0edcd9203b26d6f1a4c8d55b9f802ab957R216), in the nutshell it is `200` or `202` (partial response), not ideal but at least consistent.
   
   I don't have an integration test for this particular case (it is difficult to verify that indeed response code was not set on server side, since client will always get it), but I have unit tests for it.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] reta commented on a change in pull request #726: CXF-8367: Micrometer always reports outcome=UNKNOWN on success cases

GitBox
In reply to this post by GitBox

reta commented on a change in pull request #726:
URL: https://github.com/apache/cxf/pull/726#discussion_r525620400



##########
File path: rt/features/metrics/src/main/java/org/apache/cxf/metrics/micrometer/provider/jaxws/JaxwsFaultCodeProvider.java
##########
@@ -26,20 +26,17 @@
     
     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) {

Review comment:
       Thanks @shark300 , if for any reasons the response code is not set, we reuse the logic from `AbstractHttpDestination` (https://github.com/apache/cxf/pull/726/files#diff-d9de014df7f37e23ad17d9091884dd0edcd9203b26d6f1a4c8d55b9f802ab957R216), in the nutshell it is `200` or `202` (partial response), not ideal but at least consistent.
   
   I don't have an integration test for this particular case (it is difficult to verify that indeed response code was not set on server side, since client will always get it), but I have unit tests for it.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] reta merged pull request #726: CXF-8367: Micrometer always reports outcome=UNKNOWN on success cases

GitBox
In reply to this post by GitBox

reta merged pull request #726:
URL: https://github.com/apache/cxf/pull/726


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]