Add support for thread context map propagation #2442

ppkarwasz · 2024-04-04T10:42:17Z

Adds a new ExtendedThreadContext SPI class to help external libraries with an efficient propagation of the ThreadContextMap.

Currently integrators can retrieve and set the context map through a combination of ThreadContext#getImmutableContext, #clear and #putAll. This inevitable leads to the creation of temporary objects if the underlying implementation does not use the JDK Map class internally.

As a replacement we provide two saveMap and restoreMap methods that can be used to access the ThreadLocals directly. Due to the inherent unsafety of such operations, these methods are only available from the o.a.l.l.spi package.

We also reimplement CloseableThreadContext.Instance to take advantage of this SPI.

Adds a new `ExtendedThreadContext` SPI class to help external libraries with an efficient propagation of the `ThreadContextMap`. Currently integrators can retrieve and set the context map through a combination of `ThreadContext#getImmutableContext`, `#clear` and `#putAll`. This inevitable leads to the creation of temporary objects if the underlying implementation does not use the JDK `Map` class internally. As a replacement we provide two `saveMap` and `restoreMap` methods that can be used to access the `ThreadLocal`s directly. Due to the inherent unsafety of such operations, these methods are only available from the `o.a.l.l.spi` package. We also reimplement `CloseableThreadContext.Instance` to take advantage of this SPI.

rgoers

Son of a gun, I forgot to hit submit so none of my comments have been viewable.

To be clear, while I don't understand the purpose of this PR I am more disappointed with the mess ThreadContext (and ThreadContextMap in particular) has become with all these Map implementations, most of which are completely unnecessary simply by avoiding having to copy the map all the time as I have done in #2438.

rgoers · 2024-04-04T21:14:24Z

log4j-api/src/main/java/org/apache/logging/log4j/spi/DefaultThreadContextMap.java

