Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(rt): slf4j 1.x compatability #963

Merged
merged 6 commits into from
Sep 27, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .changes/d7863388-cf31-44d7-b613-c2376e853b97.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"id": "d7863388-cf31-44d7-b613-c2376e853b97",
"type": "bugfix",
"description": "Provide SLF4J 1.x compatible fallback implementation",
"issues": [
"awslabs/aws-sdk-kotlin#993"
]
}
2 changes: 2 additions & 0 deletions gradle/libs.versions.toml
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ okhttp-version = "5.0.0-alpha.11"
okio-version = "3.3.0"
otel-version = "1.27.0"
slf4j-version = "2.0.6"
slf4j-v1x-version = "1.7.36"
crt-kotlin-version = "0.7.1"

# codegen
Expand Down Expand Up @@ -51,6 +52,7 @@ okhttp-coroutines = { module = "com.squareup.okhttp3:okhttp-coroutines", version
opentelemetry-api = { module = "io.opentelemetry:opentelemetry-api", version.ref = "otel-version" }
opentelemetry-sdk-testing = {module = "io.opentelemetry:opentelemetry-sdk-testing", version.ref = "otel-version" }
slf4j-api = { module = "org.slf4j:slf4j-api", version.ref = "slf4j-version" }
slf4j-api-v1x = { module = "org.slf4j:slf4j-api", version.ref = "slf4j-v1x-version" }
slf4j-simple = { module = "org.slf4j:slf4j-simple", version.ref = "slf4j-version" }
crt-kotlin = { module = "aws.sdk.kotlin.crt:aws-crt-kotlin", version.ref = "crt-kotlin-version" }

Expand Down
2 changes: 1 addition & 1 deletion runtime/observability/logging-slf4j2/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0
*/
description = "Logging provider based on SLF4J 2"
description = "Logging provider based on SLF4J 2.x"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: This description is no longer fully accurate. It's based on 2.x unless it's not.

extra["displayName"] = "Smithy :: Kotlin :: Observability :: SLF4J2 binding"
extra["moduleName"] = "aws.smithy.kotlin.runtime.telemetry"

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0
*/
package aws.smithy.kotlin.runtime.telemetry.logging.slf4j

import aws.smithy.kotlin.runtime.telemetry.logging.LogLevel
import aws.smithy.kotlin.runtime.telemetry.logging.Logger

/**
* Common functionality across SLF4J 1.x and 2.x
* @param delegate the underlying slf4j logger instance
*/
internal abstract class AbstractSlf4jLoggerAdapter(protected val delegate: org.slf4j.Logger) : Logger {
override fun trace(t: Throwable?, msg: () -> String) {
if (!isEnabledFor(LogLevel.Trace)) return
if (t != null) {
delegate.trace(msg(), t)
} else {
delegate.trace(msg())
}
}
override fun debug(t: Throwable?, msg: () -> String) {
if (!isEnabledFor(LogLevel.Debug)) return
if (t != null) {
delegate.debug(msg(), t)
} else {
delegate.debug(msg())
}
}
override fun info(t: Throwable?, msg: () -> String) {
if (!isEnabledFor(LogLevel.Info)) return
if (t != null) {
delegate.info(msg(), t)
} else {
delegate.info(msg())
}
}
override fun warn(t: Throwable?, msg: () -> String) {
if (!isEnabledFor(LogLevel.Warning)) return
if (t != null) {
delegate.warn(msg(), t)
} else {
delegate.warn(msg())
}
}
override fun error(t: Throwable?, msg: () -> String) {
if (!isEnabledFor(LogLevel.Error)) return
if (t != null) {
delegate.error(msg(), t)
} else {
delegate.error(msg())
}
}
override fun isEnabledFor(level: LogLevel): Boolean = when (level) {
LogLevel.Trace -> delegate.isTraceEnabled
LogLevel.Debug -> delegate.isDebugEnabled
LogLevel.Info -> delegate.isInfoEnabled
LogLevel.Warning -> delegate.isWarnEnabled
LogLevel.Error -> delegate.isErrorEnabled
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0
*/
package aws.smithy.kotlin.runtime.telemetry.logging.slf4j

import aws.smithy.kotlin.runtime.telemetry.context.Context
import aws.smithy.kotlin.runtime.telemetry.logging.LogLevel
import aws.smithy.kotlin.runtime.telemetry.logging.LogRecordBuilder
import aws.smithy.kotlin.runtime.telemetry.logging.LoggerProvider
import org.slf4j.MDC

private val noOpLogRecordBuilder = LoggerProvider.None.getOrCreateLogger("NoOpLogger").atLevel(LogLevel.Error)

/**
* SLF4J 1.x based logger
*/
internal class Slf4j1xLoggerAdapter(logger: org.slf4j.Logger) : AbstractSlf4jLoggerAdapter(logger) {
override fun atLevel(level: LogLevel): LogRecordBuilder = if (isEnabledFor(level)) {
Slf4j1xLogRecordBuilderAdapter(this, level)
} else {
noOpLogRecordBuilder
}
}

private class Slf4j1xLogRecordBuilderAdapter(
private val delegate: Slf4j1xLoggerAdapter,
private val level: LogLevel,
) : LogRecordBuilder {

private var msg: (() -> String)? = null
private var cause: Throwable? = null
private val kvp by lazy { mutableMapOf<String, Any>() }
override fun setCause(ex: Throwable) {
cause = ex
}

override fun setMessage(message: String) {
msg = { message }
}

override fun setMessage(message: () -> String) {
msg = message
}

override fun setKeyValuePair(key: String, value: Any) {
kvp[key] = value
}

override fun setContext(context: Context) {
// TODO - add a way to get the current trace context and set the key/value pair on it?
}
Comment on lines +50 to +52
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Could be moved down to AbstractSlf4jLoggerAdapter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately it can't because this is on the LogRecordBuilder implementation which is the thing that differs between the two.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right, sorry I was thinking this was still in Slf4j1xLoggerAdapter. Nvm.


override fun emit() {
val message = requireNotNull(msg) { "no message provided to LogRecordBuilder" }
val logMethod = when (level) {
LogLevel.Error -> delegate::error
LogLevel.Warning -> delegate::warn
LogLevel.Info -> delegate::info
LogLevel.Debug -> delegate::debug
LogLevel.Trace -> delegate::trace
}

if (kvp.isNotEmpty()) {
val prevCtx = MDC.getCopyOfContextMap()
try {
kvp.forEach { (k, v) ->
MDC.put(k, v.toString())
}
logMethod(cause, message)
} finally {
MDC.setContextMap(prevCtx)
}
} else {
logMethod(cause, message)
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0
*/
package aws.smithy.kotlin.runtime.telemetry.logging.slf4j

import aws.smithy.kotlin.runtime.telemetry.context.Context
import aws.smithy.kotlin.runtime.telemetry.logging.LogLevel
import aws.smithy.kotlin.runtime.telemetry.logging.LogRecordBuilder
import org.slf4j.event.Level
import org.slf4j.spi.LoggingEventBuilder

/**
* SLF4J 2.x based logger
*/
internal class Slf4j2xLoggerAdapter(logger: org.slf4j.Logger) : AbstractSlf4jLoggerAdapter(logger) {
override fun atLevel(level: LogLevel): LogRecordBuilder =
Slf4j2xLogRecordBuilderAdapter(delegate.atLevel(level.slf4jLevel))
}

private class Slf4j2xLogRecordBuilderAdapter(
private val delegate: LoggingEventBuilder,
) : LogRecordBuilder {
override fun setCause(ex: Throwable) {
delegate.setCause(ex)
}

override fun setMessage(message: String) {
delegate.setMessage(message)
}

override fun setMessage(message: () -> String) {
delegate.setMessage(message)
}

override fun setKeyValuePair(key: String, value: Any) {
delegate.addKeyValue(key, value)
}

override fun setContext(context: Context) {
// TODO - add a way to get the current trace context and set the key/value pair on it?
}

override fun emit() = delegate.log()
}

private val LogLevel.slf4jLevel: org.slf4j.event.Level
get() = when (this) {
LogLevel.Error -> Level.ERROR
LogLevel.Warning -> Level.WARN
LogLevel.Info -> Level.INFO
LogLevel.Debug -> Level.DEBUG
LogLevel.Trace -> Level.TRACE
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,73 +5,24 @@

package aws.smithy.kotlin.runtime.telemetry.logging.slf4j

import aws.smithy.kotlin.runtime.telemetry.context.Context
import aws.smithy.kotlin.runtime.telemetry.logging.*
import org.slf4j.LoggerFactory
import org.slf4j.event.Level
import org.slf4j.spi.LoggingEventBuilder

/**
* SLF4J 2 based logger provider
* SLF4J based logger provider
*/
public object Slf4jLoggerProvider : LoggerProvider {
override fun getOrCreateLogger(name: String): Logger {
val sl4fjLogger = LoggerFactory.getLogger(name)
return Slf4JLoggerAdapter(sl4fjLogger)
}
}

private class Slf4JLoggerAdapter(private val delegate: org.slf4j.Logger) : Logger {
private fun logWith(t: Throwable?, msg: () -> String, builder: LoggingEventBuilder) =
builder.setMessage(msg)
.apply {
if (t != null) {
setCause(t)
}
}.log()
override fun trace(t: Throwable?, msg: () -> String) = logWith(t, msg, delegate.atTrace())
override fun debug(t: Throwable?, msg: () -> String) = logWith(t, msg, delegate.atDebug())
override fun info(t: Throwable?, msg: () -> String) = logWith(t, msg, delegate.atInfo())
override fun warn(t: Throwable?, msg: () -> String) = logWith(t, msg, delegate.atWarn())
override fun error(t: Throwable?, msg: () -> String) = logWith(t, msg, delegate.atError())
override fun isEnabledFor(level: LogLevel): Boolean =
delegate.isEnabledForLevel(level.slf4jLevel)

override fun atLevel(level: LogLevel): LogRecordBuilder =
Slf4jLogRecordBuilderAdapter(delegate.atLevel(level.slf4jLevel))
}

private class Slf4jLogRecordBuilderAdapter(
private val delegate: LoggingEventBuilder,
) : LogRecordBuilder {
override fun setCause(ex: Throwable) {
delegate.setCause(ex)
}

override fun setMessage(message: String) {
delegate.setMessage(message)
private val slf4jLoggerAdapter: (org.slf4j.Logger) -> Logger = try {
Class.forName("org.slf4j.spi.LoggingEventBuilder")
::Slf4j2xLoggerAdapter
} catch (ex: ClassNotFoundException) {
LoggerFactory.getLogger(Slf4jLoggerProvider::class.java).warn("falling back to SLF4J 1.x compatible binding")
::Slf4j1xLoggerAdapter
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style: May be clearer as a function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not opposed but can you clarify what you are referring to? What would you prefer to see here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A class-level val that returns a function is more complex to read than simply a fun:

private fun slf4jLoggerAdapter(delegate: org.slf4j.Logger) = try {
    Class.forName("org.slf4j.spi.LoggingEventBuilder")
    Slf4j2xLoggerAdapter(delegate)
} catch (_: ClassNotFoundException) {
    LoggerFactory.getLogger(Slf4jLoggerProvider::class.java).warn("falling back to SLF4J 1.x compatible binding")
    Slf4j1xLoggerAdapter(delegate)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying to avoid invoking the class loader on every logger instantiation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could refactor it though to something like:

private val slf4j2Found = try {
    Class.forName("org.slf4j.spi.LoggingEventBuilder")
    true
} catch(_: ClassNotFoundException) {
     LoggerFactory.getLogger(Slf4jLoggerProvider::class.java).warn("falling back to SLF4J 1.x compatible binding")
    false  
}

   override fun getOrCreateLogger(name: String): Logger {
        val sl4fjLogger = LoggerFactory.getLogger(name)
        return if (slf4j2Found) { 
             Slf4j2xLoggerAdapter(slf4jLogger)
        } else {
             Slf4j1xLoggerAdapter(slf4jLogger)
        }
    }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right. I didn't think about caching the class loader invocation. OK, your original code works better then.


override fun setMessage(message: () -> String) {
delegate.setMessage(message)
}

override fun setKeyValuePair(key: String, value: Any) {
delegate.addKeyValue(key, value)
}

override fun setContext(context: Context) {
// TODO - add a way to get the current trace context and set the key/value pair on it?
override fun getOrCreateLogger(name: String): Logger {
val sl4fjLogger = LoggerFactory.getLogger(name)
return slf4jLoggerAdapter(sl4fjLogger)
}

override fun emit() = delegate.log()
}

private val LogLevel.slf4jLevel: org.slf4j.event.Level
get() = when (this) {
LogLevel.Error -> Level.ERROR
LogLevel.Warning -> Level.WARN
LogLevel.Info -> Level.INFO
LogLevel.Debug -> Level.DEBUG
LogLevel.Trace -> Level.TRACE
}
2 changes: 2 additions & 0 deletions settings.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -78,3 +78,5 @@ include(":tests:benchmarks:serde-benchmarks")
include(":tests:compile")
include(":tests:codegen:paginator-tests")
include(":tests:codegen:waiter-tests")
include(":tests:integration:slf4j-1x-consumer")
include(":tests:integration:slf4j-2x-consumer")
44 changes: 44 additions & 0 deletions tests/integration/slf4j-1x-consumer/build.gradle.kts
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0
*/
import aws.sdk.kotlin.gradle.dsl.skipPublishing

plugins {
kotlin("jvm")
}

skipPublishing()

dependencies {
implementation(project(":runtime:observability:logging-slf4j2"))
implementation(libs.slf4j.api) {
version {
strictly(libs.versions.slf4j.v1x.version.get())
}
}
implementation(libs.slf4j.simple) {
version {
strictly(libs.versions.slf4j.v1x.version.get())
}
}

testImplementation(libs.junit.jupiter)
testImplementation(libs.kotlin.test)
testImplementation(libs.kotlin.test.junit5)
}

tasks.withType<org.jetbrains.kotlin.gradle.tasks.KotlinCompile> {
kotlinOptions.jvmTarget = "1.8"
}

tasks.test {
useJUnitPlatform()
testLogging {
events("passed", "skipped", "failed")
showStandardStreams = false
showStackTraces = true
showExceptions = true
exceptionFormat = org.gradle.api.tasks.testing.logging.TestExceptionFormat.FULL
}
}
Loading