Adds a StructuredLogger based on cats.mtl.Ask #900
Conversation
|
The flake.* files are not germane to this PR, and could be spun off into a different one. They were germane to me getting a working Metals. |
| @@ -47,7 +48,8 @@ lazy val core = crossProject(JSPlatform, JVMPlatform, NativePlatform) | |||
| name := "log4cats-core", | |||
| libraryDependencies ++= Seq( | |||
| "org.typelevel" %%% "cats-core" % catsV, | |||
| "org.typelevel" %%% "cats-effect-std" % catsEffectV | |||
| "org.typelevel" %%% "cats-effect-std" % catsEffectV, | |||
| "org.typelevel" %%% "cats-mtl" % catsMtlV | |||
There was a problem hiding this comment.
This dependency will be part of cats-effect-3.6, so I'm not feeling guilty for introducing it to core. Otherwise, this could be a log4cats-mtl module.
| F.flatMap(ask.ask)(askCtx => logger.trace(askCtx, t)(message)) | ||
|
|
||
| override def trace(ctx: Map[String, String])(msg: => String): F[Unit] = | ||
| F.flatMap(ask.ask)(askCtx => logger.trace(askCtx ++ ctx)(msg)) |
There was a problem hiding this comment.
Performance concerns, not yet proven by benchmarking:
- We pay to ask and flatMap when the level is disabled.
- In the methods with a
ctxt, we additionally pay to concatenate askCtx and ctx when the level is disabled.
An underlying SelfAwareLogger could help, but then we'd be an AskSelfAwareStructuredLogger. The traits aren't composing particularly well.
We could further optimize by getting into Slf4jLogger, but that may further contribute to the combinatorial explosion.
There was a problem hiding this comment.
as annoying as it would be, I don't think a single "duplicate" for SelfAwareStructuredLogger is too awful
|
|
||
| private[log4cats] class AskStructuredLogger[F[_]](logger: StructuredLogger[F])(implicit | ||
| F: Monad[F], | ||
| ask: Ask[F, Map[String, String]] |
There was a problem hiding this comment.
The Map returned here could be a view (say, from otel4s), but it needs to have a fast ++ to another Map.
| import cats.Monad | ||
| import cats.mtl.Ask | ||
|
|
||
| private[log4cats] class AskStructuredLogger[F[_]](logger: StructuredLogger[F])(implicit |
There was a problem hiding this comment.
I know it's out of character for this repo, but could you add some docs and tests?
There was a problem hiding this comment.
Definitely. I wanted to iterate on the approach a little bit first before investing too much in that.
This should work for non-slf4j loggers, with the aforementioned performance concerns. Is it worth trying to do both, or do we think it's better to focus on the slf4j that everyone uses in practice?
There was a problem hiding this comment.
I don't think the performance can be meaningfully worse than the existing terrible performance of every call to addContext adding another Map concatenation for every log operation
| @@ -33,10 +34,10 @@ object Slf4jLogger extends Slf4jLoggerCompat { | |||
| getLoggerFromSlf4j[F](org.slf4j.LoggerFactory.getLogger(clazz)) | |||
|
|
|||
| def getLoggerFromSlf4j[F[_]: Sync](logger: JLogger): SelfAwareStructuredLogger[F] = | |||
| new Slf4jLoggerInternal.Slf4jLogger(logger, Sync.Type.Delay) | |||
| new Slf4jLoggerInternal.Slf4jLogger(logger, Sync.Type.Delay, Applicative[F].pure(Map.empty)) | |||
There was a problem hiding this comment.
This (and line 40) wire nothing in yet. We'd need a parallel set of Ask or Local based constructors here.
| final class Slf4jLogger[F[_]]( | ||
| val logger: JLogger, | ||
| sync: Sync.Type = Sync.Type.Delay, | ||
| defaultCtx: F[Map[String, String]] |
There was a problem hiding this comment.
By getting the default context into the logger, as an effect, we are now able to avoid the asking and concatenation of the context map when the log is disabled. But this is a bit less conducive to the middleware approach: we don't want addContext to return a ModifiedContextStructuredLogger anymore.
No description provided.