Immutable injection in TextMapPropagator

[rossabaker]

Rough draft of immutable injection. Adds injected as an extension to the TextMapPropagator interface while preserving compatibility with the standard injectors we get from a compliant SDK.

See #147, #180.

[rossabaker]

Will scaladoc this if people like the approach.

[AprilAtBanno]

I like the approach ;P

[rossabaker]

Good: the mutation is hidden from the outside.

Bad: allocates a builder and carrier even if no fields are injected. Can we ask jPropagator whether it's actually going to inject anything? We could do a decorator and lazily create the builder.

Awkward: this implementation needs Sync just for the inject, which I think would be less used in Scala than this variant!

[NthPortal]

you could always check if the vault is empty first. idk if that covers all cases, but it could optimise some

[rossabaker]

I don't know whether every injector is guaranteed not to inject anything if there's nothing to inject... seems reasonable, but is the absence of an attribute ever serialized somehow? 🤔

[rossabaker]

Fails MiMa. I don't mind going to 0.3 for it.

[iRevive]

Looks good! With a few extra steps, I could pass span details downstream using http4s.

Currently, we need Vault to extract the actual span details. But there is no way to get it from within unless you pass Local[F, Vault] everywhere.

In Tracer#joinOrRoot we hide all operations with Vault and TextMapPropagator in the implementation. Should we do something similar with injected eventually? Adding a high-level utility method or def current: F[Vault] to Tracer?

Example

object Http4sExample extends IOApp.Simple {

  def globalOtel4s: Resource[IO, Otel4s[IO]] =
    Resource
      .eval(IO(GlobalOpenTelemetry.get))
      .evalMap(OtelJava.forAsync[IO])
  
  def run: IO[Unit] = {
    globalOtel4s.use { (otel4s: Otel4s[IO]) =>
      otel4s.tracerProvider.tracer("Http4sExample").get.flatMap {
        implicit tracer: Tracer[IO] =>
          
          val helloWorldService = HttpRoutes.of[IO] {
            case GET -> Root / "hello" / name =>
              tracer.rootSpan("request-hello", Attribute("name", name)).surround {
                otel4s.local.ask[Vault].flatMap { vault => // otel4s.local does not exist :(
                  val rawHeaders = otel4s.propagators.textMapPropagator.injected(vault, Map.empty[String, String])
                  val headers = Headers(rawHeaders.map(kv => kv: Header.ToRaw).toSeq: _*)
                  Ok(s"Hello, $name.", headers)
                }
              }
          }

          EmberServerBuilder.default[IO]
            .withHost(host"localhost")
            .withPort(port"8000")
            .withHttpApp(helloWorldService.orNotFound)
            .build
            .useForever
      }
    }
  }

  implicit val toMapInjector: TextMapInjector[Map[String, String]] =
    new TextMapInjector[Map[String, String]] {
      type Builder = mutable.Map[String, String]
      
      def textMapSetter: TextMapSetter[mutable.Map[String, String]] =
        new TextMapSetter[mutable.Map[String, String]] {
          def unsafeSet(carrier: mutable.Map[String, String], key: String, value: String): Unit =
            carrier.update(key, value)
        }

      def toBuilder(carrier: Map[String, String]): mutable.Map[String, String] =
        mutable.Map.from(carrier)

      def toCarrier(builder: mutable.Map[String, String]): Map[String, String] =
        builder.toMap
    }
}

And the curl:

