typelevel / log4cats

Logging Tools For Interaction with cats-effect
https://typelevel.org/log4cats/
Apache License 2.0
402 stars 74 forks source link

Unify behaviour of Logger and LoggerInterpolator #848

Closed sanssushi closed 5 months ago

sanssushi commented 5 months ago

In MessageLogger the message is passed by-name, in LoggerInterpolator by-value.

Is there a reason behind that?

If not, should we change the LoggerInterpolator like this?

  object syntax {
    implicit final class LoggerInterpolator(private val sc: StringContext) extends AnyVal {
      def error[F[_]](message: => Any*)(implicit logger: Logger[F]): F[Unit] =
        logger.error(sc.s(message: _*))

      def warn[F[_]](message: => Any*)(implicit logger: Logger[F]): F[Unit] =
        logger.warn(sc.s(message: _*))

      def info[F[_]](message: => Any*)(implicit logger: Logger[F]): F[Unit] =
        logger.info(sc.s(message: _*))

      def debug[F[_]](message: => Any*)(implicit logger: Logger[F]): F[Unit] =
        logger.debug(sc.s(message: _*))

      def trace[F[_]](message: => Any*)(implicit logger: Logger[F]): F[Unit] =
        logger.trace(sc.s(message: _*))
    }
  }

I can create a pull request if this makes sense.

danicheg commented 5 months ago

I doubt that passing these arguments by name into the String Interpolation makes much sense there, since the call of scala.StringContext#s would lead to macro expansion (or even that call might be replaced to something else by scalac accordingly to scaladoc).

sanssushi commented 5 months ago

I was stumbling upon this when rewriting logger.debug(...) to debug"..." and got very different results, for example

def run: IO[Unit] =
  doSomething[IO] *> debug"finished at: ${Clock.systemUTC().instant()}"

logs the time before doSomething runs vs.

def run: IO[Unit] =
  doSomething[IO] *> logger.debug(s"finished at: ${Clock.systemUTC().instant()}")

which logs the time after doSomething is finished.

Another example:

def run: IO[Unit] =
  doSomething[IO] *> debug"this may fail: ${1 / 0}".recover(_ => ())

does not only not recover from a failing log message, but prevents doSomething from running at all vs.

def run: IO[Unit] =
  doSomething[IO] *> logger.debug(s"this may fail: ${1 / 0}").recover(_ => ())

which recovers from a failing log message (after running doSomething).

From a users perspective the deviation of behaviour seems rather odd. Shouldn't the syntax styles be usable interchangeably? I don't know about Scala macros, but the workaround from above worked for me so I thought why not post it here.

danicheg commented 5 months ago

Again, one cannot pass by-name arguments to the scala.StringContext#s since its implementation is a def-macro. I suggest you not pass anything side-effecting / non-referentially transparent to the logger methods, and you won't see any differences here.

sanssushi commented 5 months ago

I suggest you not pass anything side-effecting / non-referentially transparent to the logger methods, and you won't see any differences here.

Fair enough. Thanks!