Redesign context implementation

[NthPortal]

Redesign context implementation to use a Context type class that supports arbitrary types for the context rather than specifically Vault. In particular, it supports different context implementations for different backends, and parameterises several types by a C parameter that refers to the type of the context.

• scaladocs
• rebase -i --autosquash

[NthPortal]

I did not anticipate native not working

Edit: new implementation that does not use path dependent types on non-concrete types should fix this

[NthPortal]

(had to rebase to fix conflicts, so I squashed at the same time)

[iRevive]

I like the concept. I will do a thorough review this weekend

[NthPortal]

I've finished writing the scaladocs, though I'm not super thrilled with them and would love feedback (writing good docs is hard)

[iRevive]

Should the class itself be final and the constructor be private?

final class Ops private[Context] (ctx: C)

[NthPortal]

it is modeled after Ordering#OrderingOps from the stdlib, for which neither of those are the case. it allows one to theoretically extend Context with more methods and have a new inner class extend Ops as well to add additional methods. is that use case significant enough? I don't know. I don't feel strongly about it one way or another.

it does remind me however why it's called OrderingOps and not just Ops (so that there isn't a naming conflict with a child Ops class), so I'll tweak that at least

[iRevive]

Is there a way to use OtelJava with Vault as a context?

[NthPortal]

no. unfortunately, Vault is fundamentally incompatible with the API of Context from the Java library (specifically, you cannot store values in a Vault using a ContextKey), precluding interoperability. what we have currently is only a hack to hold onto a few stored values that we care about, but is potentially losing nearly all of the actual context, kind of

[NthPortal]

also, Vault is not compliant with spec, as the otel spec requires that context keys have names for debugging purposes

[lacarvalho91]

its possible to store anything in Vault right? I'm just wondering why Vault can't be used for non span contexts too (the linked issue)

or I guess the opposite could also be asked, if we have this wrapper around JContext and that is good to use - why do we need Vault at all?

[iRevive]

or I guess the opposite could also be asked, if we have this wrapper around JContext and that is good to use - why do we need Vault at all?

If I get it right looking into this branch, you don't need Vault anymore. The otel4s will use IOLocal[org.typelevel.otel4s.java.context.Context] under the hood

[iRevive]

I have a feeling that we are trying to squeeze two concepts into one entity: scope management (former TraceScope) and context storage (e.g. Vault).

What do we want to achieve with these changes? Do we want to allow using something else besides Vault or do we want to tweak the scope management (a.k.a. propagation)? Or both?

[NthPortal]

I have a feeling that we are trying to squeeze two concepts into one entity: scope management (former TraceScope) and context storage (e.g. Vault).

@iRevive I don't think the changes to Context/Vault really touch TraceScope directly—this PR basically just replaces TraceScope with direct calls to Local. with no longer having to worry about storing in a JContext and then storing that in a Vault, the TraceScope implementation simplifies a lot, and it didn't seem worth having a trait for anymore. the only reason I can personally see for keeping TraceScope is to keep all the operations in one file so handling of any sharp edges (like having to match for Noop in rootScope) can be kept together with comments, possibly making mistakes less likely. but I'm not strongly convinced by that argument

[NthPortal]

What do we want to achieve with these changes?

from my perspective, "simply" replacing Vault with a wrapper around JContext, and cleaning up some complexity that arose from using Vault

[lacarvalho91]

What was the motivation for Vault to begin with? I think I saw an issue to document that

[NthPortal]

What was the motivation for Vault to begin with?

probably so the API would work with an eventual scala-only backend as well. as you can see from this PR, doing otherwise requires additional type-parameterisation and a type class

[iRevive]

What was the motivation for Vault to begin with? I think I saw an issue to document that

Vault was a popular 'context-carrier' choice in different projects (e.g. http4s) and, indeed, it's suitable for a Scala-pure implementation

#124 (comment)

[iRevive]

In my opinion, we wrongfully treat Vault as a context when it's a 'context carrier'. In fact, if the app uses IOLocal[Vault] exclusively for tracing, the Vault will always have exactly one entry, which is Scope. And Scope, in general, wraps JContext or JSpan.

So, when we store a custom entry (e.g. a passthrough header), it's not stored directly in the Vault. The entry is stored in the JContext and the JContext (wrapped into the Scope) is stored in the Vault.

Technically, we can eliminate the Vault and introduce a custom Context and use IOLocal[Context] instead. And this PR does this (if I get it right).

[NthPortal]

this needs a solid rebase.

Update: done

[iRevive]

Here are a few thoughts that I have

[iRevive]

I noticed that we don't offer any API to manipulate the context.

If I know a concrete implementation I only need Local[F, java.context.Context] to do this, which is fine:

