-
Notifications
You must be signed in to change notification settings - Fork 969
OTEP: correlating OBI traces to profiles #4855
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,57 @@ | ||||||||||
| # Correlating Profiles to OBI Traces | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would it be possible to provide additional context (e.g. links if context exists somewhere else) for correlation in general:
Also, what happens when application is getting instrumented and which mechanisms would win?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
there are other proposals which targets SDK traces specifically, see: #4719 and https://docs.google.com/document/d/1eatbHpEXXhWZEPrXZpfR58-5RIx-81mUgF69Zpn3Rz4/edit?tab=t.0#heading=h.fvztn3xtjxxm
I don't think they are, they have different ways of doing the same thing (context propagation) and if a trace exists, it will be ideally inherited both by OBI and SDKs
this OTEP is OBI specific, for SDKs I believe the work/proposal is located here: https://docs.google.com/document/d/1eatbHpEXXhWZEPrXZpfR58-5RIx-81mUgF69Zpn3Rz4/edit?tab=t.0#heading=h.fvztn3xtjxxm
I think it should be something like: or viceversa; if both sources have a trace context and it differs, perhaps it's a bug There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👋 I'm one of the authors of the gdoc mentioned -- we plan to turn it into an OTEP soon. The TL;DR of why we need OBI specifics is that due to the way OBI works it's awkward to use the same mechanism we're planning for regular SDKs and vice-versa. See also #4719 (comment) for more on this discussion.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @lmolkova We discussed the priority/fidelity (who wins?) issue at today's Profiling SIG (also previously raised by me in the proposed implementation here) Given that this OTEP focuses on a specific technical solution (fast data exchange between two OTel components), should we attempt to answer the priority/fidelity question here or somewhere more general? In today's SIG, we discussed that the same clash can be present in the rest of OTel, e.g. if one has multiple layers of instrumentation.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think having more context in the OTEP is helpful and increases chances of people reviewing it.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In my opinion it belongs in a more general document as it would dictate the correlation/priority/etc. of generic readers (eg. the profiler) with writers (eg. OBI, SDKs). Let me know if should I add (or clarify) something OBI specific |
||||||||||
|
|
||||||||||
| This OTEP introduces a standard communication channel and a specification for correlating profiles to [opentelemetry-ebpf-instrumentation (OBI)](https://github.com/open-telemetry/opentelemetry-ebpf-instrumentation) traces. | ||||||||||
|
|
||||||||||
| <!-- toc --> | ||||||||||
| * [Motivation](#motivation) | ||||||||||
| * [Design Notes](#design-notes) | ||||||||||
| * [Communication Channel](#communication-channel) | ||||||||||
| * [Data Model](#data-model) | ||||||||||
| <!-- toc --> | ||||||||||
|
|
||||||||||
| ## Motivation | ||||||||||
|
|
||||||||||
| Currently, OBI traces and profiles operate independently, making it difficult to attribute profiling data to specific traces or spans. By establishing a standard kernel-resident communication channel, this OTEP enables: | ||||||||||
|
|
||||||||||
| - Correlating profiles with their corresponding traces or spans | ||||||||||
| - End-to-end observability workflows without requiring application-level instrumentation | ||||||||||
|
|
||||||||||
| ## Design Notes | ||||||||||
|
|
||||||||||
| ### Communication Channel | ||||||||||
|
|
||||||||||
| The communication channel between OBI and the profiler is implemented via an eBPF map pinned at `$PINPATH/otel_traces_ctx_v1`. | ||||||||||
mmat11 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||
|
|
||||||||||
| $PINPATH will default to bpffs (`/sys/fs/bpf`) but there must be options to specify an alternative location. If set, the user-configured | ||||||||||
|
|
||||||||||
| location in OBI must match the one set in the profiler. | ||||||||||
|
|
||||||||||
| On startup, both OBI and the profiler, will create the map and pin it if it doesn't exist. OBI will expose an helper function for the profiler | ||||||||||
mmat11 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||
|
|
||||||||||
| to do so. | ||||||||||
|
|
||||||||||
| ### Data Model | ||||||||||
|
|
||||||||||
| As described in the [Profiles Data Model](./0239-profiles-data-model.md), the shared eBPF map uses a minimal structure to store correlation data. | ||||||||||
|
|
||||||||||
| #### eBPF Map Specification | ||||||||||
|
|
||||||||||
| ```c | ||||||||||
| struct { | ||||||||||
| __uint(type, BPF_MAP_TYPE_LRU_HASH); | ||||||||||
| __type(key, u64); | ||||||||||
| __type(value, struct trace_context); | ||||||||||
| __uint(max_entries, 1 << 14); | ||||||||||
mmat11 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
| __uint(pinning, LIBBPF_PIN_BY_NAME); | ||||||||||
| } otel_traces_ctx_v1 SEC(".maps"); | ||||||||||
| ``` | ||||||||||
|
|
||||||||||
| - **Key:** `(u64)pid_tgid` | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about Go where goroutines are multiplexed on OS threads? Will this hold a goroutine id instead?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The map will still be indexed by Something similar happens for other languages with different async primitives |
||||||||||
| - **Value:** | ||||||||||
|
|
||||||||||
| ```c | ||||||||||
| struct trace_context { | ||||||||||
| u8 trace_id[16]; | ||||||||||
| u8 span_id[8]; | ||||||||||
|
Comment on lines
+52
to
+53
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason we don't type these as
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. These are opaque 16/8-byte identifiers, not integers. The canonical form (including If we type them as We don't perform arithmetic on these values, so there's no real benefit to representing them as integers. As a side note, keeping them as byte arrays also avoids type-punning/strict-aliasing issues from casting between |
||||||||||
| }; | ||||||||||
| ``` | ||||||||||
Uh oh!
There was an error while loading. Please reload this page.