search

oshai / kotlin-logging

Lightweight Multiplatform logging framework for Kotlin. A convenient and performant logging facade.
Other
2.61k stars 112 forks source link

Logger names are empty in JS/IR when declaring loggers via `KotlinLogging.logger {}` #315

Open YarnSphere opened 1 year ago

YarnSphere commented 1 year ago

This behaviour might have changed from Kotlin JS/Legacy, but Kotlin JS/IR doesn't seem to keep information on fully qualified names at runtime (mentioned in the documentation here, and there are also a few related issues open in YouTrack: [1], [2]).

Due to this, the current implementation of KotlinLogging.logger {} creates a logger with an empty string for name in JS/IR.

I propose that kotlin-logging tries to use the class' simpleName for name of the logger in JS (at least until there is a way to obtain a qualified name in JS, if they ever plan to support it).

I'm unsure whether this would be possible with the current implementation where the string is obtained from a stack trace (would have to give it a try), but something that would surely work would be an alternative way of declaring a logger using a function with a reified type: KotlinLogging.logger<RelevantClass>().

The implementation could use the qualifiedName of RelevantClass on all platforms except JS, where the simpleName would be used instead (this function would also feel a tad less hacky/brittle than the current one where the logger name is obtained from a stack trace :stuck_out_tongue:).

I could probably have a go at implementing this at some point, but let me know if you agree with the proposal and in which direction you'd rather go: attempt to make KotlinLogging.logger {} work properly in JS/IR (unsure if possible), provide a new KotlinLogging.logger<RelevantClass>() implementation, or even both.

oshai commented 1 year ago

We already encountered a similar issue and the solution was to create another impl for js. You can see it here: https://github.com/oshai/kotlin-logging/blob/master/src/jsMain/kotlin/io/github/oshai/KotlinLogging.kt

From previous testing JS worked differently for browser / node etc' so it was difficult to have one impl that fits all.

See detailed discussion at: https://github.com/oshai/kotlin-logging/issues/279

Let me know what you think.

YarnSphere commented 1 year ago

The above implementation doesn't really help me, I'm defining my logger in common multiplatform code, not directly in the JS code. Besides, I wouldn't want the logger to be an instance variable.

I hadn't considered the node vs browser scenario. When I get a bit of time I'll try to understand the differences a bit better, but what do you think of my suggestion of a new fun KotlinLogging.logger<reified T>() implementation? I believe that T::class.simpleName should be well supported in either JS target (I'd have to test regardless). And T::class.qualifiedName well supported in all non-JS targets.

recursive-rat4 commented 1 year ago

How can a top-level function (it does not have a relevant class) obtain a logger with this API?

YarnSphere commented 1 year ago

It can't, I suppose. Not unless it specifies a class to defer their name to. It could at least be an alternative implementation for when a logger is associated with a class. Either way, it might be possible to fix the current problem in JS with KotlinLogging.logger {}, I'll have to give it a try at some point.

severn-everett commented 5 months ago

Doesn't look like there's been any concrete movement on this from the Kotlin developers' side, and the issue KT-34534 seems to be a bit of a hot potato, at least right now.