Tracer#joinOrRoot - propagate extracted headers downstream
#301
Conversation
| SpanContext.fromContext(context) match { | ||
| case Some(parent) => | ||
| childScope(parent)(fa) | ||
| Local[F, Vault].scope(childScope(parent)(fa))(context) | ||
| case None => | ||
| rootScope(fa) | ||
| Local[F, Vault].scope(fa)(context) |
There was a problem hiding this comment.
Here is the actual fix
There was a problem hiding this comment.
Dam, this is what I was missing! Had done the same changes as you've done to the context conversions but still couldn't get it working. This is what I was missing. Nice one!
There was a problem hiding this comment.
Yeah, the whole propagation thing is getting complicated.
Perhaps once we separate context carrier (a.k.a. Vault) and context itself in #291 things should get better
There was a problem hiding this comment.
why doesn't TraceScope handle these? I'm not quite understanding
There was a problem hiding this comment.
TraceScope propagation knows nothing about the external JContext, which is stored in the Vault, until we pass it.
So, once we pass the vault with external JContext (that has extract headers), things start to work.
Anytime I'm trying to reproduce the whole propagation process in my brain my brain is getting slightly damaged 😄
|
@lacarvalho91 would you like to take a look? |
There was a problem hiding this comment.
it looks to be largely correct to me and a strict improvement over what we have currently.
however, I don't think it's a long-term solution, for a couple of reasons. my instincts feel that converting between JContext and Vault feels fundamentally brittle, though perhaps I could be convinced otherwise. but more importantly, Vault is not a spec-compliant context, as its keys do not have names for debugging purposes
java/trace/src/main/scala/org/typelevel/otel4s/java/ContextConversions.scala
Show resolved
Hide resolved
| } else { | ||
| context = Scope.Noop.storeInContext(context) | ||
| Scope.Noop.storeInContext(Vault.empty) | ||
| } |
There was a problem hiding this comment.
nittest of picks: don't need { }
| import cats.syntax.apply._ | ||
| import cats.syntax.functor._ |
There was a problem hiding this comment.
| import cats.syntax.apply._ | |
| import cats.syntax.functor._ | |
| import cats.syntax.all._ |
java/trace/src/test/scala/org/typelevel/otel4s/java/trace/TracerSuite.scala
Outdated
Show resolved
Hide resolved
java/trace/src/test/scala/org/typelevel/otel4s/java/trace/TracerSuite.scala
Show resolved
Hide resolved
Definitely! It's a band-aid to move forward. Then, in #291 we can reconsider the implementation details. As I mentioned in #291, 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.
yeah, basically there is no conversion at all. All we do is store |
Closes #277.
While reviewing the #291, I spotted the reason behind #277.