Remove logger property

[t52053518]

This logger property:

logging-log4j-kotlin/log4j-api-kotlin/src/main/kotlin/org/apache/logging/log4j/kotlin/LoggingFactory.kt

Line 37 in 95c7ef2

inline val <reified T> T.logger: KotlinLogger

Makes this unusable (we'll always end up using the getter with extra overhead):

    companion object {
        private val logger = logger()
    }

Not everyone wants to use LOGGER as a name.

Please revert or move it somewhere else so it's not imported.

[vy]

@rocketraman, @jvz, would you mind helping with this one, please?

[rocketraman]

This was added via #29 by @jvz .

I have to admit, I'm not fond of the logger property either, as I prefer client code to be more explicit about loggers. In addition, the logger property introduces a function call on every access, and the user might not be cognizant of the performance impact of this, which is real. According to our benchmarks, companionObjectLookupInterfaceDirect and companionObjectExtensionPropertyFunctional which both use the logger property (and thereby the getter function) are 15-20 times slower than all the other methods:

Benchmark                                                    Mode  Cnt   Score   Error  Units
LoggingBenchmark.companionObjectExtensionPropertyDirect      avgt   10   0.826 ± 0.028  ns/op
LoggingBenchmark.companionObjectExtensionPropertyFunctional  avgt   10   0.817 ± 0.003  ns/op
LoggingBenchmark.companionObjectKotlinLoggerDirect           avgt   10   0.817 ± 0.008  ns/op
LoggingBenchmark.companionObjectKotlinLoggerFunctional       avgt   10   0.814 ± 0.004  ns/op
LoggingBenchmark.companionObjectLog4jLoggerDirect            avgt   10   0.603 ± 0.005  ns/op
LoggingBenchmark.companionObjectLog4jLoggerFunctional        avgt   10   0.604 ± 0.005  ns/op
LoggingBenchmark.companionObjectLookupInterfaceDirect        avgt   10  13.736 ± 0.021  ns/op
LoggingBenchmark.companionObjectLookupInterfaceFunctional    avgt   10  13.730 ± 0.032  ns/op
LoggingBenchmark.topLevelLoggerWithContextLookupDirect       avgt   10   0.814 ± 0.004  ns/op
LoggingBenchmark.topLevelLoggerWithContextLookupFunctional   avgt   10   0.815 ± 0.003  ns/op
LoggingBenchmark.topLevelNamedLoggerDirect                   avgt   10   0.813 ± 0.003  ns/op
LoggingBenchmark.topLevelNamedLoggerFunctional               avgt   10   0.814 ± 0.009  ns/op

I would be in favor of adding an _logger (or equivalently less discoverable name) property with an equivalent implementation, deprecating the logger property now (with appropriate documentation), and then removing logger in the next major version. That gives people who really want it the ability to use it via _logger, but users hopefully won't end up with it by mistake, and it won't take precedence over explicit logger declarations as noted by OP.

I'm also ok with removing it completely with no replacement (with the appropriate deprecation cycle). Users would still have the ability to pull it back in explicitly by extending the Logger interface (the companionObjectExtensionPropertyFunctional above).

But would like to hear opinions from @jvz since he was the one that added this originally.

[rocketraman]

Looks like the discussion that led up to this is at #20. Looks like there wasn't a strong reason to add it -- just that it felt intuitive. That reinforces my previous thoughts.

@sschuberth are you a user of this property? Would you be opposed to its renaming and/or removal, and if so, can you explain why? Would using the Logger interface explicitly be an acceptable alternative? Thanks!

[sschuberth]

@sschuberth are you a user of this property?

Yes, we heavily use it throughout the code base of https://github.com/oss-review-toolkit/ort.

Would you be opposed to its renaming and/or removal and if so, can you explain why?

Yes, I would. While renaming would be somewhat feasible in the sense that search & replace is easy, I dislike a non-readable name like _logger.

Would using the Logger interface explicitly be an acceptable alternative?

Not really, as for me it's the opposite of your "I prefer client code to be more explicit about loggers" statement from above: IMO logging should be available by default everywhere, to ease and encourage making use of it. Classes should not need to "implement" an interface just to do logging, as that seems to violate best practices by introducing tight coupling for something that's not really a capability of the implementing class.

To me, convenience outweighs performance matters here. Also, do your numbers take the caching done by cachedLoggerOf() into account? Would inlining that function help?

Generally, if people for whatever reason want

companion object {
    private val logger = logger()
}

they could simply use a FQN like

companion object {
    private val logger = org.apache.logging.log4j.kotlin.logger("name")
}

and not import org.apache.logging.log4j.kotlin.logger, no?

[rocketraman]

Also, do your numbers take the caching done by cachedLoggerOf() into account?

Yes they do.

Would inlining that function help?

My benchmark shows it doesn't help, which makes sense because the "slow" call here is the Map cache lookup, not that top-level function call.

LoggingBenchmark.companionObjectLookupInterfaceDirect        avgt   10  13.769 ± 0.050  ns/op
LoggingBenchmark.companionObjectLookupInterfaceFunctional    avgt   10  13.847 ± 0.176  ns/op

they could simply use a FQN like

companion object {
private val logger = org.apache.logging.log4j.kotlin.logger("name")
}

Not really -- in addition to being ugly because of the FQN, this variant requires specifying the name as a parameter. People generally want to use the T.logger() receiver variant.

To me, convenience outweighs performance matters here.

This is fair for many, if not most, use cases.

How about this option: we move this property into an optional module log4j-api-kotlin-autoprop. People who want to use the property just have to add that module into their dependencies. Would that work for you?

[sschuberth]

How about this option: we move this property into an optional module log4j-api-kotlin-autoprop. People who want to use the property just have to add that module into their dependencies. Would that work for you?

Yes, that would work, I wouldn't mind adding a new (tiny) dependency here.

But wouldn't moving it to a separate package within the same module suffice?

[rocketraman]

How about this option: we move this property into an optional module log4j-api-kotlin-autoprop. People who want to use the property just have to add that module into their dependencies. Would that work for you?

Yes, that would work, I wouldn't mind adding a new (tiny) dependency here.

But wouldn't moving it to a separate package within the same module suffice?

Yeah that would work too. Its a bigger change for current users but easily dealt with via search/replace or the standard Kotlin/IDE deprecation assistance.

[sschuberth]

Or maybe renaming the properly would really not that bad after all, as users could easily account for it with as import alias.

[rocketraman]

Or maybe renaming the properly would really not that bad after all, as users could easily account for it with as import alias.

I think I would lean towards changing its package because the IDE doesn't help as much with import aliases, and _logger is not as discoverable in completions.

[rocketraman]

I need to add a changelog to it, but proposed draft PR if anyone wants to review: #137

Also, the ReplaceWith annotation doesn't work correctly for this case. If anyone knows how to get the IDE to do that properly, let me know.

[t52053518]

Thanks for taking this on!

---

What'd be the ETA for 2 major releases if you're going with the deprecated approach?

That said, I don't think you can fully achieve this with ReplaceWith, and I think this might create a new bug in the intermediary build, if this is a valid use case:

1. Someone wants to use logger property, so they import the new ....autoprop.logger
2. But they also want to use something else from the old file, e.g. loggerOf() so they import the old ....logger as well
3. Compiler will fail on overload resolution ambiguity with the duplicate logger property

---

The performance impact is real. It causes a method invocation every time. Larger projects easily have more than 100 classes, further causing LRU cache misses. Finally, this cache is synchronized (usual bad), and causes thread pinning with coroutines (bad for a Kotlin-specific library since IntelliJ/Kotlin advocates for them).

I'd argue that performance is much more important for such a core library than convenience. Extension methods (if even needed, as you can see they're like cancer invading everything in scope, e.g. 123.logger.info { ... }) can be easily added by the caller even prior to 1.3.0, meanwhile we can't take this code out (apart from forking, which we're doing now).

Finally, you might even argue that this (simply updating to 1.3.0) is already a mini-regression for folks who have Lint fail on unused variables.

As much as you'd like to avoid breaking, I wonder if it would be better to do it in one go.

[sschuberth]

Slightly off-topic, but regarding the performance impact and combining it with convenience, I wonder whether we could have an annotation / Kotlin compiler plugin that auto-generates code for classes that should support logging. E.g. annotating a class with @Logger would then hard-code a logger property to the class with proper naming and without any cache lookup / additional function call.

[t52053518]

They have this nice 1-liner: Logging but just before I suggested it I decided to take a look, and heh, it's cached too...

Also on second read maybe disregard my 1-2-3 example from previous comment, I haven't actually tried that and it sounds a bit too C like...