Re: Context data propagation and logger-bound context data

[Matt Sicker <ma...@musigma.org>]

https://docs.micrometer.io/context-propagation/reference/

This is more in line with what I was thinking we should support. I’m not suggesting that we add Micrometer as a dependency to the API or Core, but we can replicate the minimal API here or figure out a smaller API for integration purposes.

Note that similar projects like the OpenTelemetry context propagation API is geared toward cross-process context propagation, not cross-thread or similar lower level primitives. The low level primitives belong in Log4j IMO.


> On Mar 27, 2024, at 16:56, Ralph Goers <ra...@dslextreme.com> wrote:
> 
> Volkan,
> 
> No more hand waving. Please see https://github.com/apache/logging-log4j2/pull/2419.
> 
> I should note that while implementing the classes I added to support this makes it easier I did not have to make any changes to the logging internals to make this work.
> 
> Ralph
> 
>> On Mar 22, 2024, at 1:45 AM, Volkan Yazıcı <vo...@yazi.ci> wrote:
>> 
>> No, it is not the same thing Matt. Let me be as explicit as I can:
>> 
>> var logger0 = getLogger();  // MDC: {}
>> var logger1 = logger0.withContextData("x", 1);  // MDC: {x: 1}
>> var logger2 = logger1.withContextData("y", 2);  // MDC: {x: 1, y: 2}
>> 
>> This is the functionality being requested. Whoever claims this can be done using a `MessageFactory`, they need to share a working [pseudo] code, instead of hand waving. So far, nobody responded to this. Piotr, speculated on a non-existing `Logger#withMessageFactory(MessageFactory)`, but there is not one single working example shared. Hence, unless you can prove me wrong with a working practical[1] example, the requested feature is currently known to be not practically possible in Log4j.
>> 
>> [1] Implementing `logger.withContextData("x", 1)` with 50 LoC Java code using the existing Log4j feature set is not a "practical example".
>> 
>> P.S. For Log4j 3 API Javadocs, you can browse to https://logging.apache.org/log4j/3.x and search for "Javadoc" in the menu. (Obviously, same works for Log4j 2 too.)
>> 
>> On Thu, Mar 21, 2024 at 6:10 PM Matt Sicker <ma...@musigma.org> wrote:
>> LogManager - log4j-api 3.0.0-alpha1 javadoc
>> javadoc.io
>> 
>> Pass your custom MessageFactory here. It’s an optional argument when creating the Logger.
>> 
>> Also, I’m not sure where to even find the current javadocs for the API. javadoc.io only seems to have this alpha release.
>> 
>> 
>>> On Mar 21, 2024, at 04:34, Volkan Yazıcı <vo...@yazi.ci> wrote:
>>> 
>>> 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 <rg...@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.karwasz@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 <ra...@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
>>>> 
>> 
> 

Re: Context data propagation and logger-bound context data

[Ralph Goers <ra...@dslextreme.com>]

In theory that library could use the ContextData class I just added to get context data no matter how it got there. However, the Micrometer library’s setValue and getValue set and retrieve the full map, not values. That won’t play nice with a ScopedContext since once you set it you would immediately be leaving the context so what you set would be instantly forgotten. Under the covers of course it could push a new map on the queue but that would likely behave very differently than the way ScopedContext does - i.e. you are completely replacing maps not incrementally modifying them.

Ralph

> On Apr 15, 2024, at 10:07 AM, Piotr P. Karwasz <pi...@gmail.com> wrote:
> 
> Hi,
> 
> On Mon, 15 Apr 2024 at 18:44, Matt Sicker <matt@musigma.org <ma...@musigma.org>> wrote:
>> Context Propagation support :: Micrometer Context Propagation
>> docs.micrometer.io
>> 
>>  <https://docs.micrometer.io/context-propagation/reference/>Context Propagation support :: Micrometer Context Propagation <https://docs.micrometer.io/context-propagation/reference/>
>> docs.micrometer.io <https://docs.micrometer.io/context-propagation/reference/>	 <https://docs.micrometer.io/context-propagation/reference/>
>> 
>> This is more in line with what I was thinking we should support. I’m not suggesting that we add Micrometer as a dependency to the API or Core, but we can replicate the minimal API here or figure out a smaller API for integration purposes.
> 
> I agree. To support `context-propagation` we don't need to add anything to the API or SPI. To support it efficiently, we need the two methods from PR [1].
> 
> Note that SLF4J is already supported by `context-propagation` (cf. [2]).
> 
> Piotr
> 
> [1] https://github.com/apache/logging-log4j2/pull/2442
> [2] https://github.com/micrometer-metrics/context-propagation/pull/194

Re: Context data propagation and logger-bound context data

["Piotr P. Karwasz" <pi...@gmail.com>]

Hi,

On Mon, 15 Apr 2024 at 18:44, Matt Sicker <ma...@musigma.org> wrote:

> Context Propagation support :: Micrometer Context Propagation
> <https://docs.micrometer.io/context-propagation/reference/>
> docs.micrometer.io
> <https://docs.micrometer.io/context-propagation/reference/>
> [image: favicon.ico]
> <https://docs.micrometer.io/context-propagation/reference/>
> <https://docs.micrometer.io/context-propagation/reference/>
>
> This is more in line with what I was thinking we should support. I’m not
> suggesting that we add Micrometer as a dependency to the API or Core, but
> we can replicate the minimal API here or figure out a smaller API for
> integration purposes.
>

I agree. To support `context-propagation` we don't need to add anything to
the API or SPI. To support it efficiently, we need the two methods from PR
[1].

Note that SLF4J is already supported by `context-propagation` (cf. [2]).

Piotr

[1] https://github.com/apache/logging-log4j2/pull/2442
[2] https://github.com/micrometer-metrics/context-propagation/pull/194