Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add log4j logging and bridge #387

wants to merge 1 commit into from

Conversation

dr0i
Copy link
Member

@dr0i dr0i commented Nov 22, 2024

Enables logging.

See #383.

Copy link
Member

@fsteeg fsteeg left a 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 👍

@fsteeg fsteeg removed their assignment Nov 22, 2024
Copy link
Member

@blackwinter blackwinter left a 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.

Comment on lines +33 to +35
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'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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}"

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

@fsteeg fsteeg Nov 22, 2024

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?

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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:

  1. Copy log4j.xml from metafacture-core to metafix/src/main/resources/.
  2. Change priority info to DEBUG in metafix/src/main/resources/log4j.xml.
  3. Add runtimeOnly "org.slf4j:slf4j-log4j12:${versions.slf4j}" to metafix-runner/build.gradle.
  4. Run ./gradlew installDist.
  5. Change into metafix-runner/build/install/metafix-runner directory.
  6. Run bin/metafix-runner /path/to/flux-with-log-object.
  7. See "DEBUG [main] ..." output.

Copy link
Member Author

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

Copy link
Member

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.

Copy link
Member

@blackwinter blackwinter Nov 22, 2024

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.

@blackwinter
Copy link
Member

Tested with dist created with installDist, ran flux with log-object, got output 👍

But not without increasing the log level first, right?

@blackwinter blackwinter assigned dr0i and unassigned blackwinter Nov 22, 2024
@fsteeg
Copy link
Member

fsteeg commented Nov 22, 2024

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

@blackwinter
Copy link
Member

but I still get the DEBUG output, without any changes to log4j.xml.

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?

@blackwinter
Copy link
Member

That's another reason why I think we shouldn't introduce any new logging dependencies. metafacture-runner uses slf4j-log4j and so should metafix-runner.

</appender>

<root>
<priority value="INFO" />
Copy link
Member

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:

Suggested change
<priority value="INFO" />
<priority value="${env:METAFIX_LOG_LEVEL:-INFO}" />

Copy link
Member Author

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

Copy link
Member Author

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

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

Comment on lines +33 to +35
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'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No additional dependencies required:

Suggested change
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" />
Copy link
Member

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:

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Review
Development

Successfully merging this pull request may close these issues.

3 participants