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

Introduce a TraceResource type class #527

Closed
wants to merge 1 commit into from

Conversation

rossabaker
Copy link
Member

Alternative design to #526. Tracing a resource only works well with "stateful" instances. This acknowledges that by adding a sub class of Trace. Instances are provided for:

  • IO
  • Transformers given a TraceResource[F] and a MonadCancel[F].
  • A no-op for Applicative[F].

Libraries that use this constraint trade the "local" instances (Kleisli[F, Span[F], *] and Local[F, Span[F]]) for the ability to span an entire resource.

@rossabaker rossabaker marked this pull request as draft March 1, 2022 04:25
@rossabaker rossabaker mentioned this pull request Mar 1, 2022
@rossabaker
Copy link
Member Author

This is basically Bayou with some deduplication because it lives in the same repo.

@rossabaker rossabaker mentioned this pull request Mar 2, 2022
@SystemFw
Copy link

SystemFw commented Mar 6, 2022

or the ability to span an entire resource.

Not just that, I also think it makes the very common pattern of creating untraced dependencies whose operations need to be traced a lot more usable.
Matter of fact, except for things like naming, this design is essentially what I had come up with just for the problem above without even thinking about tracing Resource, so with Bayou it's 3 data points that there might be something to it

@rossabaker rossabaker marked this pull request as ready for review March 6, 2022 15:35
@rossabaker
Copy link
Member Author

Perhaps this achieves subtleties I don't appreciate. I'm not precisely sure what this means:

pattern of creating untraced dependencies whose operations need to be traced a lot more usable.

Are you talking about the entryPointTrace idea? I'm not sure that this solves that. What this proposal still lacks is a nice way to inject a traced dependency in a wider scope than the entry point that uses it. For example, the database algebra that serves an HTTP request handler.

@SystemFw
Copy link

SystemFw commented Mar 6, 2022

I'm not sure that this solves that. What this proposal still lacks is a nice way to inject a traced dependency in a wider scope than the entry point that uses it. For example, the database algebra that serves an HTTP request handler.

mm, I thought it did, but I misinterpreted the "creates a new span" in the scaladoc, it's creating a child span whereas I thought it'd create a brand new one

@rossabaker
Copy link
Member Author

Okay, I think I now see what you thought you saw. I think the entry method could return Resource[F, Unit], but also would need a kernel, and have continueOrRoot-like semantics.

I was deferring dealing with the entrypoint abstraction until we settled between this and #526, because I think the problems are orthogonal. But the entry is only needed on the stateful, and this is the PR that introduces a distinction between stateful and stateless.

@SystemFw
Copy link

SystemFw commented Mar 7, 2022

Yeah they are orthogonal (and sorry for hijacking your discussion to an extent), although I think the problem solved by the entry method is much more common, where I was coming from is this:

that introduces a distinction between stateful and stateless.

@rossabaker
Copy link
Member Author

The entry point is supportable on the stateless instances, but only works with a signature that passes the continuation, and would not work particularly well for Resource. It's necessary only because of stateful Trace, but pushed down there, would let us continue to abstract over stateful vs. stateless:

def continueOrRoot[A](fa: F[A], kernel: Kernel): F[A]

@SystemFw
Copy link

SystemFw commented Mar 7, 2022

ahah I keep getting confused because in my mind Trace + Kleisli is more "stateful" (since it represents the current trace).

I think what you're saying is that if we wanted to have entry points in Trace, rather than TraceResource (really needs a nicer name), it would have to include an A => F[B] which is confusing for Resource? and that TraceResource doesn't have this problem?

def continueOrRoot[A](fa: F[A], kernel: Kernel): F[A]

ok this sounds good, so continueOrRoot won't be expressed in terms of Resource, you only see Resource where you want to trace an actual Resource?

@armanbilge
Copy link
Member

I think we need a continueOrRoot(kernel: Kernel): Resource[F, Unit] as well. I have effectively this in Bayou I believe.

@rossabaker
Copy link
Member Author

in my mind Trace + Kleisli is more "stateful"

I've been using the opposite terminology. Trace[IO] captures a root span as state when it is acquired. The statefulness is further reflected in that instances must be obtained in IO. Kleisli is "stateless" in that it's just a function of a span to be provided later. But if that's stateless, a state monad is just a function of a state to be provided later, so maybe these terms are bad.

I think what you're saying is that if we wanted to have entry points in Trace, rather than TraceResource (really needs a nicer name), it would have to include an A => F[B] which is confusing for Resource? and that TraceResource doesn't have this problem?

Yes. continueOrRoot[A](name: String, kernel: Kernel): Resource[F, Unit] (ed: I added the name parameter) has the same problem as spanR(name: String): Resource[F, Unit] for Kleisli: we span the use of the returned resource, but that span isn't ambient, or even accessible, to any of the three phases (acquisition, release, or use) of the resource.

We could alternatively add these to Trace, which would be the first instance of Span on the type class:

// Create a new span and return it as a resource
def spanR(name: String): Resource[F, Span[F]]

// Make the span ambient through `fa`
def scope[A](span: Span[F])(fa: F[A]): F[A]

Typed in GitHub, has never seen a compiler:

def traceHttp4sClient[F: Trace](c: Client[F]): Client[F] =
  Kleisli { req =>
     for {
       s <- Trace[F].spanR("whole-connection") // I span the whole connection
       r <- Resource(
         Trace[F].scope(s) { // so `s` is ambient
           Trace[F].span("response-and-headers") { // I span status and headers but not body
             Trace[F].put("http.uri", req.uri.toString) >>
             client.run(req).allocated
           }.flatMap { case (resp, release) =>
             Trace[F].put("http.status", resp.status.toInt) >>
             F.pure(resp -> Trace.scope(s) { // so `s` is ambient
               Trace[F].span("release") { // I don't span body, but how quickly connection closes
                 release
               }
             }
           }
         }
       )
     } yield (r)

That's harder to use, couples the type class to Span, and s is inaccessible to whoever uses the response. The time spent reading the body could be inferred from the time spanned by whole-connection minus response-and-headers or release, but we can't add any tracing.

Alternatively, we can do this in this PR, and spans created while using the response would fall under whole-connection:

def traceHttp4sClient[F: TraceResource](c: Client[F]): Client[F] =
  Kleisli { req =>
     for {
       _ <- TraceResource[F].spanR("whole-connection") // I span the whole connection
       r <- Resource(
         Trace[F].span("response-and-headers") { // I span status and headers but not body
           Trace[F].put("http.uri", req.uri.toString) >>
           client.run(req).allocated
         }.flatMap { case (resp, release) =>
           Trace[F].put("http.status", resp.status.toInt) >>
           F.pure(resp -> Trace[F].span("release") { // I don't span body, but how quickly connection closes
             release
           }
         }
       )
     } yield (r)

TraceResource (really needs a nicer name)

Agree. I almost called it StatefulTrace, but I guess that's confusing too.

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.

None yet

4 participants