-
Notifications
You must be signed in to change notification settings - Fork 28
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
Conversation
Awesome, this issue was blocking us from updating past 0.28.1. |
description = "Logging provider based on SLF4J 2" | ||
description = "Logging provider based on SLF4J 2.x" |
There was a problem hiding this comment.
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.
override fun setContext(context: Context) { | ||
// TODO - add a way to get the current trace context and set the key/value pair on it? | ||
} |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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 | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
}
}
There was a problem hiding this comment.
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.
SonarCloud Quality Gate failed. 0 Bugs No Coverage information Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
description = "Logging provider based on SLF4J 2" | ||
extra["displayName"] = "Smithy :: Kotlin :: Observability :: SLF4J2 binding" | ||
description = "Logging provider based on SLF4J" | ||
extra["displayName"] = "Smithy :: Kotlin :: Observability :: SLF4J binding" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the directory logging-slf4j2
also be renamed to drop the 2
?
Issue #
fixes awslabs/aws-sdk-kotlin#993
Description of changes
This PR provides automatic fallback to SLF4J 1.x compatible logging implementation when 2.x API can't be loaded. 2.x is binary compatible with 1.x but it does contain some new APIs that aren't available in 1.x (namely the fluent builder APIs). We could just map everything to a 1.x compatible API but it's not guaranteed to be the best mapping since underlying logging implementations may be more efficient than our compatibility layer can achieve.
Alternative
I was able to solve this differently by creating a separate 1.x implementation and then using Gradle component metadata rules to replace the underlying 2.x implementation we ship by default. I decided that fallback by default is a better customer experience though.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.