-1 from me for a Log4j functionality depending on Lombok. On Sun, Jul 10, 2022 at 11:17 PM Ralph Goers <ralph.go...@dslextreme.com> wrote:
> https://www.baeldung.com/lombok-custom-annotation seems to imply that we > could > add this support by extending Lombok. > > Ralph > > > On Jul 10, 2022, at 1:58 PM, Ralph Goers <ralph.go...@dslextreme.com> > wrote: > > > > Because I don’t see how it can work. > > > > A ThreadLocal is a run time construct. If you are saving location > information in > > a ThreadLocal then it is only good for the life of the request and/or > thread. But > > the location information will never change. It is fixed even before the > Class is loaded. > > So I simply don’t see the point of using a ThreadLocal. > > > > Furthermore, I am ONLY interested in injecting location information at > compile > > time. That means there will always be zero run time cost. Using an agent > or some > > other technique requires modifying the class as it is loaded or other > runtime > > manipulations. > > > > I’ve looked at the HandleLog method in Lombok. It actually isn’t very > complicated. > > It uses JCTree and the util package from javac and adds the logger field > directly > > to the class the compiler is constructing. From what I can tell it > should also be > > possible to scan the source and modify the appropriate lines in the same > way, > > but it would require a POC. > > > > Ralph > > > >> On Jul 10, 2022, at 1:09 PM, Volkan Yazıcı <vol...@yazi.ci> wrote: > >> > >> Why do you prefer `withLocation()` compared to the thread-local > approach? > >> > >> On Sun, Jul 10, 2022 at 9:55 PM Ralph Goers <ralph.go...@dslextreme.com > > > >> wrote: > >> > >>> This is exactly why I believe we should only support LogBuilder as it > >>> already > >>> has withLocation(). I see no point in adding the parameter to all the > >>> non-fluent > >>> variations. > >>> > >>> To work, the location must be passed as a parameter on the logging API > >>> call. > >>> Thus it won’t work for Log4j 1 or SLF4J. > >>> > >>> Ralph > >>> > >>>> On Jul 10, 2022, at 12:47 PM, Volkan Yazıcı <vol...@yazi.ci> wrote: > >>>> > >>>> Indeed supporting both static and dynamic weaving seems like the ideal > >>>> approach. (SPI-Fly is an interesting one. My OSGi illiteracy blocked > me > >>>> from wrapping my mind around all of its details. Nevertheless, I > think I > >>>> get the gist of it.) For both functionalities, we need to receive a > >>> package > >>>> list to scan for, right? > >>>> > >>>> Translating logger calls to the ones that receive the source location > >>>> information as arguments is also a valid direction. Though note that > this > >>>> requires doubling the size of the API surface, AFAIC. That is, for > every > >>>> `info(String)`, we need to introduce `info(String, SourceLocation)`, > etc. > >>>> Hence, I am inclined to go this route unless I am missing something. > >>>> > >>>> Piotr, what is your take on my claim that this optimization won't work > >>> for > >>>> bridges (SLF4J, log4j-1.2-api, etc.)? > >>>> > >>>> On Sat, Jul 9, 2022 at 5:02 PM Piotr P. Karwasz < > piotr.karw...@gmail.com > >>>> > >>>> wrote: > >>>> > >>>>> Hi Volkan, > >>>>> > >>>>> On Sat, 9 Jul 2022 at 12:05, Volkan Yazıcı <vol...@yazi.ci> wrote: > >>>>>> I think we can extend this experiment to implement zero-cost source > >>>>>> location capture for Log4j. Though I will appreciate your help on > some > >>>>>> loose ends. Assuming we have a bullet-proof mechanism to inline > source > >>>>>> location capture given a class, what is the right way to ship this? > As > >>> a > >>>>>> Maven plugin that kicks in at compile time? Wouldn't that make this > >>>>> feature > >>>>>> impossible to use without recompiling user sources? As a runtime > >>> utility? > >>>>>> If so, what about the cost of classpath scanning & weaving? If the > >>>>> bytecode > >>>>>> weaving only intercepts at Log4j API calls, this won't work for > Log4j 1 > >>>>>> bridge, SLF4J, or any other indirect access to the Log4j API. What > do > >>> you > >>>>>> think? I have used a thread-local to pass the source location to the > >>>>>> caller, is there a better alternative? (Putting aside the > >>> dynamic-scoped > >>>>>> variables to be shipped with Loom.) > >>>>> > >>>>> Great idea. I think that we can provide both a static and dynamic > >>>>> weaver from the same code (cf. SPI-Fly: > >>>>> https://github.com/apache/aries/tree/trunk/spi-fly). Developers > would > >>>>> be advised to statically weave their artifacts, while system > >>>>> administrators could do it during runtime. > >>>>> > >>>>> The usage of a `ThreadLocal` seems Ok to me. Alternatively we could > >>>>> add some parameters to the `Logger.log` calls, but this would mean 4 > >>>>> additional parameters on each simple call and we'll end up using the > >>>>> `Logger.log` method with an Object array. > >>>>> > >>>>> Piotr > >>>>> > >>> > >>> > > > >
