2214 - Add support for ScopedContext

[rgoers]

This adds support for a ScopedContext as requested in apache/logging-log4j-kotlin#71 and similar to #2214

[ppkarwasz]

In this epoch of concurrent programming, I am not sure if introducing another API class based on static methods and ThreadLocal's is such a good idea.

At a first glance, I would probably prefer to see a wrapper like this:

public class ContextLogger implements Logger, CloseableThreadContext.Instance {
    private final Logger delegate;
    private final Map<String, String> contextData;
    ...
}

that would allow users to modify context data and log messages at the same time.

However this subject should be discussed on dev@logging and with many other Open Source projects. For example I believe that the first logging API that will be included into "advanced" Runnables such as CoreSubscriber or Kotlin's coroutines will have a very bright future.

[ppkarwasz]

I believe that ThreadContext and ScopedContext should be write-only.

The way I see it, these are logging-only services. The presence of get() methods allows user to use these services to store and retrieve their thread-dependent data, which is in my opinion and abuse of the API.

There are more proper ways to store data, e.g. in a servlet application the username should be stored in the ServletRequest attributes, not in ThreadContext.

[rgoers]

I have to disagree with this but here we are just expressing our own opinions. Your example would require passing the ServletRequest through many methods that have no business knowing what a ServletRequest is or that they are even running in a servlet, but do need to know who the user is. For example, I have plenty of code that is shared between REST requests and Kafka consumers. It is obviously very late to suggest that the ThreadContext be write only. FWIW, the enhancement going into SLF4J does something sort of similar to this but instead of having the scoped context completely separate there you still do MDC.get to fetch attributes from the scoped context.

[ppkarwasz]

Yes, I believe it is a matter of taste and commodity.

I am a little bit worried about users using ThreadContext for business-critical functionality, since a change of logging backend (e.g. from log4j-core to log4j-to-jul), might break that functionality. For example in #2374 I assumed that the ThreadContextMap implementation used by an implementation is strictly linked to the logger context implementation; since java.util.logging formatters don't support context data, log4j-to-jul might as well use no-op ThreadContextMap. Am I wrong?

[vy]

I agree with @ppkarwasz's stance here.

Your example would require passing the ServletRequest through many methods that have no business knowing what a ServletRequest is or that they are even running in a servlet, but do need to know who the user is.

I understand the need, but using the logging framework to figure out who the session belongs to doesn't sound right to me. If a business logic needs to access the user and it is not available at that level, it should not blame the logging for this.

[rgoers]

@ppkarwasz For some reason I cannot reply to your original comment directly. Oh, I see why. You replied to your original comment with a completely different comment.

At a first glance, I would probably prefer to see a wrapper like this:

public class ContextLogger implements Logger, CloseableThreadContext.Instance {
private final Logger delegate;
private final Map<String, String> contextData;
...
}

This I could not agree to. It is a completely different paradigm. The ScopedContext creates attributes to be logged for a block of code across all the Loggers that may be encountered. What you propose here is a context for a single Logger. While that may be useful it is not the same thing at all and serves a completely different purpose.

Unfortunately, your CoreSubscriber example is completely lost on me as I have never looked at that project and don't have any idea what the purpose of the class is.

[ppkarwasz]

I think we should discuss it in dev@logging. I started a thread dedicated to this subject.

[garydgregory]

Hi all,

The "where" API name is a terrible IMO. The Javadoc stays that the method "Adds ..." so why not call it that? The method delegates to an "add..." method. The parameter names makes it look like a map addition call so why not call it "put"?

[rgoers]

@garydgregory It is modeled after https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/ScopedValue.html. Obviously it is not the same API but it is similar enough that I would hope users would easily understand it.

[vy]

I have shared my remarks in the mailing list. In short, IMHO, in its current form, ScopedContext addresses neither #2214, nor apache/logging-log4j-kotlin#71, and its API, and consequently its API's integration with the rest of the system still needs some work.

[vy]

I agree with @ppkarwasz's stance here.

Your example would require passing the ServletRequest through many methods that have no business knowing what a ServletRequest is or that they are even running in a servlet, but do need to know who the user is.

I understand the need, but using the logging framework to figure out who the session belongs to doesn't sound right to me. If a business logic needs to access the user and it is not available at that level, it should not blame the logging for this.

[vy]

Please let's not introduce a yet another Object-to-String helper interface ecosystem. I think StringBuilderFormattable fits the bill here.

[rgoers]

With regard to the get() method. This API supports Objects - something users have been requesting for a long time. I added them here because they can be safely used the way this is implemented. There is no point in accepting an object if user's cannot query them since logging will NEVER log an object, only its string representation.

How? There are no StringBuilders at all in ScopedContext. It is used to convert an object into its String representation so it can be included in the LogEvent's ContextMap as a single String item. While the Object's render method certainly might use a StringBuilder internally the code storing the data into the Map certainly doesn't want one. Furthermore, if you look at the default implementation the render method simply calls toString() on the object. For the 90+% of use cases where the object is already a String this is effectively a no-op as it just returns "this". Using a StringBuilder would require much more overhead.

[vy]

Umm... I am strongly against this. ScopedValues have landed to JVM – we should use them instead. Opting for ThreadLocals is a very risky liability and recipe for failure. I will detail this out in my review.

[rgoers]

1. This is NOT meant for only Java 21 and above. It is targeted at Log4j API which still supports Java 8. Note that it could be possible to provide a different implementation for Java 21 if it makes sense to do so.
2. ThreadLocals are NOT risky even with Virtual Threads when they are used in this way. I specifically looked for a mention of being able to disable ThreadLocals and could not find anything as I recall you previously mentioning that. However, they should NOT be used to cache objects as we have done in the past. Instead, "normal" object pooling should be used. I will also say that in my first incarnation I used a Map where the key was the threadId. But I changed that since it is nothing more than a slower version of a ThreadLocal.

As an aside, I looked at the ScopedValue javadoc and some examples of how to use it. I am not overly impressed. To be honest I would prefer ScopedContext since it is quite a bit simpler and shouldl be more powerful once I have finished.