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

Add an integration with Log4J 2's ThreadContext #1794

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

MariusVolkhart
Copy link
Contributor

Log4J 2 has a ThreadContext, which works the same way as SLF4J's MDC. Using the ThreadContext directly with coroutines breaks, but the same approach for an integration that exists for SLF4J can be used for Log4J.

The tests are copied from the SLF4J project, and are only modified to also include verification of stack state, since ThreadContext contains both a Map and a Stack.

@elizarov elizarov self-requested a review February 14, 2020 07:03
Copy link
Contributor

@elizarov elizarov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should also add this new integration module to settings.gradle file so that it becomes part of the project.

README.md Outdated
@@ -52,6 +52,7 @@ suspend fun main() = coroutineScope {
* [integration](integration/README.md) — modules that provide integration with various asynchronous callback- and future-based libraries:
* JDK8 [CompletionStage.await], Guava [ListenableFuture.await], and Google Play Services [Task.await];
* SLF4J MDC integration via [MDCContext].
* Log4J 2 ThreadContext integration via [Log4JThreadContext]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: the line should end with dot, previous one should end with semicolon.

* ThreadContext.put("kotlin", "is awesome")
* }
* assert(ThreadContext.get("kotlin") == "rocks")
* ```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above is a misleading and fragile example. In case of any suspension in the outer code or inside of withContext the behavior of this code will change, as the thread context will get restore to the state that is captured in the coroutine.

This is true for any pattern like this:

ThreadContext.put("kotlin", "rocks")
// <-- adding suspension here breaks it
withContext(Log4JThreadContext()) { ... }

What I suggest is to design a convenient API to specify additional parameters right in the Log4JThreadContext, so that you can write the above code in a less-fragile way:

withContext(Log4JThreadContext(mapOf("kotlin" to "rockts"))) { ... }

(or something similar)

* @param mdc a copy of the mapped diagnostic context.
* @param ndc a copy of the nested diagnostic context.
*/
public class Log4JThreadContextState(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need this class in the public API or in implementation for that matter? How about storing both mdc and ndc directly in Log4JThreadContext instance and cutting this middle-man out?

@qwwdfsad qwwdfsad force-pushed the develop branch 3 times, most recently from 4a49830 to aff8202 Compare March 10, 2020 17:27
Log4J 2 has a ThreadContext, which works the same way as SLF4J's MDC. Using the ThreadContext directly with coroutines breaks, but the same approach for an integration that exists for SLF4J can be used for Log4J.

The tests are copied from the SLF4J project, and are only modified to also include verification of stack state, since ThreadContext contains both a Map and a Stack.
@MariusVolkhart
Copy link
Contributor Author

@elizarov Thank you very much for the feedback. I feel I gained a much better understanding of coroutines from it, and appreciate you taking the time to leave it.

I've applied the changes you suggested, and will summarize them below.

  • Dropped support for the logging context stack. It now only supports the Map. Conversations from the Log4J 2 maintainers suggested that using the stack is not really encouraged. This matches the behavior I've seen in other applications.
  • Renamed Log4JThreadContext -> DiagnosticContext as I think it is unlikely for developers to have multiple conflicting classes of the same name, and I wanted to get away from thinking about threads.
  • Added the MutableDiagnosticContext type, for cases when the state for the duration of the coroutine context should be modified. The API for this matches closely with that of Log4J's CloseableThreadContext, which rolls-back the state when close() is called.
  • Added the immutableDiagnosticContext() function to create an optimized instance of DiagnosticContext for cases when the state should be captured when the coroutine context starts, but will not be modified.

@t1mwillis
Copy link

@MariusVolkhart Any updates on this integration?

@MariusVolkhart
Copy link
Contributor Author

@t1mwillis What sort of an update are you looking for? I believe this code to be working, but I've only had limited ability to deploy it into production.

@elizarov would you please take another look at this?

@MariusVolkhart
Copy link
Contributor Author

@t1mwillis It has recently occurred to me that it may be possible to implement the ThreadContextMap interface in such a way that the CoroutineContext is used as the storage container, rather than a ThreadLocal map as is used by the default ThreadContextMap subclasses. The API would probably look pretty similar, but there would likely be one round of copying things from the CoroutineContext to theThreadLocal and back saved.

I haven't had the chance to investigate this yet.

@t1mwillis
Copy link

@MariusVolkhart my use case was simply having a structured way to write a tracking id from the parent thread to a coroutine. At present I am grabbing this tracking id from the parent thread and then setting it in the coroutine using threadContext.put('trackingid', parentThreadTrackingId). This works but seems unnecessary

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

Successfully merging this pull request may close these issues.

3 participants