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

KLogger obscures location info #11

Closed
dhutchis opened this issue Oct 11, 2016 · 9 comments
Closed

KLogger obscures location info #11

dhutchis opened this issue Oct 11, 2016 · 9 comments

Comments

@dhutchis
Copy link

I am using log4j with a patternlayout that includes class, method, and line number information. I expect log statements to print in the form below. This form holds when I manually set val logger: Logger = LoggerFactory.getLogger(this.javaClass).

INFO TEST_CONFIG.<init>(80) - test log

Instead, when I extend a companion object (or any singleton object) with object : KLogging(), the following unhelpful information prints

INFO KLogger.info(?) - test log

The code responsible for picking the location info is here in LocationInfo. The code sees that KLogger is the immediately logging class on the stack trace and uses KLogger for location info. It should instead use the class immediately below KLogger in the stack trace.

I'm not sure how to fix it, but there should be some hook in the log4j (ideally, slf4j) library that allows manipulating location info.

@dhutchis
Copy link
Author

I see where the endpoint logging class is set in slf4j: Log4JLoggerAdapter. It seems that changing the FQCN there would be difficult while staying agnostic to the particular logging framework.

For now I will opt to handle logging without the KLogging library. Good idea though-- the library came very close to achieving a standardized elegance in Kotlin logging.

@oshai
Copy link
Owner

oshai commented Oct 12, 2016

Thanks for raising the issue and the detailed explanation. I will try to
find a solution for that.

On Oct 12, 2016 1:26 AM, "Dylan Hutchison" [email protected] wrote:

I see where the endpoint logging class is set in slf4j: Log4JLoggerAdapter
https://github.com/qos-ch/slf4j/blob/master/slf4j-log4j12/src/main/java/org/slf4j/impl/Log4jLoggerAdapter.java#L70.
It seems that changing the FQCN there would be difficult while staying
agnostic to the particular logging framework.

For now I will opt to handle logging without the KLogging library. Good
idea though-- the library came very close to achieving a standardized
elegance in Kotlin logging.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#11 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACj0QXDWz96gr_UOH_EAgf8i_8eb8841ks5qzA0AgaJpZM4KUKLT
.

@oshai
Copy link
Owner

oshai commented Oct 12, 2016

I think I might be able to solve that. Do you have any test or small
project that you can share and reproduce the issue?

On Oct 12, 2016 8:08 AM, "ohad shai" [email protected] wrote:

Thanks for raising the issue and the detailed explanation. I will try to
find a solution for that.

On Oct 12, 2016 1:26 AM, "Dylan Hutchison" [email protected]
wrote:

I see where the endpoint logging class is set in slf4j:
Log4JLoggerAdapter
https://github.com/qos-ch/slf4j/blob/master/slf4j-log4j12/src/main/java/org/slf4j/impl/Log4jLoggerAdapter.java#L70.
It seems that changing the FQCN there would be difficult while staying
agnostic to the particular logging framework.

For now I will opt to handle logging without the KLogging library. Good
idea though-- the library came very close to achieving a standardized
elegance in Kotlin logging.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#11 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACj0QXDWz96gr_UOH_EAgf8i_8eb8841ks5qzA0AgaJpZM4KUKLT
.

@dhutchis
Copy link
Author

Sorry, I don't have a small project handy at the moment. It should be easy to recreate. Any project that uses slf4j-log4j will run into it. Here's a log4j.xml you can use

<?xml version="1.0" encoding="UTF-8" ?>
<!DOCTYPE log4j:configuration SYSTEM "http://logging.apache.org/log4j/1.2/apidocs/org/apache/log4j/xml/doc-files/log4j.dtd">
<log4j:configuration>
  <appender name="CONSOLE" class="org.apache.log4j.ConsoleAppender">
    <layout class="org.apache.log4j.PatternLayout">
      <param name="ConversionPattern"
             value="%d{ISO8601} %p %C{1}.%M(%L) - %m%n"/>
    </layout>
  </appender>

  <root>
    <level value="INFO"/>
    <appender-ref ref="CONSOLE"/>
  </root>

</log4j:configuration>

oshai added a commit that referenced this issue Oct 13, 2016
uncomment line 48 to make the test fail
@oshai
Copy link
Owner

oshai commented Oct 14, 2016

a possible solution is to do something similar to: https://github.com/qos-ch/slf4j/blob/master/slf4j-ext/src/main/java/org/slf4j/ext/LoggerWrapper.java
con's are: methods can't be inlined and all logger interface methods has to be delegated explicitly as opposed to current implementation that delegation is implicit.
inline methods might have advantage on android. see issue #1. a possible solution for that is to have two KLogger flavours with a configuration to select which one to use, but this sounds like interface clutter.

@dhutchis
Copy link
Author

Inlining is fine when the inline functions are extension methods on the
slf4j Logger. That's the solution I am currently using.

On Oct 14, 2016 5:18 AM, "oshai" [email protected] wrote:

a possible solution is to do something similar to:
https://github.com/qos-ch/slf4j/blob/master/slf4j-ext/
src/main/java/org/slf4j/ext/LoggerWrapper.java
con's are: methods can't be inlined and all logger interface methods has
to be delegated explicitly as opposed to current implementation that
delegation is implicit.
inline methods might have advantage on android. see issue #1
#1. a possible
solution for that is to have two KLogger flavours with a configuration to
select which one to use, but this sounds like interface clutter.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#11 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABm-diacwhRviO-DzZahmX1zzl_7pZyfks5qz3L0gaJpZM4KUKLT
.

@oshai
Copy link
Owner

oshai commented Oct 14, 2016

couple of notes:

  • from my checks inlined methods do not have line number information. If you can share a different example it will be great.
  • extension methods are a poor solution for that case in my experience: requires imports which are sometimes not obvious to users.

oshai added a commit that referenced this issue Oct 17, 2016
Changed KLogger to interface and added two implementations:
* LocationAwareKLogger that wraps LocationAwareLogger
* LocationIgnorantKLogger that wraps the rest of the loggers
Added a test for location awerness
* removed inline of methods - this is causing troubles in location
resolving and stacktraces and not possible on interface
@oshai
Copy link
Owner

oshai commented Oct 19, 2016

fixed in 1.4

@oshai oshai closed this as completed Oct 19, 2016
@dhutchis
Copy link
Author

Nice job! The inline function logging retained information for me, for some reason. Maybe the Kotlin compiler chose not to inline the functions marked inline. It makes sense that that information would be lost.

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

No branches or pull requests

2 participants