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

Logback backend for kotlin-logging #452

Merged
merged 7 commits into from
Nov 3, 2024

Conversation

neeme-praks-sympower
Copy link
Contributor

@neeme-praks-sympower neeme-praks-sympower commented Oct 24, 2024

Proposal for #449
I'm not sure if this change requires kotlin-logging to always have Logback on runtime classpath or not (how eager is JVM classloading nowadays?) -- but we can figure that detail out later.

@oshai
Copy link
Owner

oshai commented Oct 25, 2024

Right now it fails on formatting, see how to fix spotless here: https://github.com/oshai/kotlin-logging/blob/master/CONTRIBUTING.md

In addition, since I want it to be both extended (with <class>, <method>, <line>, <file>) maybe we should wrap it with compilerOptions or internalCompilerData data class. I think it will be more obvious that this should not be set.

@neeme-praks-sympower
Copy link
Contributor Author

Thanks for the feedback! I'll get back to this next week.

@neeme-praks-sympower
Copy link
Contributor Author

I've now fixed formatting issues and introduced internalCompilerData field.

Copy link
Owner

@oshai oshai left a comment

Choose a reason for hiding this comment

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

lgtm, left some comments.

@@ -4,4 +4,13 @@ public class KLoggingEventBuilder {
public var message: String? = null
public var cause: Throwable? = null
public var payload: Map<String, Any?>? = null
public var internalCompilerData: InternalCompilerData? = null
Copy link
Owner

Choose a reason for hiding this comment

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

please document that this field is internal and should not be set

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added JavaDoc

internal fun jLogger(name: String): Logger =
logbackServiceProvider.loggerFactory.getLogger(name) as Logger

/** wrap java logger based on location awareness */
Copy link
Owner

Choose a reason for hiding this comment

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

actually here there is no difference in location awerness so this can be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed. And renamed methods and fields a bit.

@oshai
Copy link
Owner

oshai commented Nov 3, 2024

Can you please fix spotless again before I submit it?

@neeme-praks-sympower
Copy link
Contributor Author

Can you please fix spotless again before I submit it?

Fixed.

I would like to also include a test that would execute JulLoggerWrapperTest without Logback in the classpath -- to make sure that KLoggerFactory can access JulLoggerFactory without needing to load LogbackLoggerFactory (which depends on Logback).

I tried to play around with Gradle build file but my knowledge about Kotlin multiplatform (and Gradle) build system is limited -- maybe you can help out here?

@oshai oshai merged commit 1f9ecdd into oshai:master Nov 3, 2024
5 checks passed
@oshai
Copy link
Owner

oshai commented Nov 3, 2024

Can you please fix spotless again before I submit it?

Fixed.

I would like to also include a test that would execute JulLoggerWrapperTest without Logback in the classpath -- to make sure that KLoggerFactory can access JulLoggerFactory without needing to load LogbackLoggerFactory (which depends on Logback).

I tried to play around with Gradle build file but my knowledge about Kotlin multiplatform (and Gradle) build system is limited -- maybe you can help out here?

Thanks! I merged the PR and just now see this message. I think it will be difficult to do tests with / without logback in classpath. I also don't think gradle is very good at this. I think we can go without such test.

@neeme-praks-sympower
Copy link
Contributor Author

I think we can go without such test.

Just to be sure, I ran a manual test with a simple project on my machine -- seems to work just fine (using -Dkotlin-logging-to-jul=true and without Logback in the classpath).

Thanks!

@neeme-praks-sympower neeme-praks-sympower deleted the logback-delegate branch November 3, 2024 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants