typelevel / otel4s

An OpenTelemetry library for Scala based on Cats-Effect
https://typelevel.org/otel4s
Apache License 2.0
162 stars 30 forks source link

Responsibilities of Tracer #89

Closed rossabaker closed 2 months ago

rossabaker commented 1 year ago

The only thing the Otel specification requires of Tracer is that it has the span builder. We are also using it as the handle for the tracing context. We should explicitly decide whether to combine or separate these concerns.

rossabaker commented 1 year ago

I think Tracer being the home for the tracing context works reasonably well. I don't see a particularly clean separation to be had, because the spanBuilder refers to the scope.

lacarvalho91 commented 1 year ago

From my experience of this lib so far, I do wonder if we should be able to use the tracer to extract the context on the client side for propagation. Something doesn't feel quite right about using the tracer to set the context with joinOrRoot which hides the Local usage and then having to use a combo of Ask and the TextMapPropagator on the client side to extract the context and propagate it to downstream calls. I do seem to remember some discussion on other issues around the API for propagation but thought I'd give my two cents.

What I've ended up doing at $work is creating my own propagator capability which has an implementation that uses Tracer and TextMapPropagator. This is kind of an aside but thought I would mention: we also have an impl that allows you to configure custom headers to propagate as we need that for propagating things like AB experiment headers for routing.

iRevive commented 1 year ago

I tend to agree. The current propagation API is slightly cumbersome. I have at least two cases at $work, when I need to carry Ask[F, Vault] and TextMapPropagator around only to propagate the context in http4s middleware.

Carrying Ask[F, Vault] around means I cannot use OtelJava.global and I must do:

import org.typelevel.otel4s.java.instances.localForIoLocal

def middleware[F[_]: Tracer: Ask[*[_], Vault]](propagators: TextMapPropagators[F])(route: HttpRoutes[F]): HttpRoutes[F] = ???

IOLocal(Vault.empty).flatMap { implicit ioLocal: IOLocal[Vault] =>
  IO.delay(GlobalOpenTelemetry.get).flatMap { global =>
    val otel4s = OtelJava.local(global)
    otel4s.tracerProvider.get("my.app").flatMap { implicit tracer: Tracer[IO] =>
      val routes: HttpRoutes[IO] = middleware[IO](otel4s.propagators)(httpRoutes)
    }
  }
}

Instead, we can define propagate (or any suitable name) to Tracer:

trait Tracer[F[_]] {
  def propagate[A: TextMapUpdater](carrier: A): F[A]
}

and the implementation is:

def propagate[A: TextMapUpdater](carrier: A): F[A] = 
  for {
    vault <- Ask[F, Vault].ask[Vault]
  } yield propagators.textMapPropagator.injected(vault, carrier)

Then, the example above can be simplified to:

def middleware[F[_]: Trace](route: HttpRoutes[F]): HttpRoutes[F] = ???

OtelJava.global.flatMap { otel4s =>
  otel4s.tracerProvider.get("my.app").flatMap { implicit tracer: Tracer[IO] =>
    val routes: HttpRoutes[IO] = middleware[IO](otel4s.)(httpRoutes)       
  }
}
iRevive commented 1 year ago

we also have an impl that allows you to configure custom headers to propagate as we need that for propagating things like AB experiment headers for routing.

Currently, we rely on OpenTelemetry Java implementation. You can use a builder to modify the autoconfigured SDK:

import io.opentelemetry.context.propagation.{TextMapPropagator => JTextMapPropagator}
import io.opentelemetry.sdk.autoconfigure.AutoConfiguredOpenTelemetrySdk

private def createOpenTelemetry[F[_]: Sync]: F[OpenTelemetry] = Sync[F].delay {
  AutoConfiguredOpenTelemetrySdk
    .builder()
    .addPropagatorCustomizer { (autoConfigured, _) =>
      val customHeaderPropagator: JTextMapPropagator = ???
      JTextMapPropagator.composite(
        autoConfigured,
        customHeaderPropagator
      )
    }
    .setResultAsGlobal
    .build()
    .getOpenTelemetrySdk
}
lacarvalho91 commented 1 year ago

You can use a builder to modify the autoconfigured SDK

@iRevive that's interesting, I was wondering if I could just do it with OpenTelemetry, I'll give that a go. Thanks!

NthPortal commented 1 year ago

where is the default (Java-based) implementation getting an instance of Ask[F, Vault]? the only context bound it currently has is Sync

iRevive commented 1 year ago

@NthPortal the Local[F, Vault] is acquired here https://github.com/typelevel/otel4s/blob/main/java/all/src/main/scala/org/typelevel/otel4s/java/OtelJava.scala#L51-L56 and passed to the component here https://github.com/typelevel/otel4s/blob/main/java/trace/src/main/scala/org/typelevel/otel4s/java/trace/Traces.scala#L35

NthPortal commented 1 year ago

@iRevive thanks! I'll look into wiring it through for a PoC implementation of Tracer#propagate

NthPortal commented 12 months ago

PR is up for Tracer#propagate

iRevive commented 2 months ago

The Tracer API is established now. I guess we can close the task.