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

slf4j-jdk14 adapter: Logger.log's fqcn parameter ignored #425

Open
thomas-k-git opened this issue Aug 14, 2024 · 20 comments
Open

slf4j-jdk14 adapter: Logger.log's fqcn parameter ignored #425

thomas-k-git opened this issue Aug 14, 2024 · 20 comments
Assignees
Labels
DONE for fixed issues
Milestone

Comments

@thomas-k-git
Copy link

Dear all,
In the process of upgrading from slf4j1 to v2 of api and impl jars, we notice a difference in behavior, breaking our setup.

we have a complex setup with multiple adapters: jboss-logging into log4j2-to-slf4j into slf4j-jdk.

indirectly, org.slf4j.spi.LocationAwareLogger#log is called with the outmost logger class as fcqn parameter.

from my understanding the intention of the parameter is to indicate the boundary / entrypoint into the log framework and the next stackframe past this fcqn should be used as "calling class".

however in our setup, a middle layer of the log adapters is being set as java.util.logging.LogRecord#setSourceClassName and not the actual caller.

so with a callstack setup like this:

steArray = {StackTraceElement[131]@3780} 
 0 = {StackTraceElement@3781} "org.slf4j.jul.JDK14LoggerAdapter.fillCallerData(JDK14LoggerAdapter.java:182)"
 1 = {StackTraceElement@3782} "org.slf4j.jul.JDK14LoggerAdapter.innerNormalizedLoggingCallHandler(JDK14LoggerAdapter.java:155)"
 2 = {StackTraceElement@3783} "org.slf4j.jul.JDK14LoggerAdapter.log(JDK14LoggerAdapter.java:172)"
 3 = {StackTraceElement@3784} "org.apache.logging.slf4j.SLF4JLogger.logMessage(SLF4JLogger.java:245)"
 4 = {StackTraceElement@3785} "org.jboss.logging.Log4j2Logger.doLog(Log4j2Logger.java:54)"
 5 = {StackTraceElement@3786} "org.jboss.logging.Logger.infov(Logger.java:1034)"
 6 = {StackTraceElement@3787} "ACTUALCLASS"

we get
2024-08-14 14:22:14 INFO [org.apache.logging.slf4j.SLF4JLogger] message
instead of
2024-08-14 14:22:14 INFO [ACTUALCLASS] message

even though org.jboss.logging.Logger is the fcqn calling class.

it seems that behavior changed between version 1 and 2 here

I would expect precendence of the callerFQCN parameter of the org.slf4j.jul.JDK14LoggerAdapter#fillCallerData when it is present in the stacktrace (and the other BARRIER classes practically ignored in that case).

is my understanding wrong? is this a bug?

involved jars:

  • slf4j-jdk14-2.0.13.jar
  • log4j-to-slf4j-2.20.0.jar
  • jboss-logging-3.3.0.Final.jar
@ceki
Copy link
Member

ceki commented Aug 14, 2024

@thomas-k-git Can you please provide a test case reproducing the problem?

@thomas-k-git
Copy link
Author

slf4j-fcqn-issue-repro.zip
actual output when I run it:

ago 14, 2024 3:11:43 P. M. org.apache.logging.slf4j.SLF4JLogger logMessage
INFORMACIÓN: this should be logged with org.example.Main, but isnt

@ceki
Copy link
Member

ceki commented Aug 14, 2024

Why is org.apache.logging.log4j:log4j-to-slf4j:2.20.0 included in the class path since you are not using log4j 2.0?

@thomas-k-git
Copy link
Author

the reality for us here across multiple libraries in one complex application is that many log frameworks are used.
in our app we have code that uses all 3: jboss, slf4j, log4j (list goes on..). imagine the log APIs of all being used / called

@ceki
Copy link
Member

ceki commented Aug 14, 2024

So there are 3 different indirections and you would like to preserve the caller path. By the way, this is probably a org.apache.logging.log4j:log4j-to-slf4j:2.20.0 issue but I might me wrong.

@thomas-k-git
Copy link
Author

So there are 3 different indirections and you would like to preserve the caller path.
correct.

probably a org.apache.logging.log4j:log4j-to-slf4j:2.20.0 issue
I don't see how log4j-to-slf4j could have use the org.slf4j.spi.LocationAwareLogger#log API differently. The intention is to use the fqcn to find the log framework border, no?

Not sure what the exact contract/agreement of the method is, but how else could we implement getting the correct caller in such complex call stacks except using the parameter in this way?

@thomas-k-git
Copy link
Author

a simpler, maybe more general reproducer, the problem also exists (i think because of the same reason / treatment of fqcn) when just using jboss -> slf4j bridge.
here:

with this output:
`15.35.40: Executing ':org.example.Main.main()'...

Task :compileJava
Task :processResources NO-SOURCE
Task :classes

Task :org.example.Main.main()
ago 14, 2024 3:35:42 P. M. jdk.internal.reflect.DirectMethodHandleAccessor invoke
INFORMACIÓN: this should be logged with org.example.Main, but isnt
`

note the "jdk.internal.reflect.DirectMethodHandleAccessor invoke"

slf4j-fcqn-issue-repro2-direct.zip

@ppkarwasz
Copy link
Contributor

The problem is here:

static String[] BARRIER_CLASSES = new String[] { SUPER_OF_SUPER, SUPER, SELF, SUBSTITUE, FLUENT };
private boolean barrierMatch(String callerFQCN, String candidateClassName) {
if (candidateClassName.equals(callerFQCN))
return true;
for (String barrierClassName : BARRIER_CLASSES) {
if (barrierClassName.equals(candidateClassName)) {
return true;
}
}
return false;
}

