Implement process context sharing OTEP-4719 - #8718
Conversation
|
@ivoanjo If you have time I'd appreciate you looking this over from the perspective of compliance to the spec. It's largely from your demo code so hopefully it's not too far off. @jack-berg Thoughts on package structure, Java 25+ use, dependencies? It's pulling in existing protobuf support from the exporters rather than reinventing it. I've not got gradle doing quite the right thing yet - it builds all the tests, but doesn't recognize the Java 25 ones as parts of the test set, so doesn't actually run them :-( Other than that, it's good enough as a starting point for discussions. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8718 +/- ##
============================================
- Coverage 91.65% 91.27% -0.38%
- Complexity 10352 10472 +120
============================================
Files 1003 1006 +3
Lines 27210 28277 +1067
Branches 3199 3569 +370
============================================
+ Hits 24939 25810 +871
- Misses 1566 1674 +108
- Partials 705 793 +88 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Some notes on the dependencies: since protobuf encoding is bundled with gRPC support, the required protobuf bits wind up in the exporters jars, as follows: opentelemetry-exporter-common: opentelemetry-exporter-otlp-common: The last one there is a bit of a hack caused by reusing the exporter's wire plugin for protobuf codegen. It could be moved, but as long as processcontext depends on the otlp module for other things anyhow it's not a priority. |
Pull request dashboard statusWaiting on the author · refreshed 2026-08-20 15:30 UTC Move out of draft to request review. Status above doesn't look right?
|
| // memfd_create("OTEL_CTX", MFD_CLOEXEC | MFD_ALLOW_SEALING | MFD_NOEXEC_SEAL) | ||
| // note that we don't fallback to retry without MFD_ALLOW_SEALING, because the panama API | ||
| // doesn't give us a good way of determining the cause of failure. | ||
| int fd = -1; | ||
| try (Arena arena = Arena.ofConfined()) { | ||
| MemorySegment nameSegment = arena.allocateFrom(OTEL_NAME); | ||
| fd = | ||
| (Integer) | ||
| MEMFD_CREATE.invoke(nameSegment, MFD_CLOEXEC | MFD_ALLOW_SEALING | MFD_NOEXEC_SEAL); |
There was a problem hiding this comment.
A couple of possible spec compliance gaps to sanity-check:
-
MFD_NOEXEC_SEALretry. Spec publication step 2 says if the initialmemfd_createfails, retry with justMFD_CLOEXEC | MFD_ALLOW_SEALING(droppingMFD_NOEXEC_SEAL, which is newer). The code goes straight to the anonymous fallback instead. On kernels that rejectMFD_NOEXEC_SEALthis means missing the memfd path entirely. (The existing comment mentionsMFD_ALLOW_SEALING, but I think the spec's intent is to dropMFD_NOEXEC_SEAL.) -
prctlon update. Updating protocol step 7 saysprctl(PR_SET_VMA, ...)MUST be re-issued unconditionally on every update.header.update()inpublish()doesn't callinitializeName(). Since the VMA is the same one, is this being deliberately elided as a no-op, or an oversight?
|
|
||
| java { | ||
| sourceSets { | ||
| create("java25") { |
There was a problem hiding this comment.
I don't think we need to have a java 8 and java 25 source set. Here's a commit that merged them together (along with some other minor changes): jack-berg@a1d3f7c
I believe this also fixes the gradle problem where the tests weren't running.
If we end up having a java8 version of context sharing, we can split the module at that point.
|
@jack-berg thanks for the review. I'll address many of the more straightforward points individually, but there is a group that comes under the heading of 'how the heck do we wire this thing up': The source splits out 25 specific bits because the entry point, ProcessContextPublisher, is deliberately not 25 specific. Admittedly the only useful implementation of it currently needs 25, but nothing other than our lack of will would prevent e.g. a JNI implementation. More importantly, nothing prevents a 8+ NoopPublisher, which may be useful if we want to expose wiring that isn't itself 25 specific, such as having a non-null Publisher instance field elsewhere in the SDK. Conceptually the header is a singleton, or at least the memory it's wrapping is, but I'm more inclined to enforce that by making the publisher a singleton and keep the header as an implementation detail. That's partly why publish is a method rather than handled by the constructor - we can init a static final INSTANCE field at classload even if the ProcessContextData.Resource isn't ready yet. Contrast that to the golang sdk proposal Note also the shutdown method in the golang design. It's not defined in the OTEP itself and I'm not 100% sure it makes sense or is safe. You can invalidate the header, but deleting it is problematic for any external consumer that has cached its address. It follows that either close/shutdown is called only at JVM exit, or can invalidate but not free the header memory. Do we have a use case for it? The first call to publish, the caller of which may or may not lazily create the singleton publisher instance, needs to happen after the process context Resource is available. Is there a point in the agent/SDK init where we know that? Does the Resource data ever change incrementally during the process lifetime, or is it frozen? That determines when update may need to be called and hence what may need to be able to get a handle on the publisher. Related to that, the Attributes part is the link between process context and the thread context. To save space and repeated serialization effort, the dictionary in thread context puts its keys, the set of which should change infrequently, in the process context - that's what the Attributes are for. ref So from time to time the thread context publisher will want to call publish on the process context publisher. It follows that it needs the ProcessContextData, but may have only the Attributes, not the Resource. Alternative design is to have an update method on the publisher that takes only the Attributes and reuses the existing Resource. Speaking of the thread context, I think any user that wants one will want the other, so bundling it in the same module may make sense. In which case 'processcontext' is the wrong name. There is already 'context' used elsewhere in the codebase, so perhaps 'contextsharing' or 'contextpublishing' for this module? Anyhow, I'm open to suggestions on the wiring, but it feels to me like we need some form of factory/init that selects a singleton publisher impl instance on the basis of jvm version, plus perhaps an on/off config property, though I'm not clear in what circumstances a user would want to disable this feature. From there, a static lookup that allow users to get that singleton. I don't see the usecase for a shutdown(), but adding one isn't hard as long as we don't mind leaking the header memory and don't ever need to reopen it. We also need a call somewhere in the SDK init to pass the process Resource to publish(). |
|
@jack-berg please would you take a look at the build failures? I think they are due to build/test envs not using 25. That said, I'm also fairly use my build.gradle.kts isn't quite right yet. It's using 25 with what is effectvely source=25,target=25 for everything, whilst we really only want that for the java25 bits whilst having target=11 for the rest, but I can't figure out how to make it apply at that granularity. On a similar note, I had to disable animal-sniffer, which dislikes 25. Is leaving it off for this module ok, or do we need to fork the existing sniffer config to make a variant for 25? |
2 participants
OTEP-4719: Introduce a standard mechanism for OpenTelemetry SDKs to publish resource attributes for access by out-of-process readers such as the OpenTelemetry eBPF Profiler.
Elements of this implementation uses panama FFM (stable in Java 22+) to access native methods. It is not proposed to support process context on JDK releases earlier than 25.