[GitHub] [cxf] bmaehr opened a new pull request #644: [CXF-8225] Structure send method of Slf4jEventSender

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

[GitHub] [cxf] bmaehr opened a new pull request #644: [CXF-8225] Structure send method of Slf4jEventSender

GitBox
bmaehr opened a new pull request #644: [CXF-8225] Structure send method of Slf4jEventSender
URL: https://github.com/apache/cxf/pull/644
 
 
   By extracting code into subroutines it is easier to override parts e.g. to make used Logger dependent of called method

----------------------------------------------------------------
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]


With regards,
Apache Git Services
Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] reta commented on a change in pull request #644: [CXF-8225] Structure send method of Slf4jEventSender

GitBox
reta commented on a change in pull request #644: [CXF-8225] Structure send method of Slf4jEventSender
URL: https://github.com/apache/cxf/pull/644#discussion_r385458485
 
 

 ##########
 File path: rt/features/logging/src/main/java/org/apache/cxf/ext/logging/slf4j/Slf4jEventSender.java
 ##########
 @@ -38,26 +38,10 @@
 
     @Override
     public void send(LogEvent event) {
-        String cat = "org.apache.cxf.services." + event.getPortTypeName().getLocalPart() + "." + event.getType();
-        Logger log = LoggerFactory.getLogger(cat);
+        Logger log = getLogger(event);
         Set<String> keys = new HashSet<>();
         try {
-            put(keys, "Type", event.getType().toString());
-            put(keys, "Address", event.getAddress());
-            put(keys, "HttpMethod", event.getHttpMethod());
-            put(keys, "Content-Type", event.getContentType());
-            put(keys, "ResponseCode", event.getResponseCode());
-            put(keys, "ExchangeId", event.getExchangeId());
-            put(keys, "MessageId", event.getMessageId());
-            if (event.getServiceName() != null) {
-                put(keys, "ServiceName", localPart(event.getServiceName()));
-                put(keys, "PortName", localPart(event.getPortName()));
-                put(keys, "PortTypeName", localPart(event.getPortTypeName()));
-            }
-            if (event.getFullContentFile() != null) {
-                put(keys, "FullContentFile", event.getFullContentFile().getAbsolutePath());
-            }
-            put(keys, "Headers", event.getHeaders().toString());
+            fillMDC(event, keys);
 
 Review comment:
   Since you are improving it, could we make `fillMDC` return keys instead of mutating them?
   
   ```
   Set<String> keys = fillMDC(event);
   ```
   
   Thanks!

----------------------------------------------------------------
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]


With regards,
Apache Git Services
Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] bmaehr commented on a change in pull request #644: [CXF-8225] Structure send method of Slf4jEventSender

GitBox
In reply to this post by GitBox
bmaehr commented on a change in pull request #644: [CXF-8225] Structure send method of Slf4jEventSender
URL: https://github.com/apache/cxf/pull/644#discussion_r385471899
 
 

 ##########
 File path: rt/features/logging/src/main/java/org/apache/cxf/ext/logging/slf4j/Slf4jEventSender.java
 ##########
 @@ -38,26 +38,10 @@
 
     @Override
     public void send(LogEvent event) {
-        String cat = "org.apache.cxf.services." + event.getPortTypeName().getLocalPart() + "." + event.getType();
-        Logger log = LoggerFactory.getLogger(cat);
+        Logger log = getLogger(event);
         Set<String> keys = new HashSet<>();
         try {
-            put(keys, "Type", event.getType().toString());
-            put(keys, "Address", event.getAddress());
-            put(keys, "HttpMethod", event.getHttpMethod());
-            put(keys, "Content-Type", event.getContentType());
-            put(keys, "ResponseCode", event.getResponseCode());
-            put(keys, "ExchangeId", event.getExchangeId());
-            put(keys, "MessageId", event.getMessageId());
-            if (event.getServiceName() != null) {
-                put(keys, "ServiceName", localPart(event.getServiceName()));
-                put(keys, "PortName", localPart(event.getPortName()));
-                put(keys, "PortTypeName", localPart(event.getPortTypeName()));
-            }
-            if (event.getFullContentFile() != null) {
-                put(keys, "FullContentFile", event.getFullContentFile().getAbsolutePath());
-            }
-            put(keys, "Headers", event.getHeaders().toString());
+            fillMDC(event, keys);
 
 Review comment:
   I can do it and thought about it (because it would be cleaner), but it has a little side effect: If there is an exception in the middle of the method fillMDC now the set MDCs are correctly reseted. If I return the keys, then not.

----------------------------------------------------------------
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]


With regards,
Apache Git Services
Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] reta commented on a change in pull request #644: [CXF-8225] Structure send method of Slf4jEventSender

GitBox
In reply to this post by GitBox
reta commented on a change in pull request #644: [CXF-8225] Structure send method of Slf4jEventSender
URL: https://github.com/apache/cxf/pull/644#discussion_r385506819
 
 

 ##########
 File path: rt/features/logging/src/main/java/org/apache/cxf/ext/logging/slf4j/Slf4jEventSender.java
 ##########
 @@ -38,26 +38,10 @@
 
     @Override
     public void send(LogEvent event) {
-        String cat = "org.apache.cxf.services." + event.getPortTypeName().getLocalPart() + "." + event.getType();
-        Logger log = LoggerFactory.getLogger(cat);
+        Logger log = getLogger(event);
         Set<String> keys = new HashSet<>();
         try {
-            put(keys, "Type", event.getType().toString());
-            put(keys, "Address", event.getAddress());
-            put(keys, "HttpMethod", event.getHttpMethod());
-            put(keys, "Content-Type", event.getContentType());
-            put(keys, "ResponseCode", event.getResponseCode());
-            put(keys, "ExchangeId", event.getExchangeId());
-            put(keys, "MessageId", event.getMessageId());
-            if (event.getServiceName() != null) {
-                put(keys, "ServiceName", localPart(event.getServiceName()));
-                put(keys, "PortName", localPart(event.getPortName()));
-                put(keys, "PortTypeName", localPart(event.getPortTypeName()));
-            }
-            if (event.getFullContentFile() != null) {
-                put(keys, "FullContentFile", event.getFullContentFile().getAbsolutePath());
-            }
-            put(keys, "Headers", event.getHeaders().toString());
+            fillMDC(event, keys);
 
 Review comment:
   Very valid point, we could separate prepare and populate (`prepareMDC`/`populateMDC`) steps but it looks like too much at this moment. LGTM, 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]


With regards,
Apache Git Services
Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] coheigea merged pull request #644: [CXF-8225] Structure send method of Slf4jEventSender

GitBox
In reply to this post by GitBox
coheigea merged pull request #644: [CXF-8225] Structure send method of Slf4jEventSender
URL: https://github.com/apache/cxf/pull/644
 
 
   

----------------------------------------------------------------
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]


With regards,
Apache Git Services