-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add log4j logging and bridge #387
base: master
Are you sure you want to change the base?
Conversation
Enables logging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested with dist created with installDist
, ran flux with log-object
, got output 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you introducing log4j
, and also logback
? We're already using slf4j
.
runtimeOnly 'org.apache.logging.log4j:log4j-core' | ||
implementation 'org.apache.logging.log4j:log4j-1.2-api:2.9.1' | ||
implementation 'ch.qos.logback:logback-classic:1.3.14' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
runtimeOnly 'org.apache.logging.log4j:log4j-core' | |
implementation 'org.apache.logging.log4j:log4j-1.2-api:2.9.1' | |
implementation 'ch.qos.logback:logback-classic:1.3.14' | |
runtimeOnly "org.slf4j:slf4j-log4j12:${versions.slf4j}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this suggestion, I get no logging output, and the same Class path contains multiple SLF4J bindings.
warning that I get without this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That warning aside (different issue, IMO), you would have to increase the log level to DEBUG
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same with DEBUG
in log4j.xml
: no logging output. As you wrote below, it must be using some other config?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not what I saw. Changing the log level (priority) in log4j.xml
and then running ./gradlew installDist
yields debug output with log-object
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird, I have <priority value="DEBUG" />
in metafix/src/main/resources/log4j.xml
, only runtimeOnly "org.slf4j:slf4j-log4j12:${versions.slf4j}"
added in metafix-runner/build.gradle
: no logging output for log-object
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And you ran ./gradlew installDist
afterwards? For me on master
:
- Copy
log4j.xml
from metafacture-core tometafix/src/main/resources/
. - Change priority
info
toDEBUG
inmetafix/src/main/resources/log4j.xml
. - Add
runtimeOnly "org.slf4j:slf4j-log4j12:${versions.slf4j}"
tometafix-runner/build.gradle
. - Run
./gradlew installDist
. - Change into
metafix-runner/build/install/metafix-runner
directory. - Run
bin/metafix-runner /path/to/flux-with-log-object
. - See "DEBUG [main] ..." output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@blackwinter your proposal is working for me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tried it on my home machine, and here it works. Maybe some cached dependency bringing in some other config file on my office machine? So that would be +1 for using only runtimeOnly "org.slf4j:slf4j-log4j12:${versions.slf4j}"
. However, it works here even without adding any dependency, just by switching INFO
to DEBUG
in metafix/src/main/resources/log4j.xml
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, it works here even without adding any dependency
Indeed. Probably because it's pulled in by metafacture-runner. So let's just drop it altogether then.
But not without increasing the log level first, right? |
Oh, right, weird. I saw the original commit 47304f9 and thought it makes sense, but the current version is actually INFO, but I still get the DEBUG output, without any changes to |
I see. Now that you mention it, the log format is also different than what is specified in the config. Could it be that logback ignores the config file? |
That's another reason why I think we shouldn't introduce any new logging dependencies. metafacture-runner uses |
</appender> | ||
|
||
<root> | ||
<priority value="INFO" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplify changing the log level at runtime:
<priority value="INFO" /> | |
<priority value="${env:METAFIX_LOG_LEVEL:-INFO}" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is the variable set? export METAFIX_LOG_LEVEL=DEBUG; bin/metafix-runner $pathTo.flux
didnt' work. Same for export METAFIX_LOG_LEVEL="DEBUG"
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmpf, no this works , BUT it's always on DEBUG level regardless what I set .... oh , logging is so complicated ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, sorry, I overlooked that we're on Log4j 1.x which doesn't seem to support property substitution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it doesn't work. It's effectively an invalid/empty log level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, apparently it does support property substitution, it's just not readily documented. However, I wasn't able to figure out how to supply a default value in the configuration file. So it gets a little more complicated:
<priority value="${org.metafacture.metafix.logLevel}" />
diff --git metafix-runner/build.gradle metafix-runner/build.gradle
index 1179b5c..ab71abb 100644
--- metafix-runner/build.gradle
+++ metafix-runner/build.gradle
@@ -39,11 +39,15 @@ dependencies {
application {
mainClass = 'org.metafacture.runner.Flux'
+ applicationDefaultJvmArgs = [
+ "-Dorg.metafacture.metafix.logLevel=INFO"
+ ]
+
if (project.hasProperty('profile')) {
def file = project.getProperty('profile') ?: project.name
def depth = project.hasProperty('profile.depth') ? project.getProperty('profile.depth') : 8
- applicationDefaultJvmArgs = [
+ applicationDefaultJvmArgs += [
"-XX:FlightRecorderOptions=stackdepth=${depth}",
"-XX:StartFlightRecording=dumponexit=true,filename=${file}.jfr,settings=profile"
]
Then run with JAVA_OPTS=-Dorg.metafacture.metafix.logLevel=DEBUG
.
runtimeOnly 'org.apache.logging.log4j:log4j-core' | ||
implementation 'org.apache.logging.log4j:log4j-1.2-api:2.9.1' | ||
implementation 'ch.qos.logback:logback-classic:1.3.14' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No additional dependencies required:
runtimeOnly 'org.apache.logging.log4j:log4j-core' | |
implementation 'org.apache.logging.log4j:log4j-1.2-api:2.9.1' | |
implementation 'ch.qos.logback:logback-classic:1.3.14' |
</appender> | ||
|
||
<root> | ||
<priority value="INFO" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allow changing the log level at runtime:
<priority value="INFO" /> | |
<priority value="${org.metafacture.metafix.logLevel}" /> |
Specify default log level:
diff --git metafix-runner/build.gradle metafix-runner/build.gradle
index 1179b5c..ab71abb 100644
--- metafix-runner/build.gradle
+++ metafix-runner/build.gradle
@@ -39,11 +39,15 @@ dependencies {
application {
mainClass = 'org.metafacture.runner.Flux'
+ applicationDefaultJvmArgs = [
+ "-Dorg.metafacture.metafix.logLevel=INFO"
+ ]
+
if (project.hasProperty('profile')) {
def file = project.getProperty('profile') ?: project.name
def depth = project.hasProperty('profile.depth') ? project.getProperty('profile.depth') : 8
- applicationDefaultJvmArgs = [
+ applicationDefaultJvmArgs += [
"-XX:FlightRecorderOptions=stackdepth=${depth}",
"-XX:StartFlightRecording=dumponexit=true,filename=${file}.jfr,settings=profile"
]
Change log level at runtime with JAVA_OPTS=-Dorg.metafacture.metafix.logLevel=DEBUG
.
Enables logging.
See #383.