-
-
Notifications
You must be signed in to change notification settings - Fork 997
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
Comments
@thomas-k-git Can you please provide a test case reproducing the problem? |
slf4j-fcqn-issue-repro.zip ago 14, 2024 3:11:43 P. M. org.apache.logging.slf4j.SLF4JLogger logMessage |
Why is |
the reality for us here across multiple libraries in one complex application is that many log frameworks are used. |
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. |
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? |
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. with this output:
note the "jdk.internal.reflect.DirectMethodHandleAccessor invoke" |
The problem is here: slf4j/slf4j-jdk14/src/main/java/org/slf4j/jul/JDK14LoggerAdapter.java Lines 219 to 230 in 4fa92fc
This method is called to decide if
|
@ceki what do you think? is there consensus that this should be changed? |
Created https://jira.qos.ch/browse/SLF4J-604 to track this issue |
Hi all! |
@thomas-k-git Fixed in commit 69c333d In Other logging adapters/backends such as slf4j-relaod4j/reload4j and logback already had the correct behavior. |
@ppkarwasz Please let me know if commit 69c333d works for you. |
It works, but it looks a little bit over-engineered to me.
The
I can probably make a PR that simplifies the location logic by the end of the week. |
Is there a pre-release build somewhere to test? |
@thomas-k-git It should be very easy to build slf4j. JDK 11 and Maven should be enough to build. What is failing? |
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.
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]>
Ah ok, works now with mvn install. mvn compile didn't work at first. for info, error was:
|
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. |
Hi all! do we have a decision regarding which approach we want to use, the one here or in #439 ? |
Hi! Are there any plans to release 2.0.17 soon? I didn't find any periodic schedule. cheers! |
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:
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:
The text was updated successfully, but these errors were encountered: