Experimental IOLocalContextStorage #214
Conversation
java/common/src/main/scala/org/typelevel/otel4s/java/IOLocalContextStorage.scala
Outdated
Show resolved
Hide resolved
| rootLog.setLevel(Level.FINE) | ||
| rootLog.getHandlers().head.setLevel(Level.FINE) | ||
| } | ||
| ioLocal <- IOLocal(null: Context) |
There was a problem hiding this comment.
We have to add the wrapper before we refer to Context, or else we get a FINE message that we're being ignored.
|
Does it seamlessly propagate the context? At least from the examples, it seems it does. |
|
I don't think it works at all, because the dispatched fibers have their own IOLocal state. But typelevel/cats-effect#3636 may open a new path. |
java/common/src/test/scala/org/typelevel/otel4s/java/ContextStorageSuite.scala
Outdated
Show resolved
Hide resolved
Co-authored-by: Arman Bilge <armanbilge@gmail.com>
|
6b542aa probably belongs on main for general cleanliness. The motivator here is that the Java SDK loads its own testkit if it's present, in preference to any other SPI implementation, like ours. |
| class IOLocalContextStorageProvider extends ContextStorageProvider { | ||
| def get(): ContextStorage = | ||
| new IOLocalContextStorage( | ||
| Eval.later(IOLocalContextStorageProvider.localContext) |
There was a problem hiding this comment.
We can't access Context.root() before the storage is initialized. Yikes!
java/common/src/main/scala/org/typelevel/otel4s/java/IOLocalContextStorageProvider.scala
Outdated
Show resolved
Hide resolved
| object IOLocalContextStorageProvider { | ||
| val localContext: IOLocal[Context] = | ||
| IOLocal[Context](Context.root()) | ||
| .syncStep(100) |
There was a problem hiding this comment.
I think 1 is sufficient, but this would be a stupid reason to have a fatal error, so I was generous.
|
This is going to want to live in an SPI module. Having more than one on the classpath is like multiple slf4j bindings. |
java/common/src/main/scala/org/typelevel/otel4s/java/IOLocalContextStorage.scala
Outdated
Show resolved
Hide resolved
Co-authored-by: Arman Bilge <armanbilge@gmail.com>
|
I just realised there's a small problem of this not being the same type of context used in the API of the java backend, and I'm not entirely sure how to deal with that |
|
so, with the new context implementation, if we use our own context and just have the custom |
| import io.opentelemetry.context.ContextStorageProvider | ||
|
|
||
| object IOLocalContextStorageProvider { | ||
| val localContext: IOLocal[Context] = |
There was a problem hiding this comment.
any reason not to make this lazy?
|
just want to drop a note that I've been working on this (with Ross' blessing), and have a working implementation with tests. there's just the teeny, tiny problem that it depends not just on an unreleased cats effect version but on one that's not even merged yet. so, uh. not sure quite what to do about that |
Finish implementing `IOLocalContextStorage` and `IOLocalContextStorageProvider` for sharing context modifications between Java and Scala instrumentation.
NthPortal
force-pushed
the
context-storage
branch
from
b48f3b1
to
e60f793
Compare
|
this is ready for review. I'm reluctant to mark it as not a draft however, until typelevel/cats-effect#3636 is merged |
|
|
||
| import java.util.logging._ | ||
|
|
||
| object ContextStorageExample extends IOApp.Simple { |
There was a problem hiding this comment.
should this be removed now that tests exist?
|
@iRevive Has this been accidentally closed? |
|
@razvanz thanks for pointing out! I guess I linked the wrong task/PR, and it closed automatically. |
I think this is doomed, but putting it out there to continue the discussion in #202.
Edit by @NthPortal: it is not necessarily doomed