def middleware(local: Local[IO, org.typelevel.otel4s.java.context.Context]): IO[Unit] =
  for {
    key <- Context.Key.unique[IO, String]("user_id")
    _ <- local.local(
      for {
        ctx <- local.ask[Context]
        _   <- IO.println("new")
        _   <- IO.println(ctx.get(key))
      } yield ()
    )(ctx => ctx.updated(key, "some_id"))
  } yield ()

But how can I do this when I don't know which implementation of a Context will be used?

Let's say I'm a library author and making a middleware using Tracer. I want to manipulate the context (either get or set values under some keys). Currently, the Context type does not appear in the Tracer interface, therefore, I never know which implementation will be used.

def middleware[F[_]: Tracer]: F[Unit] = 
  // how can I set a value for key here?

[iRevive]

We need type Key[A] <: context.Key[A] in the Context so we can make the keys compatible with JContext, right?

val key = Context.Key.unique[SyncIO, String]("user_id").unsafeRunSync()
val ctx = Context.root.updated(key, "some_id_2")

println(ctx.underlying.get(key)) // some_id_2
println(ctx.get(key)) // Some(some_id_2)

So we can use the static keys with the underlying JContext too (e.g. can be handy, once we have a thread-local storage #214).

On the other hand, this dependent type is responsible for the burden.

[NthPortal]

But how can I do this when I don't know which implementation of a Context will be used?

import org.typelevel.otel4s.context.Context.Implicits._

def middleware[F[_]: Tracer: Console: Monad, C, K[X] <: Key[X]](implicit
    L: Local[F, C],
    c: Context.Keyed[C, K],
    kp: Key.Provider[F, K]
): F[Unit] =
  for {
    key <- kp.uniqueKey[String]("user_id")
    _ <- L.local(
      for {
        ctx <- L.ask[C]
        _   <- Console[F].println("new")
        _   <- Console[F].println(ctx.get(key))
      } yield ()
    )(ctx => ctx.updated(key, "some_id"))
  } yield ()

note: you can also use Key.Provider[SyncIO, K] rather than Key.Provider[F, K], and then create keys by calling unsafeRunSync() outside of the for comprehension

I noticed that we don't offer any API to manipulate the context.

my assumption up until now is that whoever creates/provides the Otel4s[F] instance will also provide a Local[F, C] for the appropriate C: Context type. if we'd like to create some type that boxes/wraps Tracer[F] and Context.Keyed[C, K] to make it easier to pass them around, I'm fine with that

[NthPortal]

We need type Key[A] <: context.Key[A] in the Context so we can make the keys compatible with JContext, right?

we need to have some type Key in order to make keys compatible with JContext, yes. in theory, it could be an unbounded type Key[_]; however, the upper bound of context.Key is to ensure that implementors abide by the otel spec (i.e. that keys have names for debug purposes). if you know the backend, you can create static keys either directly using the concrete type as you described, or by summoning a Key.Provider[SyncIO, ConcreteKey] and using that to create keys statically with unsafeRunSync()

[iRevive]

@NthPortal

While working on #325 I was also taking notes from your implementation. Eventually, I came up with the identical trait TextMapPropagator[Ctx] definition in the core module.

The interesting part, is that both implementations (Java and Scala SDK) work completely fine without sharing the Context interface.

What if we don't introduce a Context interface in the core module? We can make changes in smaller pieces and start with the rework of the interfaces, and then we can work with the Context.

I see the scope of the propagator interfaces rework as the following:

• [optional] Move ContextPropagators, TextMapGetter, TextMapPropagator, TextMapSetter, TextMapUpdater to the org.typelevel.otel4s.context.propagation package (module remains the same)
• [optional] Drop F[_] constraint from both ContextPropagators and TextMapPropagator (by removing def inject[A: TextMapSetter](ctx: Vault, carrier: A): F[Unit])
• Add Ctx type parameter to both ContextPropagators and TextMapPropagator
• Update type parameters in the java module
• Update SpanContext: move def storeInContext(context: Vault): Vault and def fromContext(context: Vault): Option[SpanContext] somewhere to the java module (e.g. WrappedSpanContext)
• Move vault dependency from the core-common module to the java-trace module

WDYT?

[NthPortal]

The interesting part, is that both implementations (Java and Scala SDK) work completely fine without sharing the Context interface.

@iRevive I don't fully understand what you mean by this. you are correct that the backends themselves don't need or use the Context typeclass (and in fact having to implement it is a bit duplicative, but such is life). the typeclass exists so that clients or middlewares can be written in a backend-agnostic way

[iRevive]

@NthPortal is there anything else you would like to do in the scope of the PR? Otherwise, I can merge it once the CI is green

[NthPortal]

@iRevive nope, I'll make the build.sbt change you mentioned and then squash it 👍

[NthPortal]

done, green, and ready for merge. that took way too many build attempts 😅

[NthPortal]

thanks @iRevive!