Slf4j bridge - fiber propagation

[justcoon]

FiberRefs propagation using ThreadLocal

[adamgfraser]

@justcoon Great working pushing this forward!

I think the question here is whether we need to support propagating all FiberRef values or can just propagate a known subset of FiberRef. For example, the LogContext itself is backed by a single FiberRef and is used to power the LogAnnotation data type. So we could make this a TrackingFiberRef and use a TrackingSupervisor so that these FiberRef values are propagated automatically and we don't need to use any special operator.

If we really do want to support propagating all FiberRef values then I think we will need an operator for this, perhaps withFiberRefs.

[justcoon]

@adamgfraser
i think we need to propagate also some other FiberRef-s than LogContext, for example FiberRef.currentLogAnnotations, FiberRef.currentLogSpan

also user of library itself can create own FiberRef, which may be used in logging
but, i think it could be good if we will not propagate all FiberRef-s, we should probably exclude ZEnvironment

but i am not sure if there are some functions in FiberRefs, which can be easily exclude multiple FiberRef-s from FiberRefs with one call

[adamgfraser]

@justcoon Okay, I think we can propagate the FiberRefs without the user having to use any special operator like this:

object Example extends ZIOAppDefault {

  /**
    * Accesses the current `FiberRefs` of the fiber executing this code. This
    * will only return the current `FiberRefs` if called from code being
    * executed by a ZIO fiber with `RuntimeFlag.CurrentFiber` enabled.
    */
  def unsafeGetFiberRefs()(implicit unsafe: Unsafe): Option[FiberRefs] = {
    val currentFiber = Fiber._currentFiber.get()
    if (currentFiber eq null) None
    else Some(currentFiber.unsafe.getFiberRefs())
  }

  override val bootstrap =
    Runtime.enableCurrentFiber

  val run =
    for {
      fiberRef <- FiberRef.make(0)
      _        <- fiberRef.update(_ + 42)
      _        <- ZIO.debug(unsafeGetFiberRefs()(Unsafe.unsafe))
    } yield ()
}

[adamgfraser]

Can we build the current fiber integration integration into an existing layer such as Slf4jBridge.initialize? It is great to have the ability to customize when necessary but it is also good to make common use cases easy and enableCurrentFiber is really an implementation detail where if the user is going to do something that requires it (basically propagating context from FiberRef values to ThreadLocal values we should just do it.

[justcoon]

i think we can put it to Slf4jBridge.initialize
however there should be option to disable that as enableCurrentFiber may have impact to performance

which probably ends in something like

object Slf4jBridge {

  def initialize(propagateFibers: Boolean = true): ZLayer[Any, Nothing, Unit] = ???

[adamgfraser]

Makes sense. I might split it into two layers so it can be a val and you don't need the empty parentheses when you call it.

object Slf4jBridge {

  val initialize: ZLayer[Any, Nothing, Unit] =
    ???

  val initializeWithoutFiberRefPropagation: ZLayer[Any, Nothing, Unit] =
    ???
}

[justcoon]

@adamgfraser

i added layers as you proposed, probably it is good that this propagation is enabled by default,
however it is also changing default behavior

[adamgfraser]

@justcoon Sounds good.My thought is that EnableCurrentFiber is necessary for integration between FiberRef and ThreadLocal values and that is exactly what you are asking us to do with the Slf4jBridge so that makes sense as the default and will give the "correct", though potentially not as performant value, so that should be the default. But if you think we should do it the other way definitely open to your thoughts on that.

Otherwise looks good to me subject to my comment about avoiding unnecessary ZIO workflows which we will handle separately.

[justcoon]

thank you very much for hints and review

[adamgfraser]

You are within the ZIO name space so you could just use _currentFiber here to avoid the need for the cast. I will also look at specializing the return type of currentFiber.

[justcoon]

i changed it slf4j v2 bridge to use _currentFiber, as it is in zio namespace

[adamgfraser]

This pattern of running a ZIO workflow just to log something is not very efficient. We can get the current loggers from the current FiberRefs and then we can just use those loggers to log directly without going through a ZIO workflow.

[justcoon]

yes, i think you are right, we can probably do it better,
probably by using Fiber.log
but i propose to do it in next PR

[adamgfraser]

👍

[adamgfraser]

Much cleaner without the need for any new operators!

Made a couple of suggestions for further improvements.