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 withLoggingContextAsync #165

Closed
tandel-pratik opened this issue Feb 5, 2021 · 12 comments
Closed

Add withLoggingContextAsync #165

tandel-pratik opened this issue Feb 5, 2021 · 12 comments

Comments

@tandel-pratik
Copy link

tandel-pratik commented Feb 5, 2021

For coroutines to propagate MDC context properly, we need to write code as

withLoggingContext("foo" to bar, "xyz" to thing) {
            withContext(MDCContext()) {
               // suspending block
            }
}

It'd be a lot more convenient if we could add a new method that took a suspending method as a default:

public inline fun <T> withLoggingContextAsync(vararg pair: Pair<String, String>, body: suspend () -> T): T

and reduced this boilerplate.

Happy to submit a PR if we agree on the nomenclature

@oshai
Copy link
Owner

oshai commented Feb 11, 2021

Thanks! A PR is welcome, but the reason it wasn't done in the first place is that I think it will require an additional dependency on org.jetbrains.kotlinx:kotlinx-coroutines-slf4j. We should think of a way to avoid that for cases not using coroutines.

@tandel-pratik
Copy link
Author

If there's an existing slf4j dependency, would it be too problematic to introduce a kotlinx-coroutines-slf4j dependency?

@oshai
Copy link
Owner

oshai commented Feb 12, 2021

Yes, because that adds a dependency on kotlinx-coroutines as well for anyone using kotlin-logging regardless if coroutines are used.. I prefer to keep kotlin-logging light and without that dependency consider the fact that we are talking about one method. I think a better approach might be to add such method to kotlinx-coroutines-slf4j itself or a (new) integration module kotlinx-coroutines-kotlin-logging?

@severn-everett
Copy link
Contributor

I tried taking this up but got blocked on some dependency version resolution issue in the Kotlinx Coroutines library; I'll post an update if/when I get one.

@severn-everett
Copy link
Contributor

PR in the Kolinx Coroutines repository is up, if you'd like to take a look at it.

@severn-everett
Copy link
Contributor

...and it got rejected. @oshai looks like you'll need to create another library to contain this integration, because the Coroutine library maintainers don't want it in their code base.

@oshai
Copy link
Owner

oshai commented Dec 16, 2021

@severn-everett another option is that you'll maintain such module in another repo if you'd like.

@severn-everett
Copy link
Contributor

Probably a better way to put is whether you know how they create the integration modules in the Kotlinx Coroutines library, as I could put the code there. It could be published as something like kotlin-logging-jvm-coroutine so that developers could use withLoggingContextAsync without requiring the Coroutines library as a dependency for the base kotlin-logging jar. If you don't know, I could ask the Coroutines team how they do it.

@oshai
Copy link
Owner

oshai commented Dec 18, 2021

Yes, if you know how to do that I'm ok with such PR.

@oshai
Copy link
Owner

oshai commented Feb 13, 2023

So the current status is:
PR for integration module was rejected by kotlin team as they don't want to support another module: Kotlin/kotlinx.coroutines#3092

One option to try is to have the existing slf4j module with kotlin-logging dependency.
Another option is to have an additional module here (will require changes to the lib structure).

oshai added a commit that referenced this issue Feb 17, 2023
@oshai
Copy link
Owner

oshai commented Feb 17, 2023

I created an initial PR in the same module, the only caveat is that users will have to add the slf4j coroutines dep is it is provided only.

#278

@oshai
Copy link
Owner

oshai commented Feb 19, 2023

Added in 4.0.0-beta-20.

@oshai oshai closed this as completed Jul 11, 2023
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

3 participants