Server logging middleware: change logAction to F[String => F[Unit]]#7446
Server logging middleware: change logAction to F[String => F[Unit]]#7446iRevive wants to merge 2 commits into
F[String => F[Unit]]#7446Conversation
| val logF: IO[String => IO[Unit]] = | ||
| for { | ||
| state <- local.get | ||
| } yield (message: String) => logs.update(_ :+ (message, state)) |
There was a problem hiding this comment.
This test fails on the current main branch.
1bd4833 to
227234b
Compare
| fk: F ~> G, | ||
| redactHeadersWhen: CIString => Boolean = defaultRedactHeadersWhen, | ||
| logAction: Option[String => F[Unit]] = None, | ||
| logAction: Option[F[String => F[Unit]]] = None, |
There was a problem hiding this comment.
We can also use something more flexible:
sealed trait LogAction[F[_]]
object LogAction {
def plain[F[_]](f: String => F[Unit]): LogAction[F] = PlainAction(f)
def deferred[F[_]](f: F[String => F[Unit])]: LogAction[F] = DeferredAction(f)
private[http4s] final case class LogAction[F[_]](f: String => F[Unit]) extends LogAction[F]
private[http4s] final case class DeferredAction[F[_]](f: F[String => F[Unit]]) extends LogAction[F]
}There was a problem hiding this comment.
Maybe even an implicit conversion can be handy:
implicit def toPlainLogAction[F[_]](f: String => F[Unit]): LogAction[F] = LogAction.plain(f)
implicit def toDeferredLogAction[F[_]](f: F[String => F[Unit]]): LogAction[F] = LogAction.deferred(f)227234b to
5c11224
Compare
|
I've noticed the milestone on this keeps moving and obviously we're interested in this change. Is there a strategy to do this in a way without breaking bin compat I can take a crack at, and get it in 0.23.x? |
|
Is the breakage just on the various It would be tedious and messy, but a parallel set of constructors with different names might work, with the original ones lifting the |
|
Targets main, so unmilestoning 0.23. Maybe those suggestions above help get a variant into 0.23. |
|
Thanks Ross, I'll have a go at that |
|
@rossabaker I've opened #7744 with bin compatible changes Not sure of best practises adding changes on someone else's PR so @iRevive if you want to cherry-pick my changes into here to merge this PR, feel free! |
The follow-up to https://discord.com/channels/632277896739946517/632286375311573032/1239565715036442686.
Repo example: https://github.com/alexcardell/otel4s-server-logger-repro-example/tree/master
Motivation
We want to allow capturing the 'state' while we are still within Kleisli closure. This is needed to make the MDC logging work.
Context
When
logBody = false, the logging is 'streamlined' - the message will be logged within the Kleisli closure.http4s/server/shared/src/main/scala/org/http4s/server/middleware/ResponseLogger.scala
Lines 79 to 80 in b5cd7d7
However, when
logBody = true, the logic changes drastically. The middleware binds logging action to the materialization of the body.http4s/server/shared/src/main/scala/org/http4s/server/middleware/ResponseLogger.scala
Lines 83 to 89 in b5cd7d7
Since the body's materialization may happen outside of the Kelisli (and/or on a different fiber), the relevant IOLocal context is missing.