You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by GitBox <gi...@apache.org> on 2021/07/08 12:31:46 UTC
[GitHub] [daffodil] stevedlawrence opened a new pull request #605: Replace logging infrastructure with Log4j
stevedlawrence opened a new pull request #605:
URL: https://github.com/apache/daffodil/pull/605
- Remove the existing logging infrastructure and replace it with the
Log4j Scala API. Log4j Scala API and Log4j API are added as
dependencies. Log4j Core is added as a test dependency and a CLI
dependency. Users of the Daffodil API are expected to manually add
log4j-core or some other Log4j implementation to enable logging
- Distribute a Log4j configuration with the CLI to maintain a similar
log outut as with the previous implementation
- The Scala/Java API logging functions are deprecated and are now
no-ops. The functions were not thread safe and could lead to random
test failures. Logging tests are removed--we simply use the API and
rely on Log4j testing for correctness
- We no longer have the different log levels as before, we know just
have error, warn, info, debug, and trace
DAFFODIL-2510
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mbeckerle commented on a change in pull request #605: Replace logging infrastructure with Log4j
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #605:
URL: https://github.com/apache/daffodil/pull/605#discussion_r667009200
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/compiler/Compiler.scala
##########
@@ -352,15 +351,15 @@ class Compiler private (var validateDFDLSchemas: Boolean,
val diags = pf.getDiagnostics // might be warnings even if not isError
if (err) {
Assert.invariant(diags.nonEmpty)
- log(LogLevel.Compile, "Compilation (ProcessorFactory) produced %d errors/warnings.", diags.length)
+ Logger.log.debug(s"Compilation (ProcessorFactory) produced ${diags.length} errors/warnings.")
Review comment:
I take your point that string interpolation is better because it rules out a class of errors at compile time, which I like.
Suppose the macro expanded so that it passes both the raw string (no interpolated substitutions) and the evaluated string (with substitutions) to the underlying messaging layer.
Then I think the underlying layer can handle the internationalization just fine. By comparing the raw and substituted strings it can extract the args to the string interpolation, for use with an internationalized version of the text.
So a more clever macro is the only thing needed. No source code changes. That gets rid of my concerns about this.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #605: Replace logging infrastructure with Log4j
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #605:
URL: https://github.com/apache/daffodil/pull/605#discussion_r674075461
##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -1585,33 +1554,33 @@ object Main extends Logging {
} catch {
case s: scala.util.control.ControlThrowable => throw s
case e: java.io.FileNotFoundException => {
- log(LogLevel.Error, "%s", e.getMessage())
+ Logger.log.error(e.getMessage())
1
}
case e: InvalidParserException => {
- log(LogLevel.Error, "%s", e.getMessage())
+ Logger.log.error(e.getMessage())
1
}
case e: ExternalVariableException => {
- log(LogLevel.Error, "%s", e.getMessage())
+ Logger.log.error(e.getMessage())
1
}
case e: BindingException => {
- log(LogLevel.Error, "%s", e.getMessage())
+ Logger.log.error(e.getMessage())
1
}
case e: NotYetImplementedException => {
nyiFound(e)
}
case e: TDMLException => {
- log(LogLevel.Error, "%s", e.getMessage())
+ Logger.log.error(e.getMessage())
1
}
case e: OutOfMemoryError => {
oomError(e)
}
case e: UserDefinedFunctionFatalErrorException => {
- log(LogLevel.Error, "%s", e.getMessage())
+ Logger.log.error(e.getMessage())
e.printStackTrace()
Review comment:
Good point.
In most of these cases, the exceptions are Daffodil defined exceptions, and I think we're pretty good about making sure`e.geMessage()` returns something useful. If that's not the case, I'd consider that a bug and something we should fix. Haven't had any reports about that specifically though.
The only above exception that isn't Daffodil defined is `FileNotFoundException`, but I think that is usually good about having a helpful message about what file wasn't found.
I'm also a little hesitant to print out stack traces and class names that are somewhat low level--these exceptions are all normal behavior related to bad user input, so I'd be afraid that too much detail might confuse users that aren't familiar with Java exceptions/stack traces.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on pull request #605: Replace logging infrastructure with Log4j
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on pull request #605:
URL: https://github.com/apache/daffodil/pull/605#issuecomment-876702282
> You should try turning on all logging levels (full logging, "trace" level?) and run our test suite. It should still work, excepting tests that themselves are turning on/off logging. I know with the old logging, turning it on frequently resulted in stack overflows, circular OOLAG, or just aborts. All such failures are bugs. They don't have to be fixed in this PR, but we should find them anyway, or make a separate task to fix so that one can always turn logging on.
I did turn on trace level logging and there was only one bad log messsage that caused issues. I forget where it came from, but the change to fix it was here:
https://github.com/apache/daffodil/pull/605/files#diff-54ec9da14e47187ffdeeb515a11f925da7a44fee744fb423547135277973972bR242
I didn't have any other issues with stack overflows or circularities or anything.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mbeckerle commented on a change in pull request #605: Replace logging infrastructure with Log4j
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #605:
URL: https://github.com/apache/daffodil/pull/605#discussion_r674082412
##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -1585,33 +1554,33 @@ object Main extends Logging {
} catch {
case s: scala.util.control.ControlThrowable => throw s
case e: java.io.FileNotFoundException => {
- log(LogLevel.Error, "%s", e.getMessage())
+ Logger.log.error(e.getMessage())
1
}
case e: InvalidParserException => {
- log(LogLevel.Error, "%s", e.getMessage())
+ Logger.log.error(e.getMessage())
1
}
case e: ExternalVariableException => {
- log(LogLevel.Error, "%s", e.getMessage())
+ Logger.log.error(e.getMessage())
1
}
case e: BindingException => {
- log(LogLevel.Error, "%s", e.getMessage())
+ Logger.log.error(e.getMessage())
1
}
case e: NotYetImplementedException => {
nyiFound(e)
}
case e: TDMLException => {
- log(LogLevel.Error, "%s", e.getMessage())
+ Logger.log.error(e.getMessage())
1
}
case e: OutOfMemoryError => {
oomError(e)
}
case e: UserDefinedFunctionFatalErrorException => {
- log(LogLevel.Error, "%s", e.getMessage())
+ Logger.log.error(e.getMessage())
e.printStackTrace()
Review comment:
Yeah, we have a `Misc.getSomeMessage(exception)` function which was my response to the same `getMessage()` issue you have here. Worst case if there is neither a message nor a cause throwable object, this uses the class name as the message string. It returns a `Some[String] `as an attempt to clarify that the return from this isn't allowed to be null or empty string. It's going to be some useful message. I never thought of just taking item 1 from the stack trace and use that.
`Logger.log.error(Misc.getSomeMessage(e).get)`
would be my preference for how to do these.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence merged pull request #605: Replace logging infrastructure with Log4j
Posted by GitBox <gi...@apache.org>.
stevedlawrence merged pull request #605:
URL: https://github.com/apache/daffodil/pull/605
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] tuxji commented on a change in pull request #605: Replace logging infrastructure with Log4j
Posted by GitBox <gi...@apache.org>.
tuxji commented on a change in pull request #605:
URL: https://github.com/apache/daffodil/pull/605#discussion_r674027726
##########
File path: daffodil-cli/src/templates/bash-template
##########
@@ -73,4 +74,4 @@ then
CLASSPATH=$(cygpath -apw "$CLASSPATH")
fi
-exec java $JOPTS -cp "$CLASSPATH" "$MAINCLASS" "$@"
+exec java -Dlog4j.configurationFile="$CONFDIR/log4j2.xml" $JOPTS -cp "$CLASSPATH" "$MAINCLASS" "$@"
Review comment:
Do we really want to define "conf" on line 55 and "log4j2.xml" on line 77 separately from each other? Wouldn't it be better to define the entire filename on line 55 and use just the env var itself on line 77?
```bash
CONFFILE="$BINDIR/../conf/log4j2.xml"
...
exec java -Dlog4j.configurationFile="$CONFFILE" $JOPTS -cp "$CLASSPATH" "$MAINCLASS" "$@"
```
##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -1585,33 +1554,33 @@ object Main extends Logging {
} catch {
case s: scala.util.control.ControlThrowable => throw s
case e: java.io.FileNotFoundException => {
- log(LogLevel.Error, "%s", e.getMessage())
+ Logger.log.error(e.getMessage())
1
}
case e: InvalidParserException => {
- log(LogLevel.Error, "%s", e.getMessage())
+ Logger.log.error(e.getMessage())
1
}
case e: ExternalVariableException => {
- log(LogLevel.Error, "%s", e.getMessage())
+ Logger.log.error(e.getMessage())
1
}
case e: BindingException => {
- log(LogLevel.Error, "%s", e.getMessage())
+ Logger.log.error(e.getMessage())
1
}
case e: NotYetImplementedException => {
nyiFound(e)
}
case e: TDMLException => {
- log(LogLevel.Error, "%s", e.getMessage())
+ Logger.log.error(e.getMessage())
1
}
case e: OutOfMemoryError => {
oomError(e)
}
case e: UserDefinedFunctionFatalErrorException => {
- log(LogLevel.Error, "%s", e.getMessage())
+ Logger.log.error(e.getMessage())
e.printStackTrace()
Review comment:
All the changes in Main.scala look good. I just have one comment on all those Logger.log.error(e.getMessage()) calls. In my Java experience, the first line printed by e.printStackTrace() contains more information than the line printed by Logger.log.error(e.getMessage()). The first line contains the original exception class's fully qualified name followed by ": " and then the e.getMessage() string. I often found that sometimes the e.getMessage() string was empty or contained too little information and therefore I needed to know the original exception class's name as well. However, Main.scala was always calling e.getMessage() even before your changes, so I guess the loss of information wasn't a problem for Daffodil users, yes?
##########
File path: project/Dependencies.scala
##########
@@ -42,7 +44,8 @@ object Dependencies {
"org.fusesource.jansi" % "jansi" % "2.3.3",
"org.jline" % "jline" % "3.20.0",
"org.rogach" %% "scallop" % "4.0.3",
- "net.sf.expectit" % "expectit-core" % "0.9.0" % "it,test"
+ "org.apache.logging.log4j" % "log4j-core" % "2.14.1",
+ "net.sf.expectit" % "expectit-core" % "0.9.0" % "it,test",
)
Review comment:
When shall we start sorting those dependencies alphabetically?
##########
File path: daffodil-cli/src/templates/bat-template
##########
@@ -49,4 +50,4 @@ if defined DAFFODIL_JAVA_OPTS (
set JOPTS=%JAVA_OPTS%
)
-java %JOPTS% -cp "%CLASSPATH%;%DAFFODIL_CLASSPATH%" %MAINCLASS% %*
+java -Dlog4j.configurationFile="%CONFDIR%\log4j2.xml" %JOPTS% -cp "%CLASSPATH%;%DAFFODIL_CLASSPATH%" %MAINCLASS% %*
Review comment:
Likewise, wouldn't it be better to define the entire filename on line 42 and use just the env var on line 53?
```bat
set CONFFILE=%BINDIR%..\conf\log4j2.xml
...
java -Dlog4j.configurationFile="%CONFFILE%" $JOPTS -cp "%CLASSPATH%;%DAFFODIL_CLASSPATH%" %MAINCLASS% %*
```
I also think bat-template should redefine CLASSPATH to include DAFFODIL_CLASSPATH in an earlier if statement so that both bash-template and bat-template are more similar to each other, unless this change would break the bat script somehow.
```bat
if defined DAFFODIL_CLASSPATH (
set CLASSPATH="%CLASSPATH%;%DAFFODIL_CLASSPATH%"
)
...
java -Dlog4j.configurationFile="%CONFFILE%" $JOPTS -cp "%CLASSPATH%" %MAINCLASS% %*
```
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #605: Replace logging infrastructure with Log4j
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #605:
URL: https://github.com/apache/daffodil/pull/605#discussion_r674094569
##########
File path: project/Dependencies.scala
##########
@@ -42,7 +44,8 @@ object Dependencies {
"org.fusesource.jansi" % "jansi" % "2.3.3",
"org.jline" % "jline" % "3.20.0",
"org.rogach" %% "scallop" % "4.0.3",
- "net.sf.expectit" % "expectit-core" % "0.9.0" % "it,test"
+ "org.apache.logging.log4j" % "log4j-core" % "2.14.1",
+ "net.sf.expectit" % "expectit-core" % "0.9.0" % "it,test",
)
Review comment:
I'd prefer we do that as a separate PR so we avoid mixing functional changes with style changes. I recently leanred about the .git-blame-ignore-revs file to ignore certain commits in a blame, which could be useful for ignore style-only changes, but only if there's no mixed in functional changes.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #605: Replace logging infrastructure with Log4j
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #605:
URL: https://github.com/apache/daffodil/pull/605#discussion_r676548233
##########
File path: daffodil-cli/src/templates/bash-template
##########
@@ -73,4 +74,4 @@ then
CLASSPATH=$(cygpath -apw "$CLASSPATH")
fi
-exec java $JOPTS -cp "$CLASSPATH" "$MAINCLASS" "$@"
+exec java -Dlog4j.configurationFile="$CONFDIR/log4j2.xml" $JOPTS -cp "$CLASSPATH" "$MAINCLASS" "$@"
Review comment:
I've made the update to this file, but couldn't figure out a way to get it to work without requireing bash and arrays. I couldn't seem to get things to work correctly when things include spaces due to the quoting/expansion rules. If anyone knows any tricks to get it working with sh, that'd probably be preferred.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mbeckerle commented on a change in pull request #605: Replace logging infrastructure with Log4j
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #605:
URL: https://github.com/apache/daffodil/pull/605#discussion_r667072001
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/compiler/Compiler.scala
##########
@@ -352,15 +351,15 @@ class Compiler private (var validateDFDLSchemas: Boolean,
val diags = pf.getDiagnostics // might be warnings even if not isError
if (err) {
Assert.invariant(diags.nonEmpty)
- log(LogLevel.Compile, "Compilation (ProcessorFactory) produced %d errors/warnings.", diags.length)
+ Logger.log.debug(s"Compilation (ProcessorFactory) produced ${diags.length} errors/warnings.")
Review comment:
Good. So I think we need to do nothing right now. I just wanted to not go down a path that would leave us with no options.
Seems like we should move in this direction for other messages like for SDE and such also, so that this interpolator/macro technique would apply there as well. In the past we have had some bugs due to missing args to match a "%s" in the format string, or vice versa where there were args, but no corresponding "%s" in the format string.
Properly using string interpolation with macros would fix this, yet still be amenable to internationalization. Something to consider.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #605: Replace logging infrastructure with Log4j
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #605:
URL: https://github.com/apache/daffodil/pull/605#discussion_r666473470
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/Logger.scala
##########
@@ -17,213 +17,30 @@
package org.apache.daffodil.util
-import java.text.SimpleDateFormat
-import java.util.Date
-import java.io.File
-import java.io.PrintStream
-import java.io.FileOutputStream
-import org.apache.daffodil.exceptions.Assert
-import org.apache.daffodil.util._
-import org.apache.daffodil.util.Maybe._
-import org.apache.daffodil.exceptions.UnsuppressableException
+import org.apache.logging.log4j.scala.Logging
/**
- * Simple logging system evolved from code found on Stack Overflow, on the web.
- * http://stackoverflow.com/questions/2018528/logging-in-scala
- * Mostly based on the contribution of Don Mackenzie.
+ * The log4j.scala.Logging trait adds a log4j.scala.Logger member val called
+ * 'logger' to whatever class mixes it in. Classes that mixin this trait can
+ * then just call logger.warn/info/debug/etc to log a message, which are macros
+ * to minimize overhead.
*
- * Extensively modified to use Macros for performance now.
+ * However, the Logger class is not serializable, and so all classes that might
+ * need to be serialized, (e.g. runtime objects), cannot take this approach.
+ *
+ * Instead, we have a single Logger object that mixes in this trait. This
+ * object is never serialized and so can be used anywhere, regardless of
+ * serializablility. For simplicity, consistency, and to avoid potential
+ * serialization issues, all logging should be done via the logger in this
+ * object, rather than mixing in the Logging trait, for example:
+ *
+ * Logger.log.info("Message to log")
+ *
+ * The downside to this is that it breaks the ability to use Log4j's feature to
+ * configure different log levels for different namespaces or classes or have
+ * useful line numbers, because all logging comes from and is associated with
+ * this single Logger in the 'org.apache.daffodil.util.Logger' class.
Review comment:
Yep, I considered this and it should work, but I figured that could be a future enhancement if we determine it's needed.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #605: Replace logging infrastructure with Log4j
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #605:
URL: https://github.com/apache/daffodil/pull/605#discussion_r674095247
##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -1585,33 +1554,33 @@ object Main extends Logging {
} catch {
case s: scala.util.control.ControlThrowable => throw s
case e: java.io.FileNotFoundException => {
- log(LogLevel.Error, "%s", e.getMessage())
+ Logger.log.error(e.getMessage())
1
}
case e: InvalidParserException => {
- log(LogLevel.Error, "%s", e.getMessage())
+ Logger.log.error(e.getMessage())
1
}
case e: ExternalVariableException => {
- log(LogLevel.Error, "%s", e.getMessage())
+ Logger.log.error(e.getMessage())
1
}
case e: BindingException => {
- log(LogLevel.Error, "%s", e.getMessage())
+ Logger.log.error(e.getMessage())
1
}
case e: NotYetImplementedException => {
nyiFound(e)
}
case e: TDMLException => {
- log(LogLevel.Error, "%s", e.getMessage())
+ Logger.log.error(e.getMessage())
1
}
case e: OutOfMemoryError => {
oomError(e)
}
case e: UserDefinedFunctionFatalErrorException => {
- log(LogLevel.Error, "%s", e.getMessage())
+ Logger.log.error(e.getMessage())
e.printStackTrace()
Review comment:
Sounds reasonable. I'll make the update.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #605: Replace logging infrastructure with Log4j
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #605:
URL: https://github.com/apache/daffodil/pull/605#discussion_r674088854
##########
File path: daffodil-cli/src/templates/bat-template
##########
@@ -49,4 +50,4 @@ if defined DAFFODIL_JAVA_OPTS (
set JOPTS=%JAVA_OPTS%
)
-java %JOPTS% -cp "%CLASSPATH%;%DAFFODIL_CLASSPATH%" %MAINCLASS% %*
+java -Dlog4j.configurationFile="%CONFDIR%\log4j2.xml" %JOPTS% -cp "%CLASSPATH%;%DAFFODIL_CLASSPATH%" %MAINCLASS% %*
Review comment:
Yeah, the two files should have the exact same behavior. Whatever we decide for the .sh file we should duplicate here, and fixup any differences.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mbeckerle commented on a change in pull request #605: Replace logging infrastructure with Log4j
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #605:
URL: https://github.com/apache/daffodil/pull/605#discussion_r666435862
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/DFDLFormatAnnotation.scala
##########
@@ -146,7 +146,7 @@ abstract class DFDLFormatAnnotation(nodeArg: Node, annotatedSCArg: AnnotatedSche
val defFmt = schemaSet.getDefineFormat(adjustedQN).getOrElse {
annotatedSC.schemaDefinitionError("defineFormat with name '%s', was not found.", adjustedQN.toString)
}
- log(LogLevel.Debug, "found defineFormat named: %s", adjustedQN)
+ Logger.log.debug(s"found defineFormat named: ${adjustedQN}")
Review comment:
I would just delete this one.
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/Logger.scala
##########
@@ -17,213 +17,30 @@
package org.apache.daffodil.util
-import java.text.SimpleDateFormat
-import java.util.Date
-import java.io.File
-import java.io.PrintStream
-import java.io.FileOutputStream
-import org.apache.daffodil.exceptions.Assert
-import org.apache.daffodil.util._
-import org.apache.daffodil.util.Maybe._
-import org.apache.daffodil.exceptions.UnsuppressableException
+import org.apache.logging.log4j.scala.Logging
/**
- * Simple logging system evolved from code found on Stack Overflow, on the web.
- * http://stackoverflow.com/questions/2018528/logging-in-scala
- * Mostly based on the contribution of Don Mackenzie.
+ * The log4j.scala.Logging trait adds a log4j.scala.Logger member val called
+ * 'logger' to whatever class mixes it in. Classes that mixin this trait can
+ * then just call logger.warn/info/debug/etc to log a message, which are macros
+ * to minimize overhead.
*
- * Extensively modified to use Macros for performance now.
+ * However, the Logger class is not serializable, and so all classes that might
+ * need to be serialized, (e.g. runtime objects), cannot take this approach.
+ *
+ * Instead, we have a single Logger object that mixes in this trait. This
+ * object is never serialized and so can be used anywhere, regardless of
+ * serializablility. For simplicity, consistency, and to avoid potential
+ * serialization issues, all logging should be done via the logger in this
+ * object, rather than mixing in the Logging trait, for example:
+ *
+ * Logger.log.info("Message to log")
+ *
+ * The downside to this is that it breaks the ability to use Log4j's feature to
+ * configure different log levels for different namespaces or classes or have
+ * useful line numbers, because all logging comes from and is associated with
+ * this single Logger in the 'org.apache.daffodil.util.Logger' class.
Review comment:
We could have multiple logger objects if we wanted to, so as to get some of these capabilities back. Not sure we need it, but it seems possible.
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/DFDLFormatAnnotation.scala
##########
@@ -178,7 +178,7 @@ abstract class DFDLFormatAnnotation(nodeArg: Node, annotatedSCArg: AnnotatedSche
*/
Review comment:
Suggest just delete this entirely.
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/compiler/Compiler.scala
##########
@@ -352,15 +351,15 @@ class Compiler private (var validateDFDLSchemas: Boolean,
val diags = pf.getDiagnostics // might be warnings even if not isError
if (err) {
Assert.invariant(diags.nonEmpty)
- log(LogLevel.Compile, "Compilation (ProcessorFactory) produced %d errors/warnings.", diags.length)
+ Logger.log.debug(s"Compilation (ProcessorFactory) produced ${diags.length} errors/warnings.")
Review comment:
This style fundamentally bothers me. It is incompatible with Internationalization of the logging.
However, unlike error messages like our SDE/SDW/PE/UE/VE errors, logging quite possibly should be English only, so this is ok,
I just like the idea of having only one idiom for messaging - whether it is logging, issuing a SDE, or actually printing something out.
I always prefer this style:
```
loggingOrOtherMesageCreator("Message in English text with %s placeholders", arg0, arg1, arg2...argK)
```
as the style because it enables internationalization by using the Message string as a lookup key into a database by desired locale, and the object retrieved can scrutinize arguments to choose correct text.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] tuxji commented on a change in pull request #605: Replace logging infrastructure with Log4j
Posted by GitBox <gi...@apache.org>.
tuxji commented on a change in pull request #605:
URL: https://github.com/apache/daffodil/pull/605#discussion_r674223706
##########
File path: daffodil-cli/src/templates/bash-template
##########
@@ -73,4 +74,4 @@ then
CLASSPATH=$(cygpath -apw "$CLASSPATH")
fi
-exec java $JOPTS -cp "$CLASSPATH" "$MAINCLASS" "$@"
+exec java -Dlog4j.configurationFile="$CONFDIR/log4j2.xml" $JOPTS -cp "$CLASSPATH" "$MAINCLASS" "$@"
Review comment:
Yes, I'm fine with defining the configuration file in JOPTS. Let's do that.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #605: Replace logging infrastructure with Log4j
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #605:
URL: https://github.com/apache/daffodil/pull/605#discussion_r667077290
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/compiler/Compiler.scala
##########
@@ -352,15 +351,15 @@ class Compiler private (var validateDFDLSchemas: Boolean,
val diags = pf.getDiagnostics // might be warnings even if not isError
if (err) {
Assert.invariant(diags.nonEmpty)
- log(LogLevel.Compile, "Compilation (ProcessorFactory) produced %d errors/warnings.", diags.length)
+ Logger.log.debug(s"Compilation (ProcessorFactory) produced ${diags.length} errors/warnings.")
Review comment:
Good point. I'll create some tickets to update SDE's and to consider some internalization string interpolator.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #605: Replace logging infrastructure with Log4j
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #605:
URL: https://github.com/apache/daffodil/pull/605#discussion_r667039109
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/compiler/Compiler.scala
##########
@@ -352,15 +351,15 @@ class Compiler private (var validateDFDLSchemas: Boolean,
val diags = pf.getDiagnostics // might be warnings even if not isError
if (err) {
Assert.invariant(diags.nonEmpty)
- log(LogLevel.Compile, "Compilation (ProcessorFactory) produced %d errors/warnings.", diags.length)
+ Logger.log.debug(s"Compilation (ProcessorFactory) produced ${diags.length} errors/warnings.")
Review comment:
Thinking some one must have already done this, I did find this reddit thread about someone doing an internationalization string interpolator:
https://www.reddit.com/r/scala/comments/31iey4/scala_string_interpolation_why_the_s/
It doesn't use macros, and has some overhead to do things like rebuild the raw string, but I think the idea is sound. A macro implementation would probably be necessary once we wanted to support internationalization.
Another befit of using a string interpolator is that if we had something like `i"Internationalizable message: $foo"`, then it would fairly east to find all the `i"..."` strings that need to be translated to build the database.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #605: Replace logging infrastructure with Log4j
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #605:
URL: https://github.com/apache/daffodil/pull/605#discussion_r666471940
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/compiler/Compiler.scala
##########
@@ -352,15 +351,15 @@ class Compiler private (var validateDFDLSchemas: Boolean,
val diags = pf.getDiagnostics // might be warnings even if not isError
if (err) {
Assert.invariant(diags.nonEmpty)
- log(LogLevel.Compile, "Compilation (ProcessorFactory) produced %d errors/warnings.", diags.length)
+ Logger.log.debug(s"Compilation (ProcessorFactory) produced ${diags.length} errors/warnings.")
Review comment:
Unfortunately, the Scala API logger doesn't support this args:
https://logging.apache.org/log4j/scala/api/2.12/org/apache/logging/log4j/scala/Logger.html
I'm not sure if this is a limitation of their macro implementation, or if it's just missinng, but that's not really an option.
To support that, we would probably need to add our own wrapers, or do something like `"message %s".format(args)`, which I personally find kind of ugly, but I'm not that strongly against it.
Though, there is a benefit to use Scala's string interpolation over `format`, which is that it's impossible to create a format string that doesn't match the parameters. For example, if you do something like this
```scala
"Hello: %s %s".format(oneParam)
```
Then you end up getting an `MissingFormatArgumentException` at runtime. Using string interpolation, that's not possible. So there is a class of errors that simply go away.
Also, regarding internationalization, can you just internationalize the entire string? It's maybe bit more verbose, but there isn't that much of a difference between `Bonjour: %s` and `Bonjour: %{oneParam}`, aside from needing to be careful to not translate variable names.
I also wonder if string interpolation would allow for more complex internationalization logic? For example, you could maybe internationalize ordinals with something like:
```scala
s"This is the ${ordinalString("english", num)} thing"
s"C'est la ${ordinalString("french", num)} chose"
```
I don't really know much about internationalization, or how things like ordinals are usually handled, so maybe this makes no sense. But maybe does shows some flexibility over simple format strings.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #605: Replace logging infrastructure with Log4j
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #605:
URL: https://github.com/apache/daffodil/pull/605#discussion_r674086748
##########
File path: daffodil-cli/src/templates/bash-template
##########
@@ -73,4 +74,4 @@ then
CLASSPATH=$(cygpath -apw "$CLASSPATH")
fi
-exec java $JOPTS -cp "$CLASSPATH" "$MAINCLASS" "$@"
+exec java -Dlog4j.configurationFile="$CONFDIR/log4j2.xml" $JOPTS -cp "$CLASSPATH" "$MAINCLASS" "$@"
Review comment:
My thinking of using CONFDIR was that we might want to add additional configuration files in the future. This is only the configuration file for log4j, there might be others.
But I agree maybe we could do this better. Maybe we prepend additional java defines to JOPTS? Prepending allows user to be override the values. So for example, say we had multiple config files for different APIs, it might look something like this:
```bash
CONFDIR="$BINDIR/../conf/"
...
JOPTS="-Dlog4j.configurationFile=\"$CONFDIR/log4j2.xml\" $JOPTS"
JOPTS="-Dfoo.configurationFile=\"$CONFDIR/foo.xml\" $JOPTS"
JOPTS="-Dbar.configurationFile=\"$CONFDIR/bar.xml\" $JOPTS"
exec java $JOPTS -cp "$CLASSPATH" "$MAINCLASS" "$@"
```
That makes the exec line more clear and makes it obvious how to add new default java options if needed--just prepend them to JOPTS before we exec. Thoughts?
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #605: Replace logging infrastructure with Log4j
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #605:
URL: https://github.com/apache/daffodil/pull/605#discussion_r674235468
##########
File path: project/Dependencies.scala
##########
@@ -42,7 +44,8 @@ object Dependencies {
"org.fusesource.jansi" % "jansi" % "2.3.3",
"org.jline" % "jline" % "3.20.0",
"org.rogach" %% "scallop" % "4.0.3",
- "net.sf.expectit" % "expectit-core" % "0.9.0" % "it,test"
+ "org.apache.logging.log4j" % "log4j-core" % "2.14.1",
+ "net.sf.expectit" % "expectit-core" % "0.9.0" % "it,test",
)
Review comment:
I've created [DAFFODIL-2547](https://issues.apache.org/jira/browse/DAFFODIL-2547) to track sorting the depenencies.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org