[GitHub] [nifi] turcsanyip commented on a diff in pull request #7315: NIFI-3065 Per Process Group logging

["turcsanyip (via GitHub)" <gi...@apache.org>]

turcsanyip commented on code in PR #7315:
URL: https://github.com/apache/nifi/pull/7315#discussion_r1212320791


##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/processor/SimpleProcessLogger.java:
##########
@@ -559,4 +564,33 @@ private Throwable findLastThrowable(final Object[] arguments) {
         }
         return lastThrowable;
     }
+
+    private String getDiscriminatorKey() {
+        return loggingContext.getDiscriminatorKey();
+    }
+
+    private String getLogFileSuffix() {
+        return loggingContext.getLogFileSuffix().orElse(null);
+    }
+
+    private void log(final Level level, final String message, final Object argument) {
+        log(level, message, argument, null);
+    }
+
+    private void log(final Level level, final String message, final Object argument, final Throwable throwable) {
+        if (throwable == null) {
+            logger.makeLoggingEventBuilder(level)
+                    .setMessage(message)
+                    .addArgument(argument)
+                    .addKeyValue(getDiscriminatorKey(), getLogFileSuffix())
+                    .log();

Review Comment:
   @timeabarna Thanks for implementing the PG level logging!
   
   Without reviewing the whole feature, I would just like to add some comments on `SimpleProcessLogger` log methods.
   
   It seems `LoggingEventBuilder.addArgument()` cannot receive multiple arguments (in our case `Object argument` is an array with multiple arguments). As a result, the log messages are not rendered properly: the whole argument array is replaced in the first `{}` placeholder, while the remaining `{}` placeholders stay unresolved:
   ```
   2023-05-31 23:08:39,074 ERROR [Timer-Driven Process Thread-7] o.a.n.p.helloworld.HelloWorldProcessor [HelloWorldProcessor[id=01881001-eeed-1379-569e-d0e8dd187a7d], foo, bar] Fatal error, arg1={}, arg2={}
   ```
   `addArgument()` could be called in a loop for each item in the array but I think there is a simpler solution: using vararg parameter. The `Throwable` does not need to be handled separately either, it can be the last item in the vararg. The underlying logging framework already handles it.
   
   So the method would simply look like this:
   ```
       private void log(final Level level, final String message, final Object... arguments) {
           logger.makeLoggingEventBuilder(level)
                   .addKeyValue(getDiscriminatorKey(), getLogFileSuffix())
                   .log(message, arguments);
       }
   ```



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

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org