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

Separate Logger and IO-local context to allow Logger to be a ProFunctor #130

Open
hejfelix opened this issue Feb 7, 2023 · 3 comments
Open
Labels
enhancement New feature or request investigation Needs a POC

Comments

@hejfelix
Copy link
Collaborator

hejfelix commented Feb 7, 2023

Currently, creating a logger is effectful because we create an IOLocal context.

object DefaultLogger:
  def makeIo(output: Output[IO], outputs: Output[IO]*)(using Clock[IO], Printer, Filter): IO[DefaultLogger[IO]] =
    for given StringLocal[IO] <- ioStringLocal
    yield new DefaultLogger[IO](output, outputs*)
end DefaultLogger

The actual interface requires a local instance to allow adding context to effects:

trait Logger[F[_]]:

  val stringLocal: StringLocal[F]

  def doLog(level: LogLevel, message: String)(using LogInfo): F[Unit]
...
end Logger

This in turn allows us to scope context to an effect value whenever we have a logger in scope:

def program(using Logger[IO]): IO[Unit] = 
  for
    _ <- Logger[IO].debug("This is some debug")
    _ <- Logger[IO].info("HEY!")
    _ <- Logger[IO].warn("I'm warning you")
    _ <- Logger[IO].error("I give up")
  yield ()

import Logger.*
val mainWithContext: IO[Unit] = 
  for
    given Logger[IO]  <- DefaultLogger.makeIo(consoleOutput)
    _                 <- program.withLogContext("trace-id", "4d334544-6462-43fa-b0b1-12846f871573")
    _                 <- Logger[IO].info("Now the context is gone")
  yield ()

This is very convenient and easy to use. There are 2 main drawbacks:

  1. The constructor of the logger is now effectful
  2. It's difficult to construct a correct instance of Profunctor for this interface

Challenges

If the Logger no longer has a reference to the local context, we place the burden on the user to pass around this context. It's not clear if there's a good solution to avoid this (maybe Odin has solved this?).

Proposal

Simplify the interface to something akin to:

case class LogLine(level: LogLevel, message: String, context: Map[String,String], info: LogInfo)

trait Logger[F[_]]:
  def doLog(line: LogLine): F[Unit]
end Logger
@hejfelix hejfelix added investigation Needs a POC enhancement New feature or request labels Feb 7, 2023
@dimitarg
Copy link

dimitarg commented Nov 7, 2024

maybe Odin has solved this?

https://github.com/valskalla/odin?tab=readme-ov-file#contextual-effects

See

https://github.com/valskalla/odin/blob/db4444fe4efcb5c497d4e23bdf9bbcffd27269c2/core/src/main/scala/io/odin/loggers/ContextualLogger.scala#L55

et al.

So, the solution is just Ask[F, Env], which ends up being satisfied some variation / stack on top of ReaderT for a given user program,

I think (could be mistaken) this means that you cannot (lawfully) log with context in only IO. Personally I don't find this a worthy goal anyways because I find that

  • It's easier to reason about ReaderT in the face of concurrency / parallelism / racing / cancellation / streaming than it is to reason about IOLocal;
  • The programs I write need to be materialised in ReaderT anyways, because of distributed tracing.

@hejfelix
Copy link
Collaborator Author

hejfelix commented Nov 7, 2024

It's an interesting approach – but I don't want to force users to go down the road of monad transformers, and we don't currently have a dependency on mtl.

@dimitarg
Copy link

dimitarg commented Jan 10, 2025

Hey,

I was rereading this and, unrelated to the issue here, I was wrong in hinting Ask is a requirement. If you read carefully the documentation I linked, they say at the end

Odin automatically derives required type classes for each type F[_] that has Ask[F, E] defined, or in other words for all the types that allow F[A] => F[E].

If this constraint isn't satisfied, it's required to manually provide an instance for WithContext type class:

/**
  * Resolve context stored in `F[_]` effect
  */
trait WithContext[F[_]] {
  def context: F[Map[String, String]]
}

, and that one should be able to admit an IOLocal implementation, I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request investigation Needs a POC
Projects
None yet
Development

No branches or pull requests

2 participants