Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: MDC support #31

Closed
mkobit opened this issue Dec 28, 2017 · 11 comments
Closed

Feature: MDC support #31

mkobit opened this issue Dec 28, 2017 · 11 comments

Comments

@mkobit
Copy link

mkobit commented Dec 28, 2017

Consider adding some additional functions for managing MDC (Javadoc: https://www.slf4j.org/api/org/slf4j/MDC.html) by putting and removing keys

Additional consideration: it would be great to have it with coroutines

Docs: https://www.slf4j.org/manual.html#mdc

I don't have a concrete API proposal but here is some initial thoughts:

object MDC {
  // additional overloads for varargs and up to some N number of `Pair`
  fun <T> withContext(keyPair: Pair<String, String>, body: () -> T) {
    return org.slf4j.MDC.putCloseable(keyPair.first, keyPair.second).use {
      body()
    }
  }
}
@oshai
Copy link
Owner

oshai commented Dec 28, 2017

looks like a neat idea! any other suggestions?

@oshai
Copy link
Owner

oshai commented Dec 28, 2017

How about an extension method instead:

inline fun <T> Pair<String, String>.asLoggingContextOf(body: () -> T): T = 
            MDC.putCloseable(first, second).use { body() }

@mkobit
Copy link
Author

mkobit commented Dec 28, 2017

That works well if you have a single key/value pair you want to put into context.
How would multiple pairs or "additive" contexts look like?

I think that coming up with the right API here will be a bit difficult, so it might be worth it to enumerate use cases. Here are at least a few I am thinking of:

  • wrapping some method body with single pair:

    fun myFunction(name: String) {
      mdc.withContext("name" to name) {
        doThing()
      }
    }
  • wrapping multiple pairs

    fun myFunction(name: String) {
      mdc.withContext("name" to name, "otherThing" to "otherValue") {
        doThing()
      }
    }
  • a map of parameters

    fun myFunction(params: Map<String, String>) {
      mdc.withContext(params) {
        doThing()
      }
    }
  • coroutines support in (not sure how this works without more coroutine support, possibly in a different libarary or out of scope for this request)

    fun myFunction(name: String) {
      launch(CommonPool) {
        mdc.withContext("name" to name) {
          // support for suspending functions?
          val thing1 = doThing1()
          val thing2 = doThing2()
          thing + thing2
        }
      }
    }

@oshai
Copy link
Owner

oshai commented Dec 28, 2017

I think coroutines are out of the initial scope as MDC current infra relies on threads.
Basically, for multiple pairs it is possible to do something like:

(key1 to value1).asLoggingContextOf { 
  (key2 to value2).asLoggingContextOf {
     ... 
  }
} 

So the real question here is how required / strong is the use case. Currently, I don't use MDC (although I might start using it when support will be available ;-)
But I am not sure how strong is the use case. If it was I guess it would probably be implemented in MDC.putCloseable() as well.

It is always easier to start with partial API and add later.

What do you think?

@mkobit
Copy link
Author

mkobit commented Dec 29, 2017

I agree in cutting MDC support for coroutines the first iteration of this. There might be some way to handle it with ContinuationInterceptor but that can be held off on as it isn't the main use case (for me at least) right now.

The background for how I am hoping to migrate to MDC in one part of our application:

  • We have an old log4j format custom layout that converts messages to JSON
  • loggers construct and pass in a special type type we have to the logger.info(Object message) (and other levels) that has a map of keys and values that are added to to the constructed JSON. This gets output and consumed downstream in log collection infrastructure
  • SLF4J API takes a String message, so we can't really keep passing in our custom object

I'd like to get us moving forward for us by:

  • Switch all of our stuff from using Log4j directly to SLF4J
  • Switch to using this library (which we are using in other places already)
  • store those key/values in MDC and have a Layout pull them out and format the message as JSON instead

In our case, our key/value maps sometimes have 9-10 keys and values, so repeating (k to v).asLoggingContextOf {} that many nested times is not desirable.

vararg Pair<> (or N-arg Pair methods similar to Guava with https://google.github.io/guava/releases/21.0/api/docs/com/google/common/collect/ImmutableList.html#of-E-E-E-E-E-E-E-E-E-E-E- and https://google.github.io/guava/releases/21.0/api/docs/com/google/common/collect/ImmutableList.html#of-E-E-E-E-E-E-E-E-E-E-E-E-E...-) or Map<> methods would be more useful for our case.

We can do all of that migration ourselves, but Kotlin-friendly MDC helpers are not present in this library (right now!). I can always try out a few different APIs in our own internal project and see how my teammates feel before following up here, but I thought other users may also look for it.

@oshai
Copy link
Owner

oshai commented Dec 29, 2017

ok, I will implement few flavors, let me know what you think.

oshai pushed a commit that referenced this issue Dec 29, 2017
@oshai
Copy link
Owner

oshai commented Dec 29, 2017

please see the commit let me know what you think, then I will push it to 1.4.9

@mkobit
Copy link
Author

mkobit commented Dec 30, 2017

👍 from me, looks good!

@oshai
Copy link
Owner

oshai commented Dec 30, 2017

version 1.4.9 deployed.

@oshai oshai closed this as completed Dec 30, 2017
oshai added a commit that referenced this issue Jan 17, 2018
* issue #31 - Add MDC support: withLoggingContext

* Update ChangleLog.md

* Update ChangleLog.md

* Update README.md

* Rename ChangleLog.md to ChangeLog.md (#32)
oshai added a commit that referenced this issue Jan 24, 2018
* issue #21 - make the lib multiplatform
- split to common/jvm/js
- left all code in jvm at the moment
- add build scripts

* issue #31 - Add MDC support: withLoggingContext

* merge to branch: issue #31 - Add MDC support: withLoggingContext

* add documentation

* rebase (#33)

* issue #31 - Add MDC support: withLoggingContext

* Update ChangleLog.md

* Update ChangleLog.md

* Update README.md

* Rename ChangleLog.md to ChangeLog.md (#32)

* update to kotlin 1.2.20

* split packages and gradle build

* split packages and gradle build

* split packages and gradle build

* fix src root for jvm project

* fix artifact build for jvm version
@oshai
Copy link
Owner

oshai commented Jun 11, 2018

More details on coroutines with mdc are here: Kotlin/kotlinx.coroutines#119

@oshai
Copy link
Owner

oshai commented Aug 23, 2018

coroutines support is now available, more details here: https://github.com/MicroUtils/kotlin-logging/wiki#mdc-and-threadscoroutines

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants