MDCAmbit to assist in creating and closing MDC entries

[ceki]

MDCClosable was supposed to help users remove entries at the end of a try/catch block. Unfortunately, as discussed in SLF4J-557 closable instances, including MDCCloseable instances are closed before the catch clause. This is very counter-intuitive and most probably goes against the wishes of the developer.

To alleviate this problem, MDCAmbit is being proposed under the branch_2.1.x branch.

Usage examples can be seen in the javadocs of MDCAmbit and well as in MDCAmbitTest

Given that this MDCAmbit is slated to be added to the org.slf4j package, your feedback is requested.

For example, assuming MDCAmbit is added to the org.slf4j package, what should be the SLF4J release version? Should it be version 2.0.10, 2.1 or 3.0?

[philsttr]

The concept seems ok, but the name MDCHelper does not convey the purpose of the class. Perhaps MDCScope would be better, since it will hold MDC keys scoped to a particular section of code.

Also the name removeSet() is not quite clear. Set seems like an internal concept within the class that should not be part of the public interface. Perhaps if the class is named MDCScope, then a method name like end() could be used to denote that the scope is ending. Or maybe use clear() (to be similar to MDC.clear()) or just remove().

The downside of the term "scope" is that it is also used by the upcoming Scoped Values feature. So if there is ever a future where the MDC (or something like it) is a ScopedValue instead of a ThreadLocal (which I think is definitely worth considering at some point), using the term "scope" could cause confusion.

Naming is hard.

[ceki]

Thank you for your comments. Naming is hard indeed.

Preview features may be great for experimenting with new APIs but as far as libraries are concerned, preview features are unusable given the great complexity it adds. We have no choice but to wait until Scoped Values are no longer a preview feature. However, you are right to observe that Scoped Values is something we will need to take into consideration in the future.

How about MDCAmbit instead of MDCHelper?

I also agree with your comments regarding removeSet() .

The API has been updated in response to your comments.

[mourjo]

Could we use something like a Runnable instead of a helper?

public static void withMDC(Map<String, String> ctx, Runnable lambda) {
    var previousCtx = MDC.getCopyOfContextMap();
    try {
      MDC.clear();
      MDC.setContextMap(ctx);
      lambda.run();
    } finally {
      MDC.clear();
      MDC.setContextMap(previousCtx);
    }
  }

public static void withMDC(Runnable lambda) {
    with(Map.of(), lambda);
  }

withMDC(() -> {
   MDC.put("some", "thing");
});

[ceki]

Changes to the MDC state should be additive. The idea is to set some keys and to clear them at the end withtout touching any previously set keys. Moreover, in the statement with(Map.of(), lambda); which method does with refer to?

[mourjo]

Thanks for responding! Sorry for the confusion, let me clarify what I meant.

which method does with refer to?

with refers to the mapping context the runnable starts with. Something akin to of in Map.of. So it would be to initialize the mapping context with some preset keys. Like:

withMDC(Map.of("another", "thing"), () -> {
   MDC.put("some", "thing"); // this has both "some" and "another" in the context
});

I see, but to the outside world, it would be additive - because clear and restore happens inside the call to withMDC.

Changes to the MDC state should be additive.

Combining both I guess we could use something like this:

public static void withBoundedCtx(Runnable lambda) {
    var previousCtx = MDC.getCopyOfContextMap();
    try {
      lambda.run();
    } finally {
      if (previousCtx != null ) {
         MDC.setContextMap(previousCtx);
       } else {
         MDC.clear();
       }
    }
  }

Usage:

MDC.put("another", "thing"); 
 // MDC has ony "another" as keys

withBoundedCtx(() -> {
   MDC.put("some", "thing");      // MDC has "another" and "some" as keys
});

 // MDC has ony "another" as keys

[hkupty]

I think a typical use-case for this would be a layer that always adds the same key and delegates that to a block:

public void handleRequest(Request req) {
    MDCAmbit
      .of("requestId", getRequestId(req))
      .withMdc(controller::handleRequest);
}

This design resonates well with the ScopedValue (proposed) implementation and, as I said in my comment below, if SLF4J backends can implement their own versions, I'd prefer to use ScopedValues altogether instead of ThreadLocal and this would be a nice way to abstract that.

[hkupty]

I think this is a great addition. One thing I would consider is making it extendable/implementable by the libraries implementing SLF4J. It could potentially replace MDC itself and allow ScopedValues in newer JVM instead of a ThreadLocal, which can be problematic with virtual threads.

Additionally, I'd second having a method that takes a runnable, but maybe, to preserve the additive property, prepare the state outside and only apply before running the code block, cleaning up right after...

[drub0y]

Love this API. I have two requests from a devx perspective:

First, it would be great if MDCAmbit implemented Closeable so we could use succinct try blocks like so that instead of this:

MDCAmbit mdcAmbit = new MDCAmbit();
// populate here

try {
  // logic here
} finally {
  mdcAmbit.clear();
}

we could just do this:

try(MDCAmbit mdcAmbit = new MDCAmbit()) {
   // populate here

   // logic here
}

Second, it would be awesome if the API provided a builder API so that we can do this:

MDCAmbit mdcAmbit = MDCAmbit.builder()
   .put("k0", value0)
   .put("k1", value1)
   .build();

If you were to combine the Closeable and the builder together you could have the following clean setup to every scope:

try(MDCAmbit mdcAmbit = MDCAmbit.builder()
   .put("k0", value0)
   .put("k1", value1)
   .build()) {
  // logic here
}

[ceki]

Hi @drub0y,

Thank you for your input. As mentioned at the top of this page and as discussed in SLF4J-557 Closable instances instances are closed before the catch clause is reached. This is very counter-intuitive and usually goes against the wishes of the user.

Thus, Closable is unfortunately not a good idea in this particular case.

It follows that the builder idea is not that useful either because you need to call clear within a finally block.

[zacthompson]

This MDCAmbit feature seems purely additive to the API, and as such is perfectly fine to release as part of any 2.x version in my opinion, even 2.0.10. But if you're wanting to ever mark MDCCloseable/putCloseable as deprecated for any reason, that should at minimum be a new "minor version" (2.1.x or something). No need for a 3.x line if there are no breaking changes. (e.g., if you were to ever remove MDCCloseable completely, that would probably warrant 3.x.)

(A small question: Is it accurate to say that MDCAmbit, like MDCClosable before it, is not meant for use with "stack/queue" interfaces like push/popByKey? That's probably for the best: I don't really know of any significant use of those push/pop methods "in the wild", and they seem to be largely focused on supporting older use cases.)