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

[WIP] Use iolog4s macro-based implementation of slf4j interface #32

Closed

Conversation

lorandszakacs
Copy link
Member

@lorandszakacs lorandszakacs commented May 27, 2018

Rewrote the slf4j module to use the same macro-based implementation as [iolog4s](iolog4s: https://iolog4s.github.io/iolog4s/)

One (big) problem tough. Macros cannot expand if they're hidden behind abstract methods 😢. Which makes sense, but I never thought of, or ran into, the problem until now.

So the solution I stumbled upon is some sort of weird, super hacky, behind the scenes compromise. Have a look at this program:

object Main extends App {
  import io.chrisdavenport.log4cats.Logger
  import io.chrisdavenport.log4cats.slf4j.Slf4jLogger
  import cats.effect.IO

  val slf4jL: Slf4jLogger[IO] = Slf4jLogger.create[IO]
  val logger: Logger[IO] = slf4jL

  val app = for {
    _ <- logger.info("non-macro-based info <=> old log4cats")
    _ <- slf4jL.info("macro-based info     <=> iolog4s")
    _ <- slf4jL.info("silver-lining" -> "we also have @oleg-py's MDC")(
      "macro-based info     <=> iolog4s — YEY! no runtime crash"
    )
  } yield ()

  app.unsafeRunSync()
}

When the user references the logger through the Slf4jLogger[F] interface they get something equivalent to macro-based iolog4s. When they want to switch to the more abstract algebra of Logger[F] to write their own boilerplate saving algebra they lose macro-based optimization and they get something equivalent to the current log4cats implementation. This sounds super unorthodox and to be frowned upon from a pure FP perspective, but it kinda works without breaking stuff 🤷‍♂️

I created this PR as a starting point for a discussion on how to proceed. I'll keep hacking at it, maybe I find something better.

No longer relevant after second commit:

~~Thus, referencing a logger through its nice algebra defined in io.chrisdavenport.log4cats.Logger[F] is impossible. You always have to reference it through the subclass io.chrisdavenport.log4cats.slf4j.Slf4jLogger[F] which implements Logger[F], but this would become very dangerous very quickly.

object Main extends App {

  import io.chrisdavenport.log4cats.Logger
  import io.chrisdavenport.log4cats.slf4j.Slf4jLogger

  import cats.effect.IO

  //attempting to log something fails at runtime with ???
  val logger: Logger[IO] = Slf4jLogger.create[IO]
 
  //works properly
  //val logger: Slf4jLogger[IO] = Slf4jLogger.create[IO].asInstanceOf[Slf4jLogger[IO]]

  val app = for {
    _ <- logger.debug("--- debugging ---")
    _ <- logger.info("--- informing ---")
  } yield ()

  app.unsafeRunSync()

}

We can provide a trivial translation from iolog4s to the Logger interface, but not sure how useful that is. Since the utility of iolog4s derives from its macro based that saves a few captures of variables in closures and whatnot. And your current solution for working with a sfl4j backend is better for log4cats because it doesn't render the algebra of Logger[F] dangerous.~~

The log4cats-slf4j module now uses the macro based implementation from
iolog4s: https://iolog4s.github.io/iolog4s/

This one is essentially identical to the log4s approach, but macros
generate code that wrap statements in F.delay when appropriate.
This solution fixes a problem in the previous commit where the program would
crash with ??? when the logger was being referenced from the `Logger[F]` interface.

What this implementation effectively achives is that when the user uses the the the
slf4j backend wrapper via the subtype io.chrisdavenport.log4cats.slf4j.Slf4jLogger
then they get macro-based optimizations like you see in iolog4s. But when they switch
to the abstract algebra of `Logger[F]` then they lose this macro-based optimization
and they get something that is equivalent to the old log4cats wrapper.
@ChristopherDavenport
Copy link
Member

Sorry about the delay was a holiday here. Looking into this today.

@ChristopherDavenport
Copy link
Member

So is the main problem that your algebra has additional methods that perhaps are relevant or not, in this case the MDC Implementation?

@lorandszakacs
Copy link
Member Author

lorandszakacs commented May 29, 2018

@ChristopherDavenport
There are two problems, and I consider MDC to be the secondary one.

  1. a macro-based implementation cannot work with the abstract algebra of Logger[F] because macros cannot be reified for abstract methods. Even if the underlying runtime object is the same. See the example program in the description of the PR where you refer to the same value via two different references.

  2. MDC-like algebra would be nice to have on Logger[F] because without it the only migration path from iolog4s to log4cats is by using the Sfl4jLogger[F] explicitly. And I haven't looked into how to support this with a scribe backend. Or maybe we can define a better algebra for it altogether.

@ChristopherDavenport
Copy link
Member

So first idea for algebras. We have multiple contexts

Logger, LogLevelAware, MDCLogger are three traits. Then loggers implement them and people ask for them individually. Problem is most people want all of the functionality together and LogLevelAware and MDCLogger are a bit decoupled really.

Second we build a macro based internal representation akin to iolog4s that is fully private and then this module depending on it that creates the algebra on the second compilation pass. That way we don't leak the underlying representation at all but can still build a functional component.

@lorandszakacs
Copy link
Member Author

@ChristopherDavenport I agree with that general idea for an algebra.

Second we build a macro based internal representation akin to iolog4s that is fully private and then this module depending on it that creates the algebra on the second compilation pass.

I don't understand. Would this allow macros to be expanded when clients use the algebra? Because otherwise it will have the same runtime characteristics as log4cats currently has, making the whole macro implementation effort of no real value.

I'll get back to this in a few hours. So feel free to detail any ideas here.

@lorandszakacs
Copy link
Member Author

Closed in favor of: #33

@lorandszakacs lorandszakacs deleted the feature/integrate_iolog4s branch May 30, 2018 14:37
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