implement putAll and putAllCloseable

[Okeanos]

Implements putAll and putAllCloseable described in SLF4J-387 – I ran into the same problem a short while ago that I needed a closeable MDC that discards multiple entries after the try-with resource is closed.

If more tests or other changes are required I'll happily take another look at this.

[ceki]

relates to https://jira.qos.ch/browse/SLF4J-387

[Okeanos]

@ceki If there's anything from me you need to get this merged please let me know and I'll have a look.

[ceki]

It's in the pipeline.

[Okeanos]

Updated the pull request to resolve merge conflicts with master and removed an unused import I accidentally introduced. If there's anything else you need to continue please let me know and I'll have a look.

[Okeanos]

@ceki is there a chance you or someone else from SLF4J can take a look to get this merged into a 2.x or 2.0.x release?

[ceki]

@Okeanos Would you be satisfied with MDC.putAllCloseable(List<String>) ? It seems to me that the values for the keys might not be known at try-time. Using a list of keys seems more robust. Would you agree?

[Okeanos]

I am not sure I follow – do you have a minimal example for:

It seems to me that the values for the keys might not be known at try-time.

Additionally, what would we use as keys then? The idea/intent is to put known key-value-pairs into the MDC for operations that should have no long-term effect on the thread local MDC but we in the end we can still filter log lines by MDC keys consistently.

[ceki]

@Okeanos Here is what I had in mind:

public class MDCExplore {

    public static void main(String[] args) {
        final List keyList = List.of("KEY1", "KEY2");
        MDC.put("x", "y"); // populate before try


        try (Closeable closeable = mdc_closables_placeholder(keyList)) {
            MDC.put("a", "b");
            MDC.put("KEY1", "val1");  
            // KEY2 is populated at another code block

            // some useful code

        }
   
      // outside the `try` the keys x and a remain populated, KEY1, KEY2 are cleared
    }

    // This method would be located within the org.slf4j.MDC class. 
   // In the meantime, the following method serves as a placeholder. 
    
    private static Closeable mdc_closables_placeholder(List keyList) {
       // return a Closeable containing and acting on the keyList 
       return ....;
    }
}

[Okeanos]

I think I understand what you intend, thanks for taking the time! I'll get back to you next week, when I have a little more time to illustrate my own intentions with proposing putAllCloseable as natural extension of putCloseable and why it would be useful to have around.

[Okeanos]

Okay, I had some time to write a real-world inspired example of why I think MDC.putAllCloseable(Map<String, String> map) is neat. The example is based on using Project Reactor to make async HTTP calls. The WebClient you see below has its own Thread Pool totally disconnected from the Thread Local and also will reuse connections & threads in said pool.

public String someFunction(final UUID id) {
    ResponseEntity<String> response = webClient
        .get()
        .uri(uriBuilder -> uriBuilder.pathSegment("entry", "{id}").build(id))
        // ... handle request
        .doOnEach(signal -> {
            if (signal.isOnError() ||signal.isOnNext() ) {
                // magic MDCCloseable
                try (var mdc = MDCCloseable.putAllCloseable(signal.getContextView().getOrDefault("MDC", new HashMap<>()))) {
                    var response = signal.get();
                    if (signal.isOnError()) {
                        log.warn(
                            "get pipeline failed {}", "value",
                            kv("response_status", response.getStatusCode()),
                            kv("response_body", response.getBody())
                        );
                    } else {
                        log.debug(
                            "get pipeline did stuff",
                            kv("response_status", response.getStatusCode()),
                            kv("response_body", response.getBody())
                        );
                    }
                }
            } else {
                return;
            }
        })
        //.. do more stuff
        // adds MDC to actual async call stack as context variable
        .contextWrite(Context.of("MDC", Optional.ofNullable(MDC.getCopyOfContextMap()).orElseGet(HashMap::new)))
        .block();
    
    // do stuff with the actual response and inspect it

    return response.getBody();
}

If I understood your example correctly, transferring it to my example would look similar to this:

public String someFunction(final UUID id) {
    ResponseEntity<String> response = webClient
        .get()
        .uri(uriBuilder -> uriBuilder.pathSegment("entry", "{id}").build(id))
        // ... handle request
        .doOnEach(signal -> {
            if (signal.isOnError() ||signal.isOnNext() ) {
                var localMdc = signal.getContextView().getOrDefault("MDC", new HashMap<>())

                try (var closeable = MDCCloseable.putAllKeys(localMdc.keySet())) { // insert keys to be removed
                    localMdc.forEach(MDC::put);

                var response = signal.get();
                    if (signal.isOnError()) {
                        log.warn(
                            "something failed {}", "value",
                            kv("response_status", response.getStatusCode()))
                        );
                    } else {
                        log.debug(
                            "some debug",
                            kv("response_status", response.getStatusCode())
                        );
                    }
                }
            } else {
                return;
            }
        })
        //.. do more stuff
        // adds MDC to actual async call stack as context variable
        .contextWrite(Context.of("MDC", Optional.ofNullable(MDC.getCopyOfContextMap()).orElseGet(HashMap::new)))
        .block();
    
    // do stuff with the actual response and inspect it

    return response.getBody();
}

It's not a huge difference but there's certainly a couple of "gotchas"/caveats here adding noise to the overall implementation.

I already pointed out that putCloseable(key, value) exists and would personally prefer extending the semantics and implementation of that instead of creating a new and fully separate idea.

[ascopes]

Would an override like java.util.Map.of(String, String, ...) be useful here?

In many of the places in applications where I have wanted to use this sort of functionality, I have usually wanted to add several entries at once. In this case it would read something like

try (MDC.putAllCloseable(
    "X-Request-ID", request.getHeader("X-Request-ID"),
    "User-Agent", request.getHeader("User-Agent"),
    "From", request.getHeader("From")
)) {
    processRequest();
}

versus having to declare this separately.

If not, I wonder if having the ability to chain additional put operations onto the closeable may be useful, along with a functional method to run the closeable in a closure directly.

MDC.putCloseable("foo", "bar")
   .put("baz", "bork")
   .put("qux", "quxx")
   .run(() -> {
       processRequest();
   });

where run has a signature of something like

@FunctionalInterface
public interface ThrowingRunnable<T extends Throwable> {
  void run() throws T;
}

@FunctionalInterface
public interface ThrowingSupplier<R, T extends Throwable> {
  R get() throws T;
}

public void <T extends Throwable> run(ThrowingRunnable<T> runnable) throws T {
  try (this) {
    runnable.run();
  }
}

public void <R, T extends Throwable> run(ThrowingSupplier<R, T> supplier) throws T {
  try (this) {
    return supplier.get();
  }
}

Probably out of scope for this PR, but wondering if it may compliment this functionality in the future perhaps?

[ceki]

SLF4J-557 makes a very valid point.

[Okeanos]

SLF4J-557 makes a very valid point.

That is indeed a fair point. I am now wondering how to work around that because the auto-closeable neatness I tried showing in my examples above would be sorely missed :/. A clean, consistent, and predictable API does strike me as more important though.