-
Notifications
You must be signed in to change notification settings - Fork 79
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
Add Trace.spanR #526
Add Trace.spanR #526
Conversation
@@ -22,6 +22,9 @@ trait Trace[F[_]] { | |||
*/ | |||
def kernel: F[Kernel] | |||
|
|||
/** Creates a new span, and within it acquires and releases the spanR `r`. */ | |||
def spanR[A](name: String)(r: Resource[F, A]): Resource[F, A] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two competing designs here. Bayou defines a spanR(name: String): Resource[F, Unit]
.
Bayou's signature is biased toward a "stateful" trace. There is no Local
implementation, including Kleisli[F, Span[F], *]
implementation, because the resource span can't be threaded through to the resource effect.
This signature supports Local
semantics (with the limitation that use
does not see the resource span as the ambient span). But it does not support monad transformers T
. The only way I've thought to implement them is to apply a natural transformation T ~> F
to use Trace[F].spanR
, but this does not exist for StateT
, EitherT
, OptionT
, or Nested
, which represent the compilation failures in this branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to span a Stream
with this design?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Yes, sort of? This would cover the entire stream, but not be ambient. There's no .allocated
on Stream
that gives us similar hooks to what we have on Resource
:
def stream[F[_]: Trace: MonadCancelThrow, A](name: String)(s: Stream[F, A]): Stream[F, A] =
Stream.resource(Trace[F].spanR(name)(Resource.unit)) >> s
In other words, if we tried to span each value, all the "emit" spans would be siblings of the stream span:
def stream[F[_]: Trace: MonadCancelThrow, A](name: String)(s: Stream[F, A]): Stream[F, A] =
Stream.resource(Trace[F].spanR(name)(Resource.unit)) >>
s.translate(new (F ~> F) {
def apply[B](fb: F[B]): F[B] = Trace[F].span("emit")(fb)
})
This would not be true in your preferred definition of Trace[IO]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that's not all that different from this definition for the transformer instances. It spans r
, but is not ambient for its acquisition or release:
def spanR[A](name: String)(r: Resource[StateT[F, S, *], A]): Resource[StateT[F, S, *], A] =
trace.spanR(name)(Resource.unit).mapK(StateT.liftK[F, S]) >> r
def spanR[A](name: String)(r: Resource[IO, A]): Resource[IO, A] = | ||
Resource.eval(local.get).flatMap(parent => | ||
parent.span(name).flatMap { child => | ||
def inChild[B](io: IO[B]): IO[B] = | ||
local.set(child).bracket(_ => io)(_ => local.set(parent)) | ||
Resource(inChild(r.allocated).map { case (a, release) => | ||
a -> inChild(release) | ||
}) | ||
} | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A simpler definition would just bracket the entire resource with child
, and also make child
the ambient span for use
. But this definition is consistent with the Local
semantics: child
still spans the resource, and is ambient for acquire
and release
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
I still think it would be nice to offer the alternative implementation as well. In practice it's what I'd want to use, and I don't care if its inconsistent with Kleisli
!
@@ -140,7 +177,7 @@ object Trace { | |||
|
|||
} | |||
|
|||
implicit def liftKleisli[F[_], E](implicit trace: Trace[F]): Trace[Kleisli[F, E, *]] = | |||
implicit def liftKleisli[F[_]: MonadCancelThrow, E](implicit trace: Trace[F]): Trace[Kleisli[F, E, *]] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if we can salvage the other monad transformers, they will probably all need a MonadCancelThrow
constraint to deal with transforming the Resource
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a PR futzing with Resource#mapK
constraints but I don't think it helps much here.
I think I encountered this working on Bayou, and I wondered if it was a good argument to have two Trace
typeclasses after all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that PR helps much here: you still need MonadCancelThrow[F]
, and we're going F ~> G
and G ~> F
unless someone is more clever than me. (Possible! Nay, probable!)
The base instances, other than noop
, all already require MonadCancelThrow
, so I don't think this limits which effects can be traced. But it could require a stronger constraint in a few places along the way.
def spanR[A](name: String)(r: Resource[StateT[F, S, *], A]): Resource[StateT[F, S, *], A] = | ||
trace.spanR(name)(Resource.unit).mapK(StateT.liftK[F, S]) *> r |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't fully grasped the implications of these definitions, but is this much different than if we tried to implement spanR(name: String): Resource[F, Unit]
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tested this, but ...
-
If the transformed instance is "stateful" (parents
acquire
,use
, andrelease
), so are these, and would be nicer expressed asspanR(name: String): Resource[F, Unit]
. -
If the transformed instance is "local" (parents
acquire
andrelease
but notuse
), we still span the resource, but parent none of the three phases. -
Local and stateful tracing are equivalent through the current
Trace
and the current abstraction works well until we introduce resources. -
Resource tracing can be implemented either in a stateful subclass without local instances, or in the
Trace
constraint with an awkward signature and weird semantics for the transformers. -
If we resort to a subclass, libraries would be choosing the tradeoff on behalf of apps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks, it took me a while to grasp this. IIUC, for a "stateful" instance if you have
def spanR[A](name: String)(r: Resource[F, A]): Resource[F, A]
then you can get "for free"
def spanR(name: String): Resource[F, Unit] = spanR(name)(Resource.unit)
with the exact same semantics as if Trace
had been defined in terms of spanR(name: String): Resource[F, Unit]
to begin with.
However, for a stateless instance there is a difference whether you use spanR[A](name: String)(r: Resource[F, A])
or spanR(name: String): Resource[F, Unit]
. So you want to use the former whenever possible, and only use the latter when necessary e.g. Stream
and transformer instances.
So in summary, defining Trace
as in this PR doesn't compromise the power of the stateful IOLocal
instance, it will work exactly like it does for a Bayou-style Trace
.
What it does compromise however is the library experience: since we want spanR(name: String): Resource[F, Unit]
to be the second-choice, we don't directly offer it on the typeclass, even though it's easily derived in practice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes:
acquire | release | use | |
---|---|---|---|
Stateful base | ✅ | ✅ | ✅ |
Stateful transformed | ✅ | ✅ | ✅ |
Stateless base | 🤷 | 🤷 | 🚫 |
Stateless transformed | 🚫 | 🚫 | 🚫 |
✅ - both spanR
🚫 - neither spanR
🤷 - spanR(name: String)(r: Resource[F, A])
, but not spanR(name: String)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, thanks. This PR seems good then: it doesn't compromise the stateful IOLocal
instance, while still being at least somewhat compatible with Kleisli
. On the library side, you can trace any Resource
-like thing either directly or with trace.spanR(name)(Resource.unit)
. So everyone should be happy, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not convinced it's the right abstraction when we have a type class where a column mixes ✅ and 🚫.
I'll fix the other modules, unmark it as a draft, and we'll see if anyone else has any thoughts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not convinced it's the right abstraction when we have a type class where a column mixes ✅ and 🚫.
That would leave us with "Stateless I" I believe? Where you can span a resource-like thing but can't parent any of the acquire/use/release phases.
FWIW I'd like Stateless I even more. Then we can have spanR(name: String): Resource[F, Unit]
!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Under Stateless I, we can never add fields to any phase of a resource span. It is never ambient and can't be retrieved.
#527 is the most consistent and this has the broadest support. I think that's the tradeoff we're stuck with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's assuming that we can't have "inconsistent" implementations of Trace
, which I think is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am in favor of continuing with #527 over this one, particularly if typelevel/fs2#2843 works out. |
Is natchez okay to take on an fs2 dependency? |
The repo already depends on it for xray. Not sure it's smart for core to take it on, but an fs2 module wouldn't hurt. (Half-baked idea: as @zmccoy is fond of saying, tracing is most important around network calls. Having a natchez-fs2 that traced some of its io functions would be cool.) |
It turns out a def spanS[F[_], A](name: String)(s: Stream[Kleisli[F, String, *], A]): Stream[Kleisli[F, String, *], A] =
s.translate(
new (Kleisli[F, String, *] ~> Kleisli[F, String, *]) {
def apply[B](k: Kleisli[F, String, B]) = k.local(_ => name)
}
)
val s = Stream.eval(Kleisli.ask[IO, String])
(s ++ spanS("child")(s) ++ s)
.compile
.toList
.run("root")
.flatMap(IO.println) prints |
Tracing can be viewed as an exchange If we redefine Expectation is that:
Caveats:
|
The API for tracing a stream (example from armanbilge/bayou#1) ends up looking like def stream(implicit trace: Trace[IO]): Stream[IO, Unit] = {
Trace[Stream[IO, *]].span("new_root")(
Stream(1, 2, 3, 4, 5, 6, 7, 8, 9, 10)
.covary[IO]
.chunkN(3)
.flatMap { chunk =>
Trace[Stream[IO, *]].span("child")(
Stream.eval(trace.put("vals" -> chunk.toList.mkString(","))) >>
Stream.sleep[IO](1.second)
)
}
)
} To weave in and out of Stream, we have to accept the simpler |
Just figured I'd share that we've been using a custom I've open sourced it here. I don't intend to promote yet another tracing library. I'd much rather these concepts make it into Natchez. |
Any thoughts on this design versus #527 now that some time has passed? @armanbilge @rossabaker |
I moved onto otel4s and don't remember all the nuances, but I think the basic tradeoff was that this behaves similarly between stateless and stateful instances, and #527 was able to make the resource span the parent of Thanks for picking it up. |
Adds a way to span a resource, from acquisition through release.
See #514.