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 2022/02/18 21:32:18 UTC
[GitHub] [daffodil] alexanderrevello opened a new pull request #759: redirect dfdlx:trace output to trace logging and create integration t…
alexanderrevello opened a new pull request #759:
URL: https://github.com/apache/daffodil/pull/759
…est to demonstrate
--
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] alexanderrevello commented on a change in pull request #759: redirect dfdlx:trace output to trace logging and create integration t…
Posted by GitBox <gi...@apache.org>.
alexanderrevello commented on a change in pull request #759:
URL: https://github.com/apache/daffodil/pull/759#discussion_r812047567
##########
File path: daffodil-cli/src/it/scala/org/apache/daffodil/parsing/TestCLIParsing.scala
##########
@@ -1319,4 +1319,34 @@ class TestCLIparsing {
}
+ @Test def test_2575_DFDLX_Trace_output(): Unit = {
+
+ val schemaFile = Util.daffodilPath("daffodil-cli/src/it/resources/org/apache/daffodil/CLI/trace_input.dfdl.xsd")
+ val inputFile = Util.daffodilPath("daffodil-cli/src/it/resources/org/apache/daffodil/CLI/input/trace_input.txt")
+ val log4jConfig = Util.daffodilPath("daffodil-cli/src/it/resources/org/apache/daffodil/CLI/conf/log4j2_traceOutput.xml")
+ val (testSchemaFile, testInputFile, testLog4jConfig) = if (Util.isWindows) (Util.cmdConvert(schemaFile), Util.cmdConvert(inputFile), Util.cmdConvert(log4jConfig)) else (schemaFile, inputFile, log4jConfig)
+
+ val DAFFODIL_JAVA_OPTS = Map("DAFFODIL_JAVA_OPTS" -> ("-Dlog4j.configurationFile=" + testLog4jConfig + " -Xms256m -Xmx2048m -Dfile.encoding=UTF-8"))
+
+ val shell = Util.start("", envp = DAFFODIL_JAVA_OPTS)
+
+ try{
+
+ val cmd = String.format("%s -vvvv parse -r output -s %s %s", Util.binPath, testSchemaFile, testInputFile)
+
+ shell.sendLine(cmd)
+
+ //show stderr has no output
+ shell.expectIn(1,contains(""))
Review comment:
This was only for this particular test it can be changed in whatever configuration is defined by the user. The log4j configuration file was changed to output to stdout instead of stderr because the PTR was to not put directly into stderr, but if using the log4j interface it would not be directly in stderr
--
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 #759: redirect dfdlx:trace output to trace logging and create integration t…
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on pull request #759:
URL: https://github.com/apache/daffodil/pull/759#issuecomment-1048238930
In the future, you don't need to close a pull request if you have updates. You can just push changes to your branch and the PR will be updated. Once the PR is approve, you can squash + force push and we can merge the PR. It makes it a little easier if all the changes are in a single PR.
--
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] alexanderrevello commented on a change in pull request #759: redirect dfdlx:trace output to trace logging and create integration t…
Posted by GitBox <gi...@apache.org>.
alexanderrevello commented on a change in pull request #759:
URL: https://github.com/apache/daffodil/pull/759#discussion_r812018751
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/dpath/DFDLXFunctions.scala
##########
@@ -185,7 +186,7 @@ case class DFDLXTrace(recipe: CompiledDPath, msg: String)
}
case other: DINode => other.namedQName.toString
}
- System.err.println("trace " + msg + ":" + nodeString)
+ Logger.log.trace(s"dfdlx:trace ${msg} : ${nodeString}")
Review comment:
I was under the impression that users would want to capture logged information by itself using the log4j configuration, but if that is not the case the level should be raised. Would it be too cost intensive to have an SDW and a log4j info?
--
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] alexanderrevello commented on a change in pull request #759: redirect dfdlx:trace output to trace logging and create integration t…
Posted by GitBox <gi...@apache.org>.
alexanderrevello commented on a change in pull request #759:
URL: https://github.com/apache/daffodil/pull/759#discussion_r812015387
##########
File path: daffodil-cli/src/it/scala/org/apache/daffodil/parsing/TestCLIParsing.scala
##########
@@ -1319,4 +1319,34 @@ class TestCLIparsing {
}
+ @Test def test_2575_DFDLX_Trace_output(): Unit = {
+
+ val schemaFile = Util.daffodilPath("daffodil-cli/src/it/resources/org/apache/daffodil/CLI/trace_input.dfdl.xsd")
+ val inputFile = Util.daffodilPath("daffodil-cli/src/it/resources/org/apache/daffodil/CLI/input/trace_input.txt")
+ val log4jConfig = Util.daffodilPath("daffodil-cli/src/it/resources/org/apache/daffodil/CLI/conf/log4j2_traceOutput.xml")
+ val (testSchemaFile, testInputFile, testLog4jConfig) = if (Util.isWindows) (Util.cmdConvert(schemaFile), Util.cmdConvert(inputFile), Util.cmdConvert(log4jConfig)) else (schemaFile, inputFile, log4jConfig)
+
+ val DAFFODIL_JAVA_OPTS = Map("DAFFODIL_JAVA_OPTS" -> ("-Dlog4j.configurationFile=" + testLog4jConfig + " -Xms256m -Xmx2048m -Dfile.encoding=UTF-8"))
Review comment:
Yes there is a regex filter that captures the trace output of dfdlx:trace only, I agree it will not be needed if the level is changed higher
--
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] alexanderrevello closed pull request #759: redirect dfdlx:trace output to trace logging and create integration t…
Posted by GitBox <gi...@apache.org>.
alexanderrevello closed pull request #759:
URL: https://github.com/apache/daffodil/pull/759
--
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] alexanderrevello commented on a change in pull request #759: redirect dfdlx:trace output to trace logging and create integration t…
Posted by GitBox <gi...@apache.org>.
alexanderrevello commented on a change in pull request #759:
URL: https://github.com/apache/daffodil/pull/759#discussion_r812022622
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/dpath/DFDLXFunctions.scala
##########
@@ -185,7 +186,7 @@ case class DFDLXTrace(recipe: CompiledDPath, msg: String)
}
case other: DINode => other.namedQName.toString
}
- System.err.println("trace " + msg + ":" + nodeString)
+ Logger.log.trace(s"dfdlx:trace ${msg} : ${nodeString}")
Review comment:
the level should be raised regardless and I agree info would be better
--
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] alexanderrevello commented on pull request #759: redirect dfdlx:trace output to trace logging and create integration t…
Posted by GitBox <gi...@apache.org>.
alexanderrevello commented on pull request #759:
URL: https://github.com/apache/daffodil/pull/759#issuecomment-1048376494
Sorry I misinterpreted the single commit and other rules disallowing merging, I will not do this again in the future
--
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 #759: redirect dfdlx:trace output to trace logging and create integration t…
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #759:
URL: https://github.com/apache/daffodil/pull/759#discussion_r811913090
##########
File path: daffodil-cli/src/it/scala/org/apache/daffodil/parsing/TestCLIParsing.scala
##########
@@ -1319,4 +1319,34 @@ class TestCLIparsing {
}
+ @Test def test_2575_DFDLX_Trace_output(): Unit = {
+
+ val schemaFile = Util.daffodilPath("daffodil-cli/src/it/resources/org/apache/daffodil/CLI/trace_input.dfdl.xsd")
+ val inputFile = Util.daffodilPath("daffodil-cli/src/it/resources/org/apache/daffodil/CLI/input/trace_input.txt")
+ val log4jConfig = Util.daffodilPath("daffodil-cli/src/it/resources/org/apache/daffodil/CLI/conf/log4j2_traceOutput.xml")
+ val (testSchemaFile, testInputFile, testLog4jConfig) = if (Util.isWindows) (Util.cmdConvert(schemaFile), Util.cmdConvert(inputFile), Util.cmdConvert(log4jConfig)) else (schemaFile, inputFile, log4jConfig)
+
+ val DAFFODIL_JAVA_OPTS = Map("DAFFODIL_JAVA_OPTS" -> ("-Dlog4j.configurationFile=" + testLog4jConfig + " -Xms256m -Xmx2048m -Dfile.encoding=UTF-8"))
+
+ val shell = Util.start("", envp = DAFFODIL_JAVA_OPTS)
+
+ try{
+
+ val cmd = String.format("%s -vvvv parse -r output -s %s %s", Util.binPath, testSchemaFile, testInputFile)
+
+ shell.sendLine(cmd)
+
+ //show stderr has no output
+ shell.expectIn(1,contains(""))
Review comment:
Why have the log output go to stdout instead of stderr. Reduces the possibility of ever having a case where the actual output of daffodil is confused with the actual log output.
##########
File path: daffodil-cli/src/it/scala/org/apache/daffodil/parsing/TestCLIParsing.scala
##########
@@ -1319,4 +1319,34 @@ class TestCLIparsing {
}
+ @Test def test_2575_DFDLX_Trace_output(): Unit = {
+
+ val schemaFile = Util.daffodilPath("daffodil-cli/src/it/resources/org/apache/daffodil/CLI/trace_input.dfdl.xsd")
+ val inputFile = Util.daffodilPath("daffodil-cli/src/it/resources/org/apache/daffodil/CLI/input/trace_input.txt")
+ val log4jConfig = Util.daffodilPath("daffodil-cli/src/it/resources/org/apache/daffodil/CLI/conf/log4j2_traceOutput.xml")
+ val (testSchemaFile, testInputFile, testLog4jConfig) = if (Util.isWindows) (Util.cmdConvert(schemaFile), Util.cmdConvert(inputFile), Util.cmdConvert(log4jConfig)) else (schemaFile, inputFile, log4jConfig)
+
+ val DAFFODIL_JAVA_OPTS = Map("DAFFODIL_JAVA_OPTS" -> ("-Dlog4j.configurationFile=" + testLog4jConfig + " -Xms256m -Xmx2048m -Dfile.encoding=UTF-8"))
Review comment:
I think if you change the log level to not trace, you probably shouldn't need to change the log4j configuration, so this can maybe go way. I assume this is just needed to deal with the crazy verbosity of the trace log level?
##########
File path: daffodil-cli/src/it/resources/org/apache/daffodil/CLI/input/trace_input.txt
##########
@@ -0,0 +1 @@
+0
Review comment:
I would suggest just making the test use the `Util.echoN()` function so that input data comes is read from stdin rather than a file. It simplifies the test a bit, avoids a file with just a zero in it, and avoids having to update the Rat.scala file.
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/dpath/DFDLXFunctions.scala
##########
@@ -185,7 +186,7 @@ case class DFDLXTrace(recipe: CompiledDPath, msg: String)
}
case other: DINode => other.namedQName.toString
}
- System.err.println("trace " + msg + ":" + nodeString)
+ Logger.log.trace(s"dfdlx:trace ${msg} : ${nodeString}")
Review comment:
Even though the function is called `dfdlx:trace` I'm not sure we want to actually log the message at a the `trace()` log level. The trace log level is incredibly verbose, and should really only ever be used when a developer is debugging Daffodil.
Unlike the trace log level, the `dfdlx:trace` function is used when someone wants to debug a schema, which rarely requires a deap level of daffodil debug level logging. I would argue we want to log at either the error, warning, or info log level. Those are normal log levels that won't overwhelm the user with log messages. Though, I'm not sure which is more appropriate. Info feels more correct to me, but it means the user would need to bump the verbosity to get any output since we default to the warning log level, so that might be intuitive. This would also simply the test a bit since you would really need the complex log4j2 configuration.
Maybe another option would be to use an SDW instead of the logging infrastructure. This always shows up regardless of the log level, and it can be easily suppressed. Though, there might be issue with backtracking--I'm not sure if backtracking also removes SDWs, which we definitely don't want
--
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