+
+    @Override
+    public Object save() {
+        return localMap.get();


I don't really understand this. All of these ThreadContextMaps seem to do this same thing. It isn't a "save". All you are doing is getting a reference to the Map. In many of these cases the Map that was "saved" could change after the save operation since it is pointing to the same copy of the Map. Also, what is the value of "saving" a referece to the Map? I could understand if it was storing JSON as that could transmitted.

The DefaultThreadContextMap and CopyOnWriteSortedArrayThreadContextMap just return the underlying object, because they use immutable structures (an immutable map for the former and a frozen StringMap for the latter.

If you look at the implementation of GarbageFreeSortedArrayThreadContextMap it makes a copy of the map:

logging-log4j2/log4j-api/src/main/java/org/apache/logging/log4j/spi/GarbageFreeSortedArrayThreadContextMap.java

Lines 213 to 217 in bf9c703

@Override

public Object save() {

final StringMap map = localMap.get();

return map != null && !map.isEmpty() ? createStringMap(map) : null;

}

It would still be possible to make this garbage-free if we use a recycler for the copied map.

rgoers · 2024-04-04T21:34:12Z

log4j-api/src/main/java/org/apache/logging/log4j/spi/ExtendedThreadContext.java

+     * @see #restoreMap
+     */
+    public static Object saveMap() {
+        return getThreadContextMap().save();


This bothers me a LOT. Without having some idea of the saved format calling restore could end up being a disaster.

I am trying to figure out when, how, or why I would use this. I could imagine using it to send the ThreadContextMap to a downstream service but none of the implementations actually save anything useful for that. While this could be used to save the current state so I can add more entries and then just restore the map to its previous state, this doesn't actually provide that functionality. The caller would still have to invent some place to store the stack of maps, so you would probably still need an extra ThreadLocal to do that.

The main application I had in mind for this method is ThreadLocalAccessor#getValue used in Spring Reactor.

When working with the reactive stack (cf. Spring's documentation) the processing of a web request is represented as a pipeline of CoreSubscriber objects, where each subscriber processes the output of the previous stage. The onX methods may be called on any thread, so the context data is stored in the CoreSubscriber object. As far as I know they don't use ThreadLocal.

The context propagation offered by Micrometer is also supposed to work in other stacks. For example the Netty-based Spring Webflux WebClient is supposed to:

propagate the context data through Netty's outbound handlers,

store it in Netty's channel before the request hits the wire,

restore it when an answer is received,

propagate the context data through Netty's inbound handlers.

As far as I understand the propagation is restricted to the bounds of the JVM, so the opaque object in the context is not a problem. This object serves a single purpose: supply the object to the restoreMap method of the ThreadContextMap that created it.

I have a similar use case in https://github.com/spinnaker/kork/blob/master/kork-security/src/main/java/com/netflix/spinnaker/security/AuthenticatedRequestDecorator.java where thunks are decorated to propagate contexts. Having to clear out the whole map and copy data over seems error-prone. Runtimes that don't map directly to physical threads may involve swapping out the thread context data a lot (such as WebFlux or Ktor).

@jvz If I am not mistaken you should be able to use ScopedContext for this. However, I think I must misunderstand something. Your code does:

Copy the context map.

Clear the context map.

populate the context map using all the keys and values from the copy.

What exactly does this accomplish? I understand copying the map so you can restore it on the way out in case it was modified, but copying, clearing, and repopulating seems unnecessary.

FWIW, If you were using a ScopedContext anything added to the context would automatically be cleared when your run method exits.

In thinking about this I realize why I dislike this.

As much as you didn't like the get method of ScopedContext i really dislike adding these two methods to the ThreadContextMap interface. That is the public API and these methods are dangerous. I would also not expect the public API of an immutable map would let itself be replaced. I would suggest creating a separate interface and have the ThreadContextMap implementations implement that.

IMO save and restore are terrible names. They make it sound like the context is being serialized. Instead they should be getMap() and setMap() since that is what they explicitly do.

I just merged a commit that causes all ContextDataProviders to be treated equally. This change only applies to the ThreadContextMap. I would suggest that the interface you are creating should be equally applicable to any ContextDataProvider - if they implement the new interface. This could mean you could modify the ContextData class to have methods like

Map<String, ?> getContextMap(String provider); Map<String, Map<String, ?> getContextMaps(); void setContextMap(String provider, Map<String, ?> map); void setContextMaps(Map<String, Map<String, ?>> maps);

I don't think when saving and restoring you would want to merge the maps as when you "restore" you wouldn't get what you started with.

As much as you didn't like the get method of ScopedContext i really dislike adding these two methods to the ThreadContextMap interface. That is the public API and these methods are dangerous. I would also not expect the public API of an immutable map would let itself be replaced. I would suggest creating a separate interface and have the ThreadContextMap implementations implement that.

The ThreadContextMap is in the o.a.l.l.spi package, so it is not what I consider public API. Same applies to ExtendedThreadContext. This package already contains dangerous methods like LoggerContextFactory#shutdown, but I don't consider this a problem, because these are used by integrators that are supposed to know what they are doing.

IMO save and restore are terrible names. They make it sound like the context is being serialized. Instead they should be getMap() and setMap() since that is what they explicitly do.

I agree, the names are not great. What would you think about getContextData and setContextData?

I just merged a commit that causes all ContextDataProviders to be treated equally. This change only applies to the ThreadContextMap. I would suggest that the interface you are creating should be equally applicable to any ContextDataProvider - if they implement the new interface. This could mean you could modify the ContextData class to have methods like

Map<String, ?> getContextMap(String provider); Map<String, Map<String, ?> getContextMaps(); void setContextMap(String provider, Map<String, ?> map); void setContextMaps(Map<String, Map<String, ?>> maps);

I don't think when saving and restoring you would want to merge the maps as when you "restore" you wouldn't get what you started with.

I am not convinced this is a good idea. Users register custom ContextDataProviders to inject custom data into logs. They are not interested in Log4j propagating this data to other threads, because they can do it on their own. Look at log4j-context-data-2.17 for example: the propagation of the traceId and spanId is a core functionality of their library. They don't need Log4j Core to do it for them.

That is why we only need to provide integration points to propagate native Log4j sources. Even MDCContextMap should probably have no-op save and restore methods. This is also why I am opposed to having two thread locals that users need to propagate.

ppkarwasz · 2024-04-12T08:35:32Z

Son of a gun, I forgot to hit submit so none of my comments have been viewable.

It happens to me all the time or even I hit it, but a network problem prevents the comment from being posted.

To be clear, while I don't understand the purpose of this PR I am more disappointed with the mess ThreadContext (and ThreadContextMap in particular) has become with all these Map implementations, most of which are completely unnecessary simply by avoiding having to copy the map all the time as I have done in #2438.

I agree the ThreadContextMap is a mess, with unnecessary abstractions (like StringMap), whose only purpose is to "give Java 7 users the Java 8 Map#forEach method today).

ppkarwasz · 2024-04-12T09:05:01Z

I realized now that in this PR I assume that the Object returned by saveMap will be used only once in a restoreMap call. This assumption might be too restrictive for context propagation.

src/changelog/.2.x.x/add_extended_thread_context.xml

Co-authored-by: Volkan Yazıcı <[email protected]>

…ation

ppkarwasz · 2024-04-16T09:32:16Z

@remkop,

Is there any usage for context propagation in the "garbage-free world"? My guess is that switching threads using Reactive streams or similar provides too much garbage for it to be a viable option.

If that is the case the existing ThreadContext.getImmutableContext might be enough for most users and this PR is not needed.

remkop · 2024-04-16T10:20:14Z

Is there any usage for context propagation in the "garbage-free world"? My guess is that switching threads using Reactive streams or similar provides too much garbage for it to be a viable option.

If that is the case the existing ThreadContext.getImmutableContext might be enough for most users and this PR is not needed.

@ppkarwasz I don't think there is a garbage-free way to do Reactive streams. I would keep this simple and not worry about any garbage-free use cases.

ppkarwasz · 2024-04-16T10:29:44Z

@remkop,

@ppkarwasz I don't think there is a garbage-free way to do Reactive streams. I would keep this simple and not worry about any garbage-free use cases.

Thanks, I am closing this PR then and I'll propose a solution based on the existing getImmutableContextOrNull method then.

ppkarwasz added the enhancement Additions or updates to features label Apr 4, 2024

ppkarwasz added this to the 2.24.0 milestone Apr 4, 2024

ppkarwasz self-assigned this Apr 4, 2024

ppkarwasz mentioned this pull request Apr 4, 2024

Scoped context #2438

Merged

ppkarwasz requested a review from rgoers April 10, 2024 13:07

rgoers reviewed Apr 12, 2024

View reviewed changes

ppkarwasz mentioned this pull request Apr 12, 2024

Optional ThreadLocalAccessors for SLF4J's MDC micrometer-metrics/context-propagation#194

Merged

Fix nullability and mutability problems

0404d45

vy reviewed Apr 15, 2024

View reviewed changes

src/changelog/.2.x.x/add_extended_thread_context.xml Outdated Show resolved Hide resolved

ppkarwasz and others added 4 commits April 15, 2024 22:07

Fix changelog schema location

fc3a097

Co-authored-by: Volkan Yazıcı <[email protected]>

Add JSpecify dependency

0351e78

Merge remote-tracking branch 'apache/2.x' into feature/context-propag…

881a055

…ation

Rename save/restore messages

3a30631

Revert changes to MDCContextMap

4515668

ppkarwasz closed this Apr 16, 2024

ppkarwasz deleted the feature/context-propagation branch August 14, 2024 08:00

	@Override
	public Object save() {
	final StringMap map = localMap.get();
	return map != null && !map.isEmpty() ? createStringMap(map) : null;
	}

Uh oh!

Add support for thread context map propagation #2442

Add support for thread context map propagation #2442

Uh oh!

Conversation

ppkarwasz commented Apr 4, 2024

Uh oh!

rgoers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ppkarwasz commented Apr 12, 2024

Uh oh!

ppkarwasz commented Apr 12, 2024

Uh oh!

Uh oh!

ppkarwasz commented Apr 16, 2024

Uh oh!

remkop commented Apr 16, 2024

Uh oh!

ppkarwasz commented Apr 16, 2024

Uh oh!

Uh oh!