It looks like the location only ever gets the file name, because that's what StackTraceElement gives. So that's fine.
I think I'm ready for a formal review. Should I create pull requests on github for https://github.com/shawjef3/logging-log4j2/tree/message-location and https://github.com/shawjef3/logging-log4j-scala/tree/message-location, or is there another process? On Wed, Dec 20, 2017 at 2:11 PM, Matt Sicker <[email protected]> wrote: > I think that should be configurable in the layout options. > > On 20 December 2017 at 13:04, Dominik Psenner <[email protected]> wrote: > > > Could a compile time environment variable like SrcRootDirectory do the > job? > > > > On 20 Dec 2017 7:49 p.m., "Jeffrey Shaw" <[email protected]> wrote: > > > > > I got it working using a custom ExtendedLogger instead of mocking. > > > > > > It looks like for file name, there are only two options. We can have > the > > > file name, or the full path to the file. The path relative to the > project > > > root looks impossible to get, unless we look for magic names like > "src". > > > Any opinion as to which to use? I'm tempted to use just the file name, > > > since the full path to the file seems intrusive, and is dependent on > the > > > build machine. > > > > > > On Wed, Dec 20, 2017 at 11:35 AM, Matt Sicker <[email protected]> > wrote: > > > > > > > It's possible that macros and mocks don't work well together, though > > > that's > > > > just a guess. > > > > > > > > On 20 December 2017 at 00:00, Jeff Shaw <[email protected]> wrote: > > > > > > > > > I should add that manually testing it works. > > > > > > > > > > Sent from my phone > > > > > > > > > > > On Dec 20, 2017, at 12:45 AM, Jeffrey Shaw <[email protected]> > > > wrote: > > > > > > > > > > > > I added some tests for traced, but they don't pass. The mocks > say, > > > > > "Actually, there were zero interactions with this mock." I could > use > > > some > > > > > help getting these two tests to work. > > > > > > > > > > > > https://github.com/shawjef3/logging-log4j-scala/blob/ > > > > > message-location/log4j-api-scala_2.12/src/test/scala/org/ > > > > > apache/logging/log4j/scala/LoggerTest.scala#L574 > > > > > > > > > > > >> On Sun, Dec 17, 2017 at 8:50 PM, Jeffrey Shaw < > [email protected] > > > > > > > > wrote: > > > > > >> Thanks for the encouragement everyone! I never worked on an > Apache > > > > > project before and had no idea what to expect from the community. > > > > > >> > > > > > >> I've made some progress. One cool thing I added was a `traced` > > > method > > > > > (source), which does the work you'd want for traceEntry, traceExit, > > and > > > > > throwing. It would be cool to add catching as well, but that would > > > > require > > > > > tree traversal, which is beyond me at the moment. I also haven't > > > figured > > > > > out how to add the parameter lists. Anyway, an example: > > > > > >> > > > > > >> before: > > > > > >> def f() = {...} > > > > > >> > > > > > >> after: > > > > > >> def f() = logger.traced(Level.INFO) {...} > > > > > >> > > > > > >>> On Mon, Dec 11, 2017 at 9:55 PM, Matt Sicker <[email protected] > > > > > > wrote: > > > > > >>> From the little I've worked with macros (worked more with > > scalameta > > > > and > > > > > >>> shapeless), that looks good so far. If you can add some unit > > tests, > > > > > then > > > > > >>> we'd be happy to merge! > > > > > >>> > > > > > >>> On 11 December 2017 at 20:41, Jeffrey Shaw <[email protected] > > > > > > wrote: > > > > > >>> > > > > > >>> > Great news! I'm able to run LoggingApp in the scala api repo > > > > without > > > > > it > > > > > >>> > calling StackLocatorUtil.calcLocation, but it prints the same > > > > > messages as > > > > > >>> > before. I have to use my patch to log4j of course. > > > > > >>> > > > > > > >>> > See https://github.com/shawjef3/logging-log4j-scala/commits/ > > > > > >>> > message-location > > > > > >>> > > > > > > >>> > On Sun, Dec 10, 2017 at 3:56 PM, Matt Sicker < > [email protected] > > > > > > > > wrote: > > > > > >>> > > > > > > >>> > > This sounds like it'd make a great addition to the Scala > API! > > > > > >>> > > > > > > > >>> > > On 9 December 2017 at 15:36, Jeffrey Shaw < > > [email protected]> > > > > > wrote: > > > > > >>> > > > > > > > >>> > > > Ralph, I agree with you entirely. My intent for these new > > log > > > > > methods > > > > > >>> > is > > > > > >>> > > > that they only be called from compile-time generated > code. > > > > > >>> > > > > > > > > >>> > > > On Sat, Dec 9, 2017 at 4:30 PM, Ralph Goers < > > > > > >>> > [email protected]> > > > > > >>> > > > wrote: > > > > > >>> > > > > > > > > >>> > > > > I don’t understand how this is a good idea. To use this > > you > > > > > would > > > > > >>> > need > > > > > >>> > > to > > > > > >>> > > > > do something like: > > > > > >>> > > > > > > > > > >>> > > > > Message msg = new StringMessage(getCaller(), “My > > Message”); > > > > > >>> > > > > logger.debug(msg); > > > > > >>> > > > > > > > > > >>> > > > > Unfortunately the line number would point to the line > > where > > > > > >>> > getCaller() > > > > > >>> > > > is > > > > > >>> > > > > called not to the logger statement. > > > > > >>> > > > > > > > > > >>> > > > > I had thought about modifying AbstractLogger to do > > > > > >>> > > > > public void debug(final Marker marker, final String > > > message) > > > > { > > > > > >>> > > > > logIfEnabled(getCaller(), Level.DEBUG, marker, > > message, > > > > > >>> > (Throwable) > > > > > >>> > > > > null); > > > > > >>> > > > > } > > > > > >>> > > > > instead of the current > > > > > >>> > > > > public void debug(final Marker marker, final String > > > message) > > > > { > > > > > >>> > > > > logIfEnabled(FQCN, Level.DEBUG, marker, message, > > > > > (Throwable) > > > > > >>> > null); > > > > > >>> > > > > } > > > > > >>> > > > > But the amount of changes required to get it into the > > > > LogEvent > > > > > was > > > > > >>> > > large. > > > > > >>> > > > > OTOH, if we create a CallerLocationMessage that > contains > > > the > > > > > >>> > > > > StackTraceElement and then have existing Messages > extend > > > that > > > > > then we > > > > > >>> > > > could > > > > > >>> > > > > store the location in the Message if it is a > > > > > CallerLocationMessage. > > > > > >>> > > > Calling > > > > > >>> > > > > getCaller() in this way would be much better since it > is > > > at a > > > > > fixed > > > > > >>> > > depth > > > > > >>> > > > > from the caller. > > > > > >>> > > > > > > > > > >>> > > > > With Java 9 this could become: > > > > > >>> > > > > public void debug(final Marker marker, final String > > > message) > > > > { > > > > > >>> > > > > logIfEnabled(stackWalker.walk( > > > s->s.skip(1).findFirst(), > > > > > >>> > > Level.DEBUG, > > > > > >>> > > > > marker, message, (Throwable) null); > > > > > >>> > > > > } > > > > > >>> > > > > although that would pass a StackFrame instead of a > > > > > StackTraceElement. > > > > > >>> > > The > > > > > >>> > > > > only problems with this is that there is still some > > > overhead > > > > in > > > > > >>> > calling > > > > > >>> > > > > StackWalker like this. Also, if this is called from a > > > facade, > > > > > such as > > > > > >>> > > > > log4j-slf4j-impl then the number of levels that have to > > be > > > > > skipped > > > > > >>> > > would > > > > > >>> > > > be > > > > > >>> > > > > different. > > > > > >>> > > > > > > > > > >>> > > > > I would really prefer if there was some way to capture > > the > > > > line > > > > > >>> > number > > > > > >>> > > > > information for the various loggers when the annotation > > > > > processor > > > > > >>> > runs > > > > > >>> > > at > > > > > >>> > > > > compile time. > > > > > >>> > > > > > > > > > >>> > > > > Ralph > > > > > >>> > > > > > > > > > >>> > > > > > On Dec 9, 2017, at 1:22 PM, Jeffrey Shaw < > > > > [email protected] > > > > > > > > > > > >>> > wrote: > > > > > >>> > > > > > > > > > > >>> > > > > > Thanks for the link, Mikael. I'll take a look at it. > > > > > >>> > > > > > > > > > > >>> > > > > > I added some plumbing to core to allow clients to > pass > > a > > > > > >>> > > > > StackTraceElement > > > > > >>> > > > > > to loggers. I'd like a code review. I'm happy to try > > > other > > > > > methods. > > > > > >>> > > See > > > > > >>> > > > > the > > > > > >>> > > > > > following commit. > > > > > >>> > > > > > https://github.com/shawjef3/logging-log4j2/commit/ > > > > > >>> > > > > 9c42073e9ca4f25a2f4075b1791eee2893534c54 > > > > > >>> > > > > > > > > > > >>> > > > > > On Sat, Dec 9, 2017 at 10:09 AM, Mikael Ståldal < > > > > > [email protected]> > > > > > >>> > > > > wrote: > > > > > >>> > > > > > > > > > > >>> > > > > >> Have you tried the Log4j Scala API? > > > > > >>> > > > > >> > > > > > >>> > > > > >> http://logging.apache.org/ > log4j/2.x/manual/scala-api. > > > html > > > > > >>> > > > > >> > > > > > >>> > > > > >> It does currently not support this, but it uses > Scala > > > > > macros, and > > > > > >>> > > this > > > > > >>> > > > > >> could be added there. But log4j-api and/or > log4j-core > > > > > probably > > > > > >>> > needs > > > > > >>> > > > to > > > > > >>> > > > > >> adapted as well. > > > > > >>> > > > > >> > > > > > >>> > > > > >> > > > > > >>> > > > > >> > > > > > >>> > > > > >> On 2017-12-09 07:30, Jeffrey Shaw wrote: > > > > > >>> > > > > >> > > > > > >>> > > > > >>> Hello, > > > > > >>> > > > > >>> I've found that I am able to use Scala macros to > > > provide > > > > > >>> > > compile-time > > > > > >>> > > > > >>> source information for log messages. However, I > don't > > > see > > > > > a way > > > > > >>> > to > > > > > >>> > > > > inject > > > > > >>> > > > > >>> this into log4j's logging mechanism. > > > > > >>> > > > > >>> > > > > > >>> > > > > >>> I'm wondering if there is something I'm missing, or > > if > > > > > LogEvent's > > > > > >>> > > > > >>> getSource > > > > > >>> > > > > >>> method could be duplicated in Message. > > > > > >>> > > > > >>> > > > > > >>> > > > > >>> We could then have zero-overhead location > information > > > in > > > > > logs. > > > > > >>> > I'm > > > > > >>> > > > > >>> thinking > > > > > >>> > > > > >>> that tools other than Scala could also take > advantage > > > of > > > > > this. > > > > > >>> > > > > >>> > > > > > >>> > > > > >> > > > > > >>> > > > > > > > > > >>> > > > > > > > > > >>> > > > > > > > > >>> > > > > > > > >>> > > > > > > > >>> > > > > > > > >>> > > -- > > > > > >>> > > Matt Sicker <[email protected]> > > > > > >>> > > > > > > > >>> > > > > > > >>> > > > > > >>> > > > > > >>> > > > > > >>> -- > > > > > >>> Matt Sicker <[email protected]> > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > Matt Sicker <[email protected]> > > > > > > > > > > > > > -- > Matt Sicker <[email protected]> >
