Ralph, could you show how those two users can use a `MessageFactory` to create `Logger`s with predefined additional context data?
On Thu, Mar 21, 2024 at 7:25 AM Ralph Goers <rgo...@apache.org> wrote: > Unfortunately this is another message I somehow didn't get in my inbox. > Replying to it via lists.a.o is not a great experience but is the best I > can do. > > On 2024/03/20 13:51:56 Volkan Yazıcı wrote: > > I agree with the way Piotr dissects the problem. I think `ScopedContext`, > > even though it has its own merits, doesn't address the problem reported > by > > users. They simply want a new logger associated with some additional > > context data. > > Two users do. I have personally been asked for something like > ScopedContext several times. > As I replied to Piotr, we already solved the problem of adding data to > Loggers. That is what MessageFactories are intended for. > > > > > *[See my comments below.]* > > > > On Mon, Mar 18, 2024 at 10:40 AM Piotr P. Karwasz < > piotr.karw...@gmail.com> > > wrote: > > > > > * we can create a `Logger` wrapper "bound" to context data as Mikko > > > does. This wrapper will take care of setting the `ThreadContext` > > > before the logger call and restore it after it. > > > > Creating a wrapper `Logger` can work without needing to deal with > > `ThreadContext`. I can think of two different ways to carry this out: > > > > 1. Currently, `AbstractLogger` only creates `Message`s. We can rework > it > > to create `LogEvent`s too. Once `AbstractLogger` gets its hand on a > > `LogEvent`, it can enrich its context data as it wishes. > > 2. We can extend `ContextDataInjector` with a new `void > > injectContextData(Logger logger, StringMap target)` method, provide a > > `ContextDataInjector` implementation that branches on `logger > instanceof > > ContextDataProvider`, and call `ContextDataInjector` with the > associated > > `Logger` in `LogEventFactory`. > > We can do lots of things, most of which I wouldn't recommend. As to yours: > 1. Logger/AbstractLogger got very complex with Async, Garbage Free, > Reliablity Strategies, etc. Trying to move creating the LogEvent sooner is > likely to be a major PITA and could seriously impact performance. While we > could add a context map to AbstractLogger we would have to pass that on the > logging calls to LoggerConfig and deal with all that that means - remember, > a LoggerConfig can be handling multiple Loggers. > 2. I don't recommend extending ContextDataInjector. That proved difficult > to work with which is why we now recommend using ContextDataProviders. You > really can only have one ContextDataInjector. Also, please note that > ContextDataInjector is called while constructing the LogEvent. The LogEvent > isn't passed the Logger, only the LoggerName. Looking up the Logger to do > this is yet another way to slow down logging. > > > > > On Tue, Mar 19, 2024 at 7:45 AM Ralph Goers <ralph.go...@dslextreme.com> > > wrote: > > > In the meantime, I provided > > https://github.com/apache/logging-log4j2/pull/2385 which I very loosely > > modeled after ScopedValues. > > > > The fact that `ScopedContext` tries to imitate `ScopedValue` using > > `ThreadLocal`s is extremely confusing (from a user's pov) and risky > > liability (from a maintainer's pov). I guess you wanted to implement *a* > > `ScopedValue` without using *the* `ScopedValue` to be compatible with > Java > > 8. If so, that really sounds like the `o.a.l.log4j.util.Supplier` > downward > > spiral. We can rather have an *internal* `Log4jScopedValue` interface and > > provide Java 8 (using `InheritableThreadLocal`) and Java 21+ (using > > `ScopedValue`) compatible solutions in an MRJ (Multi-Release JAR). > > I am NOT trying to make ScopedContext compatible with ScopedValue. I am > trying to make it conceptually close enough to ScopedValue that users will > understand what it is doing. > We can argue about naming if you want. Gary has already expressed his > opinion. > > > > We can integrate `ScopedContext` to the `LogEventFactory` by providing a > > specialized `ContextDataInjector` plugin – assuming `LogEventFactory` > > employs all available `ContextDataInjector` plugins. > > ScopedContext is integrated with a ContextDataProvider, which is the > supported way to do this. Again, you cannot have more than one > ContextDataInjector so providing "specialized versions" is a pipe dream. > You will simply have to enhance the one we already have. > ContextDataInjector is NOT a plugin. > > > > > I find the current ceremony also too long: > > `ScopedContext.getCurrent().where("key1", "value1").run(...)`. I would > > rather aim for `ScopedContext.run(key, value, runnable)` and similar > > `ScopedContext.op(..., runnable)` interaction. > > Those are going to be provided as well. > > Ralph >