curl -v localhost:8000/hello/world           
*   Trying 127.0.0.1:8000...
* Connected to localhost (127.0.0.1) port 8000 (#0)
> GET /hello/world HTTP/1.1
> Host: localhost:8000
> User-Agent: curl/7.87.0
> Accept: */*
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< Date: Wed, 26 Apr 2023 11:35:04 GMT
< Connection: keep-alive
< Content-Type: text/plain; charset=UTF-8
< X-B3-SpanId: b6422c655435936d
< X-B3-Sampled: 1
< X-B3-TraceId: 71abb6fadbd47ffdf675a0cc13287881
< Content-Length: 13
<
* Connection #0 to host localhost left intact
Hello, world.%

---

By the way, should implicit val toMapInjector: TextMapInjector[Map[String, String]] = be available out of the box?

[rossabaker]

Adding a high-level utility method or def current: F[Vault] to Tracer?

The current context is available in the Java API via Context.current(). A ThreadLocal static won't work for us, of course. Exposing it via Tracer seems questionable... it's associated with the Otel4s instance. I don't know how much we want to clutter Tracer, but we need to do something. I don't have a strong feeling on this yet.

By the way, should implicit val toMapInjector: TextMapInjector[Map[String, String]] = be available out of the box?

Yes.

Chris and April and I discussed today that TextMapSetter[ListBuffer[String, String]] should. Probably also mutable.Map[String, String].

[rossabaker]

I don't know whether every injector is guaranteed not to inject anything if there's nothing to inject... seems reasonable, but is the absence of an attribute ever serialized somehow? 🤔

[rossabaker]

Are we likely to need an Otel4s wherever we inject? I am thinking maybe there should be a context: F[Vault] on Otel4s itself.

We are supposed to provide an attachContext and detachContext function, but that fights our Local semantics.

[iRevive]

Are we likely to need an Otel4s wherever we inject? I am thinking maybe there should be a context: F[Vault] on Otel4s itself.

Otel4s lives in the core modules (which aggregate core-trace, core-metrics, and core-common modules). As a library author, I may prefer pulling only core-trace module, if I do not need metrics.

For example, to implement the propagation middleware, I need the following:

1. A way to get the current Vault
2. An instance of TextMapPropagator[F[_]]

Then, the middleware can be defined as:

def middleware[F[_]: Tracer](current: F[Vault], propagator: TextMapPropagator[F])(routes: HttpRoutes[F]): HttpRoutes[F]

Where:

1. Tracer - we need it to do the tracing magic (e.g. Tracer[F].joinOrRoot)
2. current: F[Vault] and TextMapPropagator[F] - mandatory to extract the current context and make it into Map[String, String]. F[Vault] can be replaced with Local[F, Vault].

And the end user of a library can do the following:

def run: IO[Unit] =
  globalOtel4s.use { (otel4s: Otel4s[IO]) =>
    otel4s.tracerProvider.tracer("Http4sExample").get.flatMap { implicit tracer: Tracer[IO] =>
      val httpApp = middleware(otel4s.context, otel4s.propagators.textMapPropagator)(routes).orNotFound

      EmberServerBuilder.default[IO].withHttpApp(httpApp).build.useForever
    }
  }

Looks ok, from my point of view.

---

To keep the middleware signature cleaner and stick with the minimal set of dependencies, can we make module-specific traits? E.g. :

core-common:

trait Otel4sCommon[F[_]] {
  def current: F[Vault] 
}

core-trace:

trait Otel4sTrace[F[_]] { self: Otel4sCommon[F] =>
  def propagators: ContextPropagators[F]
  def tracerProvider: TracerProvider[F]
}

core-metrics:

trait Otel4sMetrics[F[_]] { self: Otel4sCommon[F] =>
  def meterProvider: MeterProvider[F]
}

core:

trait Otel4s[F[_]] extends Otel4sCommon[F] with Otel4sTrace[F] with Otel4sMetrics[F]

---

If we do it that way, the library that pulls only core-trace can define the middleware as the following:

def middleware[F[_]: Tracer](otel: Otel4sTrace[F])(routes: HttpRoutes[F]): HttpRoutes[F]

My question: is it worth it?

[NthPortal]

it would probably be better to add an implicit TextMapSetter for all C < mutable.Iterable[(String, String)], rather than just mutable.Map and ListBuffer specifically.

implicit instances of TextMapUpdater for all C < immutable.Iterable[(String, String)], or at least for all C < immutable.Map[String, String] would probably be good as well

[rossabaker]

I don't have as much experience with subtype-bound implicits, but it seems to work, and the semantics are right down to that subtype. I like it.

[AprilAtBanno]

actually, it turns out mutable.Iterable has no additional methods, so it has to be for Buffer and mutable.Map it looks like. unless someone can also make an argument for mutable.Set?

[AprilAtBanno]

immutable-injection...AprilAtBanno:otel4s:immutable-injection

[rossabaker]

Backtracking to 4568b85. We will get @AprilAtBanno's instances and tests on another PR, but this is already a long conversation.

[rossabaker]

trait Otel4s[F[_]] extends Otel4sCommon[F] with Otel4sTrace[F] with Otel4sMetrics[F]

That gets the job done, but it gives cake pattern vibes. We still need Tracer for middleware. Otel4sTrace is capable of creating a Tracer, but not of giving us the one we already have.

In Java, they typically pull the Context from a ThreadLocal. This obviously sucks for asynchrony, but we emulate it with IOLocal. Having an Ask[F, Vault] constraint on method that need it is natural. Otel4s (or Otel4sCommon) could provide an instance of it.

In Go, they pass around a Context explicitly. If you look at it from just the right angle, it's like writing apps in ReaderT[Id, Context, A]... and ReaderT[F, Context] is ... an Ask[F, Context]!

Then we're down to figuring out how to plumb in the propagators, and are kind of back to the classic dilemma: is it an explicit algebra we pass around, or a capability trait that serves as a constraint on F?

[rossabaker]

Are we agreed this is an improvement in current form, and we can continue to talk about the best way to thread things through?

[AprilAtBanno]

I've been thinking about the colour of the shed, and I'm not sure TextMapUpdater is the best name. while "updater" is better than "setter" (the latter of which can only refer to mutation), both mutable and immutable maps in scala have update and updated methods respectively, making the name TextMapUpdater still unclear about whether or not it performs mutation.
however, I also don't have a great name off the top of my head myself