I’m not sure I would be comfortable applying the patch this way. Have you run a clirr report on your changes? I am concerned that this could break customizations that users might have made. What is the reason SourceLocation had to be used instead of StackTraceElement?
Ralph > On Dec 20, 2017, at 9:58 PM, Jeffrey Shaw <[email protected]> wrote: > > 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]> >>
