opentelemetry-process-context: implement Resource publishing (OTEP-4719)#5337
opentelemetry-process-context: implement Resource publishing (OTEP-4719)#5337herin049 wants to merge 44 commits into
Conversation
|
Key components of this PR are written in Rust, reviewers of this repo may not be experts in this language. I suggest adding comments to all the rust files for ease of understanding: publish.rs.txt |
ocelotl
left a comment
There was a problem hiding this comment.
I found a few things that may be required in the OTEP but missing here. If so, they are all very small changes to this PR.
A few other things are minor but also worth of considering.
ocelotl
left a comment
There was a problem hiding this comment.
Some more automatic review comments ✌️
Co-authored-by: Diego Hurtado <ocelotl@users.noreply.github.com>
aabmass
left a comment
There was a problem hiding this comment.
Very cool! My hunch is a lot of this rust code which implements the exposition format from the OTEP is not python specific, and maybe the OTEP authors have already written it or would be open to maintaining it elsewhere since it could apply across languages.
I'm not super up to date on this though
There is a PR for this package in the Rust repo: https://github.com/open-telemetry/opentelemetry-rust/pull/3460/changes#diff-a475efbb673d2bddd008746034e46ce4acd67d12f3629d7a2fd57120659276d7 But, it introduces a lot of heavy dependencies that we should try to avoid. Maybe there is a discussion to be had about making this lighter weight? I'm also unsure of whether that implementation is complete since it doesn't appear to handle forking properly from what I can tell. |
Are there use cases where it would need to be called from python code? |
This PR only contains the logic to publish the There will be a follow-up PR which will call |
Ahh thanks I missed that makes sense. Do you mind updating the PR title with "Sharing Resource Attributes". If it's OK, I'd like to wait for @lzchen to take a pass at the Rust code and approve the approach. |
|
I think one question should be answered in the description of this PR: why does this need native code? My understanding is that we need it for the thread local stuff that is not an OTEP yet. When I prototyped an ancient version of this (the protocol was different, a socket was involved and the the profile read to well-known locations instead of memfd) I implemented a small wrapper on a thread_local variable in C and handled everything in Python: |
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| platform: |
There was a problem hiding this comment.
Does the epbf profiler works on these many platforms? I think it runs on arm64 and amd64 only.
I did mention this in the SIG that a large portion of this logic could be implemented without native code (maybe all), but practically it made more sense to keep it in native code for the following reasons:
Also, (as you mentioned), the thread context proposal will very likely be accepted which requires native code to be introduced anyways. |
|
This PR has been automatically marked as stale because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 days of this comment. |
|
Don't close this PR |
# Conflicts: # pyproject.toml # scripts/eachdist.py
Description
External profilers and observability tools (such as the OpenTelemetry eBPF Profiler) operate outside the instrumented process and have no access to resource attributes configured inside OpenTelemetry SDKs. OTEP-4719 (Process Context Sharing) introduces a standard mechanism for OpenTelemetry SDKs to publish resource attributes for access by out-of-process readers
This PR introduces the
opentelemetry-process-contextpackage as a Rust backed Python extension (built with maturin/pyo3) that implements the process outlined in OTEP-4719. It publishes the SDK's resource attributes toa memory region that any external reader with access to
/proc/<pid>/mapsand/proc/<pid>/memcan discover.Native Code
While a large portion of this logic in this package could be implemented without native code, practically it makes more sense to keep it as native code for the following reasons:
madvise), (although,ctypescould be used)prostRust crate, we eliminate the runtime dependency onprotobufPython API
The introduced Python API has a very light surface area, consisting of two methods to publish and unpublish the process context.
Compatibility
The original OTEP explicitly mentions that this functionality is Linux only. However, I have tried to implement this package in such a way that limited testing/development can be performed natively on any Posix compatible system (e.g. MacOS). However, the full functionality of OTEP-4719 is only available on Linux and Windows is explicitly not supported.
memfd_create("OTEL_CTX")->mmap(MAP_PRIVATE)mmap/proc/<pid>/mapsshowsmemfd:OTEL_CTX/proc)madvise(MADV_DONTFORK)prctl(PR_SET_VMA_ANON_NAME, "OTEL_CTX")CLOCK_BOOTTIMECLOCK_MONOTONICFixes #5291
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Does This PR Require a Contrib Repo Change?
Checklist: