ability to create custom logger factories to make non-SLF4J implementations

[thadcodes]

A proposal for a change that is slightly breaking but allows consumers to create their own kLogger implementations for custom logging environments. My use-case is for Android apps the SLF4J-Android implementation is deprecated and badly formats exceptions and cuts off most of the stack trace so I used this to have an implementation that in an Android app simply calls the Log.x functions.

For simplicity it also is a breaking change as it removes the SLF4J logger base interface from KLogger in JVM so I proposed a 2.0 version. Interested to hear any feedback

[oshai]

What do you think about taking a different approach and having another sub module for jvm android? This way you can implement it to call the android log itself directly.

[thadcodes]

I toyed with that a bit but I felt this approach was more flexible and didn't require making the core library dependent on Android directly

…

On Mon, Jul 13, 2020, 5:10 PM Ohad Shai ***@***.***> wrote: What do you think about taking a different approach and having another sub module for jvm android? This way you can implement it to call the android log itself directly. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#121 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAC5YDVJDVQDMQNFPJNEQULR3NZ3NANCNFSM4OYSERCA> .

[oshai]

I think it would make sense to have an android module that depends on Android. Not that the entire lib depend on android of course.

[thadcodes]

I basically wrote that for my internal library. Can share some of the source code if someone wanted to make an official module for it to publish. Would have to do it in the morning tomorrow

…

On Mon, Jul 13, 2020, 5:50 PM Ohad Shai ***@***.***> wrote: I think it would make sense to have an android module that depends on Android. Not that the entire lib depend on android of course. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#121 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAC5YDTL4PZLWRNNOUCMWWDR3N6RDANCNFSM4OYSERCA> .

[thadcodes]

Below is the promised code promised to @oshai using this to create an android module that depends on this if that was desired:

import android.util.Log
import mu.KLoggable
import mu.KLogger
import mu.Marker
import mu.KLoggerFactory
import java.lang.reflect.Modifier

/**
 * Android Logging implementation of [KLogger]
 */
class AndroidLogger(override val name: String) : KLogger {
    // No implementation, "catching" support not applied to android logger, unlikely to be used at this point
    override fun <T : Throwable> catching(throwable: T) = Unit

    override fun debug(msg: () -> Any?) {
        if (Log.isLoggable(name, Log.DEBUG)) {
            Log.d(name, msg.toStringSafely())
        }
    }

    override fun debug(t: Throwable?, msg: () -> Any?) {
        if (Log.isLoggable(name, Log.DEBUG)) {
            Log.d(name, msg.toStringSafely(), t)
        }
    }

        // No implementation, "marker" support not applied to android logger, unlikely to be used at this point
    override fun debug(marker: Marker?, msg: () -> Any?) = Unit

    // No implementation, "marker" support not applied to android logger, unlikely to be used at this point
    override fun debug(marker: Marker?, t: Throwable?, msg: () -> Any?) = Unit

    // No implementation, "entry" support not applied to android logger, unlikely to be used at this point
    override fun entry(vararg argArray: Any?) = Unit

    override fun error(msg: () -> Any?) {
        if (Log.isLoggable(name, Log.ERROR)) {
            Log.e(name, msg.toStringSafely())
        }
    }

    override fun error(t: Throwable?, msg: () -> Any?) {
        if (Log.isLoggable(name, Log.ERROR)) {
            Log.e(name, msg.toStringSafely(), t)
        }
    }

    // No implementation, "marker" support not applied to android logger, unlikely to be used at this point
    override fun error(marker: Marker?, msg: () -> Any?) = Unit

    // No implementation, "marker" support not applied to android logger, unlikely to be used at this point
    override fun error(marker: Marker?, t: Throwable?, msg: () -> Any?) = Unit

    // No implementation, "exit" support not applied to android logger, unlikely to be used at this point
    override fun exit() = Unit

    // No implementation, "exit" support not applied to android logger, unlikely to be used at this point
    override fun <T> exit(result: T): T = throw NotImplementedError("Exit not supported by android logger")

    override fun info(msg: () -> Any?) {
        if (Log.isLoggable(name, Log.INFO)) {
            Log.i(name, msg.toStringSafely())
        }
    }

    override fun info(t: Throwable?, msg: () -> Any?) {
        if (Log.isLoggable(name, Log.INFO)) {
            Log.i(name, msg.toStringSafely(), t)
        }
    }

    // No implementation, "marker" support not applied to android logger, unlikely to be used at this point
    override fun info(marker: Marker?, msg: () -> Any?) = Unit

    // No implementation, "marker" support not applied to android logger, unlikely to be used at this point
    override fun info(marker: Marker?, t: Throwable?, msg: () -> Any?) = Unit

    // No implementation, "throwing" support not applied to android logger, unlikely to be used at this point
    override fun <T : Throwable> throwing(throwable: T): T =
        throw NotImplementedError("throwing not supported by android logger")

    override fun trace(msg: () -> Any?) {
        if (Log.isLoggable(name, Log.VERBOSE)) {
            Log.v(name, msg.toStringSafely())
        }
    }

    override fun trace(t: Throwable?, msg: () -> Any?) {
        if (Log.isLoggable(name, Log.VERBOSE)) {
            Log.v(name, msg.toStringSafely(), t)
        }
    }

    // No implementation, "marker" support not applied to android logger, unlikely to be used at this point
    override fun trace(marker: Marker?, msg: () -> Any?) = Unit

    // No implementation, "marker" support not applied to android logger, unlikely to be used at this point
    override fun trace(marker: Marker?, t: Throwable?, msg: () -> Any?) = Unit

    override fun warn(msg: () -> Any?) {
        if (Log.isLoggable(name, Log.WARN)) {
            Log.w(name, msg.toStringSafely())
        }
    }

    override fun warn(t: Throwable?, msg: () -> Any?) {
        if (Log.isLoggable(name, Log.WARN)) {
            Log.w(name, msg.toStringSafely(), t)
        }
    }

    // No implementation, "marker" support not applied to android logger, unlikely to be used at this point
    override fun warn(marker: Marker?, msg: () -> Any?) = Unit

    // No implementation, "marker" support not applied to android logger, unlikely to be used at this point
    override fun warn(marker: Marker?, t: Throwable?, msg: () -> Any?) = Unit
}

private fun (() -> Any?)?.toStringSafely() = this?.invoke()?.toString().orEmpty()

/**
 * [KLoggerFactory] that emits [android.util.Log] backed log entries
 */
class AndroidLoggerFactory : KLoggerFactory {
    override fun logger(func: () -> Unit): KLogger = logger(name(func))

    override fun logger(name: String): KLogger = AndroidLogger(name)

    override fun logger(loggable: KLoggable): KLogger = logger(unwrapCompanionClass(loggable.javaClass).name)
}

/**
 * unwrap companion class to enclosing class given a Java Class
 */
private fun <T : Any> unwrapCompanionClass(clazz: Class<T>): Class<*> {
    clazz.enclosingClass?.also { enclosingClass ->
        try {
            val field = enclosingClass.getField(clazz.simpleName)
            if (Modifier.isStatic(field.modifiers) && field.type == clazz) {
                // && field.get(null) === obj
                // the above might be safer but problematic with initialization order
                return enclosingClass
            }
        } catch (e: Exception) {
            // ok, it is not a companion object
        }
    }
    return clazz
}

/**
 * get class name for function by the package of the function
 */
private fun name(func: () -> Unit): String {
    val name = func.javaClass.name
    return when {
        name.contains("Kt$") -> name.substringBefore("Kt$")
        name.contains("$") -> name.substringBefore("$")
        else -> name
    }
}

[oshai]

@thadcodes thanks for the code sample, I created #122 with the suggestion raised here. Hope someone will pick it up. Other than that please let me know what would you like to do with this PR.

[thadcodes]

Well I'm using my fork that has this pr in it personally and plan to keep using it to get rid of the slf logger dependency for the klogger interface. I'd like to see it merged into a branch for a 2.0 version since it's breaking if someone is expecting klogger to extend the logger interface. But I get the sense you are not a fan. What are your thoughts on it? If you want to leave my fork as a fork I understand and will close the pr. If you have suggestions on modifications that still remove the slf interface from the jvm version then I'd love to discuss.

…

On Thu, Jul 16, 2020, 5:23 PM Ohad Shai ***@***.***> wrote: @thadcodes <https://github.com/thadcodes> thanks for the code sample, I created #122 <#122> with the suggestion raised here. Hope someone will pick it up. Other than that please let me know what would you like to do with this PR. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#121 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAC5YDRG4TZPJBBUYCNP7NLR35VWHANCNFSM4OYSERCA> .

[oshai]

@thadcodes I think I get your point and see what you're trying to do.
I also got a similar request for ios on the slack channel.

I think it is possible to do it without breaking changes by implementing a "custom" module. I don't see why KLoggerFactory should be in the common module and not in a custom module only. After all the common module is not necessary an actual implementation. So I think the correct way to implement is only in the custom module KotlinLogging should delegate the logger calls to a KLoggerFactory which can be a var set by the user. The jvm module should be left as is.

The only issue is to what platform compile that module, but I think we can compile it to jvm, and later compile it to other platforms if needed.

Hope it's clear, Let me know if you have any question.

[thadcodes]

@oshai the part that makes this a breaking change is that the mu.KLogger interface in JVM extends the org.slf.Logger interface. The code I referenced in #121 (comment) doesn't provide implementations for those calls for Android Logger and it's used from the common interface, especially since these additional methods are in the platform independent version.

Actually the correct way in my view is that mu.KLogger should be entirely implemented in common code for multiplatform. I don't see how this becomes a true multiplatform library without that. I think we could make the current JVM version of the KLogger interface get renamed to something like KLoggerWithSLF and have it extend both the mu.KLogger and org.slf.Logger then make the default factory return that. Then those using the sfl functions could still cast to the new interface in JVM only but other implementers can use the pure Kotlin interface

[oshai]

I am not sure I quite follow you.
The way I see it is that KLogger is our interface to writing log messages. From common code you have to call it to "publish" your messages, and you should have a way to obtain it, and this is what KotlinLogging is doing in the common code. In the common code you don't care about the actual implementation.
How the logs are actually being handled is platform dependent and that is why I think it's enough to have those both interfaces as just expected in the common code.
Once you compile your code to a specific platform, all common code implementation will be bound to the platform specific implementation. In the case of Android, it will not be bound to the JVM module but to a new custom module that will have an option to "inject" loggers.

I am not 100% sure my solution is feasible. However, it feels to me that since multi platform might be change it's api in the future (maybe even with kotlin 1.4) it's not a good idea to introduce a concept based on a breaking change that we might not need / need to break again later to support. Maybe after 1.4 is being released it will make more sense to do so.

[thadcodes]

I think for proper multi-platform KLogger can't be an expect interface because then it is saying it has specific functions that implementers have to do differently for different platforms. Furthermore and more importantly Android uses JVM so I don't see how you can not have a different interface for android that doesn't require the org.slf.Logger member functions that may not apply and are not part of the documented interface for KLogger in common code

[oshai]

I think for proper multi-platform KLogger can't be an expect interface because then it is saying it has specific functions that implementers have to do differently for different platforms.

I think the idea of KLogger is to have different implementation per platform. MPP android platform is one example of platform to support.

Furthermore and more importantly Android uses JVM so I don't see how you can not have a different interface for android that doesn't require the org.slf.Logger member functions that may not apply and are not part of the documented interface for KLogger in common code.

A new android module or common module with custom injected logger should not depend on org.slf.Logger. Right now only the jvm module depends on slf4j and I think it should remain like that.

[thadcodes]

I don't feel it's clear that "android" is a platform in MPP. I think this is just allowing some details on how it creates the assets and publishes as an Android library but I'm not sure. I've tried a bit to verify but I can't seem to get it working. Will keep on trying but just wanted to let you know i have not ignored this.

[thadcodes]

@oshai I'm not going to have the time to get android dependencies to work at this time in the actual project but I see your argument. I'll close this PR and either I or someone else will work on that change at a later time

[oshai]

Thanks for the update and the effort done so far. Hope you will have time to get back to it in the future.

[oshai]

See updates on #122 on how to solve the issue.