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

Responsibilities of Tracer #89

Closed
rossabaker opened this issue Jan 23, 2023 · 10 comments
Closed

Responsibilities of Tracer #89

rossabaker opened this issue Jan 23, 2023 · 10 comments
Labels
tracing Improvements to tracing module

Comments

@rossabaker
Copy link
Member

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 rossabaker added the tracing Improvements to tracing module label Jan 23, 2023
@rossabaker
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Contributor

iRevive commented Jul 11, 2023

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
Copy link
Contributor

iRevive commented Jul 11, 2023

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
Copy link
Contributor

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
Copy link
Contributor

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

@NthPortal
Copy link
Contributor

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

@NthPortal NthPortal mentioned this issue Jul 26, 2023
2 tasks
@NthPortal
Copy link
Contributor

PR is up for Tracer#propagate

@iRevive
Copy link
Contributor

iRevive commented May 3, 2024

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

@iRevive iRevive closed this as completed May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tracing Improvements to tracing module
Projects
None yet
Development

No branches or pull requests

4 participants