rolang / dumbo

Simple database migration tool for Postgres with skunk on JVM and Native
MIT License
24 stars 3 forks source link

Consider replacing Console[F] with log4cats #36

Open colinpilloud opened 3 months ago

colinpilloud commented 3 months ago

Hi @rolang! Big fan of Flyway and the Typelevel ecosystem! Thanks for putting this open source project together.

I'm using dumbo for a pretty simple CLI application. It'd be great to be able to fine-tune the text output from dumbo that's currently produced via Console[F].println using a logging framework; as a dependency it'd be even better if it leveraged SLF4J via something like log4cats-slf4j.

I'd be happy to take a crack at this myself if it's something you'd be interested in!

rolang commented 3 months ago

Hi @colinpilloud ! Right, I guess logging could use a better solution. For now I kept Console[F] because skunk currently also requires a console instance for logging, so you need one anyway. Can't tell at the moment why skunk is not using any logging APIs/frameworks.

You can actually still provide a custom Console[F] instance which can use any logger underneath. I just created a branch with an example module on how you can get the logging via log4cats with slf4j and logback ExampleSlf4jLogback.scala (if you want to try you can checkout the branch and run docker compose up -d && sbt exampleLog4Cats/run). Would that suffice for your use case?

Good thing is that skunk logs will also get formatted using the same logger. One problem with that I guess is that you can't configure the logging for skunk and dumbo separately (e.g. keep dumbo logs, but mute skunk ones). I am not sure it's worth it to add dependencies on logging frameworks for the few info/debug logs done from the same class in dumbo :thinking:. One could maybe create a simple Logger interface like

package dumbo

trait Logger[F[_]] {
  def info(message: String): F[Unit]
  def error(message: String): F[Unit]
  // add more when needed
}

for now and require that along with Console[F] for skunk. One can then provide this logger instance using any logger underneath for dumbo and a separate console instance for skunk. I'd rather consider dependency on log4cats if it gets more complex where a LoggerFactory would be useful or skunk is going to add it to the core.

colinpilloud commented 3 months ago

Very useful example and will absolutely suffice for my needs! I thought something like this should be possible but couldn't quite fit the pieces together, so I appreciate you taking the time to put this together.

The Console[F] requirement from skunk is super interesting; I would love to research more about how skunk suggests approaching logging for both libraries and apps and how other developers are doing things!

This probably doesn't need to remain open as an issue; I defer to you on the matter, although either way I appreciate your prompt reply!

rolang commented 3 months ago

@colinpilloud just found that there was a conversation initiated on skunk discord channel about logging: link to message. Sounds like the plan is to use otel4s instead of log4cats.

I'd keep the issue open for now to have a think about it and gather ideas.