Profiles: simplify profile stack trace representation (ASCII fix, conflicts resolved) #708
Conversation
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
- 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 <[email protected]>
christos68k
changed the title
aalexand
reviewed
Aug 26, 2025
aalexand
approved these changes
Aug 26, 2025
jhalliday
approved these changes
Aug 27, 2025
|
@tigrannajaryan In light of @felixge being away this week, can we merge this PR instead of #645 (and I'm assuming we can still make it in If squash-merging please use 19aa542 for the commit message and verify that @felixge is credited as the author (I fixed merge conflicts and updated the ASCII diagram but the crux of the work belongs to @felixge). If simple merging without a squash, there should be no issue. I cherry-picked @felixge 's commit and he's set as the author. Modifications (conflict resolution) are mine. |
tigrannajaryan
approved these changes
Aug 27, 2025
Merged
tigrannajaryan
pushed a commit
that referenced
this pull request
Sep 2, 2025
### Changed - profiles: drop gzip requirement. [#661](#661) - profiles: avoid `optional` keyword usage. [#659](#659) - profiles: make `profile_id` optional. [#665](#665) - profiles: use single `Profile.sample_type` and clarify use of timestamps. [#649](#649) - all: add notes about the attribute values restrictions. [#683](https://github.com/open-telemetry/opentelemetry-proto/pull/683)<br>⚠️ **IMPORTANT**: These restrictions can be dropped in a future minor release. - profiles: clarify usage of the zero value as the first element of tables in `ProfilesDictionary`. [#688](#688), [#698](#698) - profiles: unsigned `time_nanos` and `duration_nanos` in `Profile`. [#692](#692) - profiles: improve attribute encoding in `ProfilesDictionary`. [#672](#672) - profiles: simplify profile stack trace representation. [#708](#708) ### Fixed - examples: fix OTLP JSON Event example body. [#666](#666) - docs: minor specification fixes around `UNAVAILABLE` and `RetryInfo`. [#669](#669) ### Removed - profiles: remove `default_sample_type`. [#679](#679) - profiles: remove `has_*` debug info fields, they are moving to attributes. [#595](#595) - profiles: remove `Location.is_folded`. [#690](#690)
|
Thanks for handling this @christos68k. LGTM. And sorry I wasn't available. GopherCon is done now, so I'll be responsive again. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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.
Summary
This PR is essentially @felixge 's #645 with the ASCII diagram updated and rebased on top of current main, resolving conflicts.
I opened it in light of @tigrannajaryan 's comment here to try and get it merged before
1.8.0is out as @felixge will probably not have the time to update #645 before the end of this week.Please refer to #645 for extensive comments and review history.