Update ASCII diagram for new Stack message#2
Closed
christos68k wants to merge 26 commits intofelixge:simpler-stack-trace-v2from
Closed
Update ASCII diagram for new Stack message#2christos68k wants to merge 26 commits intofelixge:simpler-stack-trace-v2from
christos68k wants to merge 26 commits intofelixge:simpler-stack-trace-v2from
Conversation
…metry#685) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
This resolves the ambiguity when a `ScopesProfiles` message contains more than one `profiles` entry matching the `default_sample_type`. Also add a comment to clarify which profile viewers should display by default. This was previously also ambiguous. This change creates a minor problem when it comes to ensuring that pprof profiles can round-trip through otel conversion as it may require the `ScopeProfiles.profiles` entries to be reordered in order to honor the `default_sample_type` of the original pprof message. Naively converting the resulting otel profile back to pprof would cause the order of `Profile.sample_types` to be changed. Merging this change indiciates consensus within the profiling group that this issue should be handled by adding a `pprof.profile_order` attribute (name TBD) to the `ScopeProfiles.attributes` during the initial pprof->otel conversion. This label will allow converting the resulting otel profile back to pprof without any information loss. See [open-telemetrygh-633][] for additional details. [open-telemetrygh-633]: open-telemetry#633 (comment) Co-authored-by: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com>
The has_* debug info fields were derived from the pprof format. We decided to move them to attributes, see open-telemetry/semantic-conventions#2522.
…#684) Updated the relationship between `ScopeProfiles` and `Profile` from 1-1 to 1-n in the relationships diagram. This is a documentation-only change that fixes the diagram to accurately reflect the actual data model. The protobuf message definition shows: ``` message ScopeProfiles { // The instrumentation scope information for the profiles in this message. // Semantically when InstrumentationScope isn't set, it is equivalent with // an empty instrumentation scope name (unknown). opentelemetry.proto.common.v1.InstrumentationScope scope = 1; // A list of Profiles that originate from an instrumentation scope. repeated Profile profiles = 2; ... } ``` The `repeated Profile profiles` field indicates a one-to-many relationship, where a single `ScopeProfiles` can contain multiple `Profile` instances. The inconsistency in the diagram seems to have originated [here](open-telemetry@7312bdf) when removing `ProfileContainer`.
Similar to the boolean attributes `Mapping.has_*` (open-telemetry#595 and open-telemetry/semantic-conventions#2522) also drop `Location.is_folded`. The complementary PR for the OTel SemConv, that builds on top of open-telemetry/semantic-conventions#2522 is florianl/semantic-conventions#1 Co-authored-by: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com>
…-telemetry#688) Makes clear that all dictionary table fields should encode a 'null' element at index 0, such that optional fields pointing into the tables can use 0 value to indicate null.
We are already using unsigned type for [Sample.timestamps_unix_nano](https://github.com/open-telemetry/opentelemetry-proto/blob/b14635e74b7909c9f55db39666c667d4ea3092b7/opentelemetry/proto/profiles/v1development/profiles.proto#L422-L424) but not for the time-related fields in [Profile](https://github.com/open-telemetry/opentelemetry-proto/blob/b14635e74b7909c9f55db39666c667d4ea3092b7/opentelemetry/proto/profiles/v1development/profiles.proto#L235-L237). Also see: - open-telemetry#691
…metry#697) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…emetry#703) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…metry#672) Subsequent to open-telemetry#659 (profiles: avoid 'optional' keyword usage) and in accordance with discussions in the profiling SIG, this PR updates the dictionary message docs to impose a more consistent handling of 'null pointer' semantics in the encoding pattern.
florianl
approved these changes
Aug 22, 2025
…emetry#704) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
- Introduce a first-class Stack message type and lookup table. - Replace location index range based stack trace encoding on Sample with a single stack_index reference. - Remove the location_indices lookup table. The primary motivation is laying the ground work for [timestamp based profiling][timestamp proposal] where the same stack trace needs to be referenced much more frequently compared to aggregation based on low cardinality attributes. Timestamp based profiling is also expected to be used with the the upcoming [Off-CPU profiling][off-cpu pr] feature in the eBPF profiler. Off-CPU stack traces have a different distribution compared to CPU samples. In particular stack traces are much more repetitive because they only occur at call sites such as syscalls. For the same reason it is also uncommon to see a stack trace are a root-prefix of a previously observed stack trace. We might need to revisit the previous [previous benchmarks][benchmarks] to confirm these claims. The secondary motivation is simplicitly. Arguably the proposed change here will make it easier to write exporters, processors as well as receivers. It seems like we had rough consensus around this change in previous SIG meetings, and it seems like a good incremental step to make progress on the timestamp proposal. [timestamp proposal]: open-telemetry#594 [off-cpu pr]: open-telemetry/opentelemetry-ebpf-profiler#196 [benchmarks]: https://docs.google.com/spreadsheets/d/1Q-6MlegV8xLYdz5WD5iPxQU2tsfodX1-CDV1WeGzyQ0/edit?gid=2069300294#gid=2069300294 Modified-by: Christos Kalkanis <christos.kalkanis@elastic.co>
christos68k
force-pushed
the
ck/fix-ascii
branch
from
9fbeda6 to
e147f2a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
10 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Also moved the leaf frame docstring inside the
Stackmessage, to the field (see here).