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
