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

Tracing with local semantics #105

Closed
wants to merge 4 commits into from
Closed

Tracing with local semantics #105

wants to merge 4 commits into from

Conversation

rossabaker
Copy link
Member

@rossabaker rossabaker commented Jan 31, 2023

This reimagines Tracer with local semantics, which works around the issues with IOLocal and race (including stream interrupt scopes) corrupting the state. All tracing methods need to be passed an effect, in which a specified span is scoped into the Tracer.

Fixes #85 and #88. Supersedes #102.

Copy link
Member Author

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a partial effort, but it makes real some of the other sketches. Running it as is generates the appropriate trace.

@@ -54,7 +52,7 @@ private[otel4s] trait TracerMacro[F[_]] {
* @param attributes
* the set of attributes to associate with the span
*/
def span(name: String, attributes: Attribute[_]*): Resource[F, Span[F]] =
def span(name: String, attributes: Attribute[_]*): SpanBuilder[F] =
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could return SpanBuilder or a new SpanOps. The idea is to have resource-like methods, most importantly use and surround.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer SpanOps. That way we can split responsibilities.
Also, the implementation of SpanBuilderImpl should be much cleaner.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would SpanBuilder extend SpanOps, or introduce an intermediate call, like fs2's .compile?

Copy link
Contributor

@iRevive iRevive Feb 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess an intermediate call would be better.

We can mimic the existing implementation:

trait Tracer[F[_]] {
  def span(name: String, ...): SpanOps[F] // exists in TracerMacro, basically a shortcut for spanBuilder(...).build
  def spanBuilder(name: String, ...): SpanBuilder[F]
}

trait SpanBuilder[F[_]] {
  def build: SpanOps[F]
}

@@ -75,9 +73,10 @@ private[otel4s] trait TracerMacro[F[_]] {
def rootSpan(
name: String,
attributes: Attribute[_]*
): Resource[F, Span[F]] =
): SpanBuilder[F] =
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should return the same as span.

@@ -104,7 +103,8 @@ private[otel4s] trait TracerMacro[F[_]] {
def resourceSpan[A](name: String, attributes: Attribute[_]*)(
resource: Resource[F, A]
): Resource[F, Span.Res[F, A]] =
macro TracerMacro.resourceSpan[F, A]
macro TracerMacro.resourceSpan[F, A]
*/
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is the biggest loss in local semantics: we can only hug the acquire and release, not the use.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With changes from #86, we can do the trick:

val resource: Resource[IO, Unit] = Resource.make(IO.sleep(50.millis))(_ => IO.sleep(100.millis))

tracer.span("resource").wrapResource(resource).use { _ =>
  Work[IO].doWork
}

image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That branch still has this "stateful" definition of createScope:

private def createScope(scope: Scope): Resource[F, Unit] =
Resource
.make(local.getAndSet(scope).to[F])(p => local.set(p).to[F])
.void

I think that's where we run into the problems with interruptScope, vs. this "local" definition:

private def createScope(scope: Scope): F ~> F =
new (F ~> F) {
def apply[A](fa: F[A]): F[A] =
MonadCancelThrow[F].bracket(local.getAndSet(scope).to[F])(
Function.const(fa)
)(p => local.set(p).to[F])
}

I don't yet see how to make wrapResource work with the local semantics. If we do, we get nice traces like the above and avoid the corruption via common fs2 combinators, but I'm still not convinced it's possible.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Your #86 works even in an FS2 interruptScope. 🤔

  def run: IO[Unit] =
    tracerResource.use { implicit tracer =>
      val resource: Resource[IO, Unit] = Resource.make(IO.sleep(50.millis))(_ => IO.sleep(100.millis))

      def stream(name: String) =
        Stream.eval(tracer.spanBuilder(name).wrapResource(resource).start.surround(
          Work[IO].doWork
        ))

      tracer.span("root").surround(
        (stream("uninterrupted") ++ stream("interrupted").interruptScope).compile.drain
      )
    }

Jaeger console screenshot showing use still sibling of acquire and release in both interrupted and uninterrupted subtraces

Copy link
Contributor

@iRevive iRevive Feb 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What were your expectations (a.k.a. interruptScope break tracing)? An incorrect parent for stream("interrupted").interruptScope?

I didn't play enough with interruptScope so I genuinely don't know 😅

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, but this does not. The second DoWork escapes from the interrupted span:

    tracerResource.use { implicit tracer =>
      val resource: Resource[IO, Unit] = Resource.make(IO.sleep(50.millis))(_ => IO.sleep(100.millis))

      def stream(name: String) =
        Stream.resource(tracer.spanBuilder(name).wrapResource(resource).start)
          .evalMap(_ => Work[IO].doWork)

      tracer.span("root").surround(
        (stream("uninterrupted") ++ stream("interrupted").interruptScope).compile.drain
      )
    }

Screenshot 2023-02-02 at 9 14 33 AM

(Apologies for the lack of alt text ... I need to figure out how to dump these spans to a text tree...)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can print spans as a tree in tests

https://github.com/typelevel/otel4s/blob/main/java/trace/src/test/scala/org/typelevel/otel4s/java/trace/TracerSuite.scala#L470

The format is {span_name} {start} -> {end}

resource-span 1000000000 -> 1675000000 =>
  acquire 1000000000 -> 1025000000 =>
    acquire-inner 1000000000 -> 1025000000
  use 1025000000 -> 1375000000 =>
    body-1 1025000000 -> 1125000000
    body-2 1125000000 -> 1325000000
    body-3 1325000000 -> 1375000000
  release 1375000000 -> 1675000000

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once we merge #105, we will not be able to do fs2.Stream.resource(tracer.spanBuilder(name).wrapResource(resource).start), since there is no resource exposed.

Here is an example with both #105 and #86:
https://github.com/typelevel/otel4s/pull/107/files#diff-1864b2af85047d6d543da3ec001b74204471c7489f78a01fcb5db5f502480b81R58-R66

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooh, neat, this is the first time both good ideas have been on one branch. I think we'd need a wrapStream, but I also think it might work. Let me see if I can break this one! 😆

def use[A](f: Span[F] => F[A]): F[A] =
start.use { case (span, nt) => nt(f(span)) }

private def start: Resource[F, (Span[F], F ~> F)] =
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love this signature, but it's private.

Resource
.make(local.getAndSet(scope).to[F])(p => local.set(p).to[F])
.void
private def createScope(scope: Scope): F ~> F =
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a signature we can make work with any cats.mtl.Local, and makes the whole thing go.

@rossabaker
Copy link
Member Author

To exercise this, I tried tracing Skunk with otel4s on 2ef92ef. skunk-core compiles successfully!

@rossabaker
Copy link
Member Author

Note that this approach also addresses #85.

@rossabaker
Copy link
Member Author

rossabaker commented Feb 1, 2023

Natchez rootJVM compiling, sans tests. I'm opening a PR, so there's a convenient place to discuss the pros and cons of the Otel4s API within a large example: rossabaker/skunk#1

@rossabaker
Copy link
Member Author

I think this is superseded by #107. It makes backward progress on tracing resources without #86.

@rossabaker rossabaker closed this Feb 7, 2023
rossabaker added a commit to iRevive/otel4s that referenced this pull request Feb 7, 2023
chr12c pushed a commit to chr12c/otel4s that referenced this pull request Jun 26, 2023
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.

A Natchez-like "continuation" API to avoid MonadCancelThrow leakage?
2 participants