On Sun, Jul 10, 2022 at 4:58 PM Ralph Goers <[email protected]> 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 agree fully that it would be ideal to have this be a compile-time transformation. Gary > > 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ı <[email protected]> wrote: > > > > Why do you prefer `withLocation()` compared to the thread-local approach? > > > > On Sun, Jul 10, 2022 at 9:55 PM Ralph Goers <[email protected]> > > 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ı <[email protected]> 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 <[email protected] > >>> > >>> wrote: > >>> > >>>> Hi Volkan, > >>>> > >>>> On Sat, 9 Jul 2022 at 12:05, Volkan Yazıcı <[email protected]> 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 > >>>> > >> > >> >
