Repository navigation
Extend the way of how to create the logger #480
Description
Note: I know that's an old discussion, but just to revise it again.
Problem
Currently, the "idiomatic" way to create logger is a top-level variable.
import io.github.oshai.kotlinlogging.KotlinLogging
// Place definition above class declaration, below imports,
// to make field static and accessible only within the file
private val logger = KotlinLogging.logger {}- Lot's of JVM backend teams prefer to have private variables inside the class. What if we have couple of private static variables, should we also place them as a top level declaration? And what if we have instance private variables as well? This introduce inconsistency and hard-to-read code base, since some of the variables could be as a top-level, some of them as a companion object and some of them as a instance variables.
import io.github.oshai.kotlinlogging.KotlinLogging
private val logger = KotlinLogging.logger {}
class MyClass {
private companion object {
private val someStaticVar = 1
private val anotherStaticVar = 2
}
private val someVar = 3
}- Let's add to the example above a combination with long doc on top of the class that's move out completely the logger from view.
import io.github.oshai.kotlinlogging.KotlinLogging
private val logger = KotlinLogging.logger {}
/**
* very long Doc
*/
class MyClass {
private companion object {
private val someStaticVar = 1
private val anotherStaticVar = 2
}
private val someVar = 3
}- Exposes the top-level class to the public API.
If I'm writing a Java library, adding a variable at the top level introduces an public empty top-level class. In recent version of Kotlin we can "embed" the top-level variable using@file:JvmName
@file:JvmName("MyPublicAPI") // may work in recent kotlin versions
package example
import io.github.oshai.kotlinlogging.KotlinLogging
private val logger = KotlinLogging.logger {}
class MyPublicAPI {
}-
Top level variables introduce additional top level class (if we are not using
@file:JvmNamethe same as class name). It just makes the final jar bigger. -
Before Kotlin 2.0 each lambda functions compiled to the anonymous classes, that also makes final jar bigger, because for every logger we need a lambda. https://kotlinlang.org/docs/whatsnew20.html#generation-of-lambda-functions-using-invokedynamic
-
There is ongoing proposal about static namespace/static variables and current syntax does not allow to migrate (with IDEA/compiler hint) from this syntax:
class MyClass {
private companion object {
private val logger = KotlinLogging.logger(this)
}
}to this
class MyClass {
private static val logger = KotlinLogging.logger(this::class) // also can be optimized with "inline .class syntax"
}- Kotlin statics and static extensions Kotlin/KEEP#348
- https://youtrack.jetbrains.com/issue/KT-15595/Optimize-away-unused-and-empty-private-companion-object
- https://youtrack.jetbrains.com/issue/KT-19954/companion-val
- https://youtrack.jetbrains.com/issue/KT-11420
- https://youtrack.jetbrains.com/issue/KT-11968/Research-and-prototype-namespace-based-solution-for-statics-and-static-extensions
Suggested solution
My proposal would be to change logger(func: () -> Unit) to logger(ref: Any). Currently in all cases className derived via reflection (or even via stack trace in js), so it does not matter if it's a lambda or an object. The teams are free to chose what syntax to use then. And also the current code with lambda top-level declaration will work as well.
val topLevelNamedLogger = KotlinLogging.logger("TopLevelNamedLogger")
val topLevelLambdaLogger = KotlinLogging.logger {}
class MyClass {
val classNamedLogger = KotlinLogging.logger("MyClass")
val classLambdaLogger = KotlinLogging.logger {}
val classRefLogger = KotlinLogging.logger(this)
companion object {
val companionNamedLogger = KotlinLogging.logger("MyClassCompanion")
val companionLambdaLogger = KotlinLogging.logger {}
val companionRefLogger = KotlinLogging.logger(this)
}
}Braking changes
- Logger property delegate for Kotlin/Js was removed. It's not possible to make it work in case of companion object.
- KotlinLogging.logger(underlyingLogger: Logger) was removed due to the clash with object method.
Other changes
- Covered all use cases for Kotlin/js and Kotlin/wasm
- Optimize the KLoggerNameResolver for jvm, since we don't need to search indexOf multiple times
Activity
github-actions commentedon Mar 3, 2025
Thank you for reporting an issue. See the wiki for documentation and slack for questions.
Issue oshai#480 - Extend the way of how to create the logger
Issue oshai#480 - apply formatting
Issue oshai#480 - remove clashed KotlinLogging extension
oshai commentedon Mar 6, 2025
Hi,
In my opinion the recommended way to configure balances ease of use, simplicity and conciseness.
Adding in companion will also work, and if you provide the name there is no need for the lambda (although in latest jvm the lambda is even not created iirc).
You raised here few issues and the change in the PR is quite big.
I approved it to run to see how it affects current tests, but to address it better I suggest to minimize and separate the changes so we can discuss what necessary and what's less appropriate currently.
scrat98 commentedon Mar 6, 2025
@oshai I know that it seems lot's of changes, but actually it's not. We can separate on 3 topics:
Currently, for Kotlin/JS, there's a workaround involving
LoggerDelegate, which prevents using the "idiomatic" approach. To address this, I rewrote the implementation to support all possible logger declaration use cases and added test coverage. However, it seems that using parameterized tests isn't an option, which is why there are so many changes. You’ll also find a "TODO: use parameterized test?" comment in the code.But then I also realized that this code also will work with Kotlin/WASM and decided to add tests and change implementation as well.
Optimize the KLoggerNameResolver for jvm, since we don't need to search indexOf multiple times. https://github.com/oshai/kotlin-logging/pull/481/files#diff-a4a47d5d1948c766fb2400625d6e712791c339132da44e33e6885c638676f0e8
Proposal to change
logger(func: () -> Unit)tologger(ref: Any). This is a minor change, as the current implementation already works well with both lambdas and other references (except in Kotlin/JS). From a code modification perspective, the cost is almost negligible. The main consideration here is stylistic preference. In my opinion, usinglogger(ref: Any)doesn’t alter functionality but simply provides the flexibility to choose the most suitable logger declaration based on your programming language(target platform). To discuss this point, I would prefer to cover all the problems from the issue description one by one.oshai commentedon Mar 6, 2025
So I suggest to separate it into 3 pull requests.
Let’s start with the small one then move on to bigger changes.
About js I remember vaguely there wasn’t a good way to support both browser and node but maybe it changed or I missed something. I can try to find the old issue.
Issue oshai#480 - merge master