This method is called to decide if candidateClassName is the entry point of the logging system. It is called in two different situations:

  1. When LocationAwareLogger is called. In this case callerFQCN matches the parameter passed by JBoss Logging and the BARRIER_CLASSES loop should be skipped.
  2. When any other Logger or LoggingEventBuilder method is called. In this case the callerFQCN has a fixed value of org.slf4j.jul.JDK14LoggerAdapter and the loop is necessary.

@thomas-k-git
Copy link
Author

@ceki what do you think? is there consensus that this should be changed?

@ceki
Copy link
Member

ceki commented Aug 28, 2024

Created https://jira.qos.ch/browse/SLF4J-604 to track this issue

@thomas-k-git
Copy link
Author

Hi all!
Any update on the issue? anything that I can do to assist? cheers and thx

@ceki
Copy link
Member

ceki commented Oct 21, 2024

@thomas-k-git Fixed in commit 69c333d

In JDK14LoggerAdapter, the logic searching for the stack trace line just before actual the caller was adapted to take into account an extra logging layer such as Jboss logging.

Other logging adapters/backends such as slf4j-relaod4j/reload4j and logback already had the correct behavior.

@ceki ceki self-assigned this Oct 21, 2024
@ceki ceki added the DONE for fixed issues label Oct 21, 2024
@ceki ceki added this to the 2.0.17 milestone Oct 21, 2024
@ceki
Copy link
Member

ceki commented Oct 21, 2024

@ppkarwasz Please let me know if commit 69c333d works for you.

@ppkarwasz
Copy link
Contributor

@ceki,

It works, but it looks a little bit over-engineered to me.
The callerBoundary can IMHO be precisely determined and barrierMatch can be reduced to:

 private boolean barrierMatch(String callerFQCN, String candidateClassName) { 
     return candidateClassName.equals(callerFQCN)); 
 }

The callerBoundary depends on the type of API used:

  1. For the classic API, the boundary is AbstractLogger (not JDK14LoggerAdapter as the current code sets). All the trace, debug, etc. methods are defined in AbstractLogger: LegacyAbstractLogger and JDK14LoggerAdapter will never be the caller boundary.
  2. For the fluent API, it is up to the LoggingEventBuilder to set the correct boundary and DefaultLoggingEventBuilder does it correctly.
  3. For the LoggingEventAware API the caller should set the caller boundary and JDK14LoggerAdapter should use it (currently it does not use that information).
  4. For the SubstituteLogger, I believe that it is up to the SubstituteLogger to set itself as caller boundary, not up to the SLF4J implementations to be aware of SubstituteLogger.

I can probably make a PR that simplifies the location logic by the end of the week.

@thomas-k-git
Copy link
Author

Is there a pre-release build somewhere to test?
had troubles setting up / running maven locally on the repo :/
does it fix both reproducers attached here https://github.com/user-attachments/files/16613965/slf4j-fcqn-issue-repro2-direct.zip and further up?

@ceki
Copy link
Member

ceki commented Oct 28, 2024

@thomas-k-git It should be very easy to build slf4j. JDK 11 and Maven should be enough to build. What is failing?

ppkarwasz added a commit to ppkarwasz/slf4j that referenced this issue Oct 28, 2024
The `JDK14LoggerAdapter` class uses the wrong caller boundary for location unaware methods.

This PR:

- Sets the correct caller boundary for classic API method calls (`AbstractLogger`),
- Adds the missing `LoggingEventAware` interface and fixes its implementation. The interface was previously implemented, but not declared.
- Adds a test for qos-ch#425.

The `SubstituteLogger` tests depend on qos-ch#438.
ppkarwasz added a commit to ppkarwasz/slf4j that referenced this issue Oct 28, 2024
The `JDK14LoggerAdapter` class uses the wrong caller boundary for location unaware methods.

This PR:

- Sets the correct caller boundary for classic API method calls (`AbstractLogger`),
- Adds the missing `LoggingEventAware` interface and fixes its implementation. The interface was previously implemented, but not declared.
- Adds a test for qos-ch#425.

The `SubstituteLogger` tests depend on qos-ch#438.

Signed-off-by: Piotr P. Karwasz <[email protected]>
@thomas-k-git
Copy link
Author

thomas-k-git commented Oct 29, 2024

Ah ok, works now with mvn install. mvn compile didn't work at first. for info, error was:

INFO] Changes detected - recompiling the module!
[INFO] Compiling 1 source file to /mnt/data/workspaces/slf4j/slf4j-simple/target/classes/META-INF/versions/9
...
[INFO] -------------------------------------------------------------
[ERROR] COMPILATION ERROR :
[INFO] -------------------------------------------------------------
[ERROR] /mnt/data/workspaces/slf4j/slf4j-simple/src/main/java9/module-info.java:[2,15] module not found: org.slf4j
...
[INFO] SLF4J Simple Provider .............................. FAILURE [ 0.327 s]
...
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.10.1:compile (module-compile) on project > slf4j-simple: Compilation failure
[ERROR] /mnt/data/workspaces/slf4j/slf4j-simple/src/main/java9/module-info.java:[2,15] module not found: org.slf4j

@thomas-k-git
Copy link
Author

Just did local tests with the current master with the suggested fix: Fixes both reproducers and our actual prodution setup, which has a bit more stuff added on top!

@ppkarwasz I also tested the changes on your branch. Same: fixes both reproducers and our location production setup.

@thomas-k-git
Copy link
Author

Hi all! do we have a decision regarding which approach we want to use, the one here or in #439 ?

@thomas-k-git
Copy link
Author

Hi! Are there any plans to release 2.0.17 soon? I didn't find any periodic schedule. cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DONE for fixed issues
Projects
None yet
Development

No branches or pull requests

3 participants