Change logLevel to not touch simple-logger defaultLogLevel (#368)

Fix #331

> `Config.logLevel` / `DEVELOCITY_API_LOG_LEVEL` internally changes
`org.slf4j.simpleLogger.defaultLogLevel`, which changes level of all
loggers, not only the library's as the javadoc says. In practice, this
is an issue when the user is using `simple-logger` and using loggers (or
using other dependencies that use loggers), as changing
`Config.logLevel` / `DEVELOCITY_API_LOG_LEVEL` will unexpectedly affect
those loggers.

Author: gabrielfeo

library/src/main/kotlin/com/gabrielfeo/develocity/api/internal/LoggerFactory.kt

@@ -6,6 +6,7 @@ import kotlin.reflect.KClass
 
 internal interface LoggerFactory {
     fun newLogger(cls: KClass<*>): Logger
+    fun newLogger(name: String): Logger
 }
 
 internal class RealLoggerFactory(
@@ -17,11 +18,16 @@ internal class RealLoggerFactory(
         return org.slf4j.LoggerFactory.getLogger(cls.java)
     }
 
+    override fun newLogger(name: String): Logger {
+        setLogLevel()
+        return org.slf4j.LoggerFactory.getLogger(name)
+    }
+
     private fun setLogLevel() {
         System.setProperty(LOG_LEVEL_SYSTEM_PROPERTY, config.logLevel)
     }
 
     companion object {
-        const val LOG_LEVEL_SYSTEM_PROPERTY = "org.slf4j.simpleLogger.defaultLogLevel"
+        const val LOG_LEVEL_SYSTEM_PROPERTY = "org.slf4j.simpleLogger.log.com.gabrielfeo.develocity.api"
     }
 }

library/src/main/kotlin/com/gabrielfeo/develocity/api/internal/OkHttpClient.kt

@@ -13,6 +13,8 @@ import okhttp3.logging.HttpLoggingInterceptor.Level.BODY
 import java.time.Duration
 import org.slf4j.Logger
 
+private const val HTTP_LOGGER_NAME = "com.gabrielfeo.develocity.api.OkHttpClient"
+
 /**
  * Base instance just so that multiple created [Config]s will share resources by default.
  */
@@ -58,7 +60,7 @@ private fun OkHttpClient.Builder.addNetworkInterceptors(
     if (config.cacheConfig.cacheEnabled) {
         addNetworkInterceptor(buildCacheEnforcingInterceptor(config))
     }
-    val logger = loggerFactory.newLogger(HttpLoggingInterceptor::class)
+    val logger = loggerFactory.newLogger(HTTP_LOGGER_NAME)
     addNetworkInterceptor(HttpLoggingInterceptor(logger = logger::debug).apply { level = BASIC })
     addNetworkInterceptor(HttpLoggingInterceptor(logger = logger::trace).apply { level = BODY })
     // Add authentication after logging to prevent clients from leaking their access key

library/src/test/kotlin/com/gabrielfeo/develocity/api/OkHttpClientTest.kt

@@ -65,9 +65,25 @@ class OkHttpClientTest {
         assertTrue(client.readTimeoutMillis > defaultTimeout)
     }
 
+    @Test
+    fun `Logs under library package`() {
+        val loggerFactory = ProxyLoggerFactory(delegate = RealLoggerFactory(Config()))
+        buildClient(loggerFactory = loggerFactory)
+        loggerFactory.createdLoggers.let {
+            assertTrue(it.isNotEmpty())
+            it.forEach {
+                assertTrue(
+                    it.name.startsWith("com.gabrielfeo.develocity.api"),
+                    "Logger name '${it.name}' should start with 'com.gabrielfeo.develocity.api'"
+                )
+            }
+        }
+    }
+
     private fun buildClient(
         vararg envVars: Pair<String, String?>,
         clientBuilder: OkHttpClient.Builder? = null,
+        loggerFactory: LoggerFactory? = null,
     ): OkHttpClient {
         val fakeEnv = FakeEnv(*envVars)
         if ("DEVELOCITY_ACCESS_KEY" !in fakeEnv)
@@ -85,6 +101,6 @@ class OkHttpClientTest {
             null -> Config()
             else -> Config(clientBuilder = clientBuilder)
         }
-        return buildOkHttpClient(config, RealLoggerFactory(config))
+        return buildOkHttpClient(config, loggerFactory ?: RealLoggerFactory(config))
     }
 }

library/src/test/kotlin/com/gabrielfeo/develocity/api/internal/LoggerFactoryTest.kt

@@ -4,23 +4,37 @@ import kotlin.test.Test
 import com.gabrielfeo.develocity.api.Config
 import kotlin.test.AfterTest
 import kotlin.test.BeforeTest
-import kotlin.test.assertEquals
+import kotlin.test.assertTrue
+import kotlin.test.assertFalse
 
 class LoggerFactoryTest {
 
     private val logLevelProperty = RealLoggerFactory.LOG_LEVEL_SYSTEM_PROPERTY
+    private val defaultLogLevelProperty = "org.slf4j.simpleLogger.defaultLogLevel"
 
     @BeforeTest
     @AfterTest
     fun cleanup() {
         System.clearProperty(logLevelProperty)
+        System.clearProperty(defaultLogLevelProperty)
         env = FakeEnv("DEVELOCITY_URL" to "https://example.com/")
     }
 
     @Test
-    fun `Level always copied from`() {
-        val loggerFactory = RealLoggerFactory(Config(logLevel = "foo"))
-        loggerFactory.newLogger(LoggerFactoryTest::class)
-        assertEquals("foo", System.getProperty(logLevelProperty))
+    fun `Given custom log level set, loggers created with log level`() {
+        check(Config().logLevel != "trace") { "Precondition failed: default level is already trace" }
+        val loggerFactory = RealLoggerFactory(Config(logLevel = "trace"))
+        val logger = loggerFactory.newLogger(LoggerFactoryTest::class)
+        assertTrue(logger.isTraceEnabled)
+    }
+
+    @Test
+    fun `Given custom log level set, unrelated loggers unaffected`() {
+        val unrelatedLogger = org.slf4j.LoggerFactory.getLogger("foo.Bar")
+        check(!unrelatedLogger.isTraceEnabled) { "Precondition failed: unrelated logger is already trace" }
+        val loggerFactory = RealLoggerFactory(Config(logLevel = "trace"))
+        val logger = loggerFactory.newLogger(LoggerFactoryTest::class)
+        assertTrue(logger.isTraceEnabled)
+        assertFalse(unrelatedLogger.isTraceEnabled)
     }
 }

library/src/test/kotlin/com/gabrielfeo/develocity/api/internal/ProxyLoggerFactory.kt

@@ -0,0 +1,17 @@
+package com.gabrielfeo.develocity.api.internal
+
+import com.gabrielfeo.develocity.api.Config
+import kotlin.reflect.KClass
+
+internal class ProxyLoggerFactory(
+    private val delegate: LoggerFactory,
+) : LoggerFactory {
+
+    val createdLoggers: MutableList<org.slf4j.Logger> = mutableListOf()
+
+    override fun newLogger(cls: KClass<*>) = delegate.newLogger(cls)
+        .also { createdLoggers.add(it) }
+
+    override fun newLogger(name: String) = delegate.newLogger(name)
+        .also { createdLoggers.add(it) }
+}