This repository has been archived by the owner on Jul 19, 2023. It is now read-only.
Use pyroscope tree representation in MergeProfilesStacktraces API #702
Conversation
This file contains 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
kolesnikovae
changed the title
kolesnikovae
commented
May 23, 2023
Comment on lines
55
to
68
| message MergeProfilesStacktracesResult { | ||
| StacktracesMergeFormat format = 3; | ||
| // The list of stracktraces with their respective value | ||
| repeated StacktraceSample stacktraces = 1; | ||
| repeated string function_names = 2; | ||
| // Merge result marshaled to pyroscope tree bytes. | ||
| bytes tree_bytes = 4; | ||
| } | ||
|
|
||
| enum StacktracesMergeFormat { | ||
| MERGE_FORMAT_UNSPECIFIED = 0; | ||
| MERGE_FORMAT_STACKTRACES = 1; | ||
| MERGE_FORMAT_TREE = 2; | ||
| } |
There was a problem hiding this comment.
I was thinking about using oneof but decided to use an explicit enum (because eventually we should use a single format).
I don't feel strong about that – please let me know if you want me to change it
cyriltovena
reviewed
May 24, 2023
| lock.Lock() | ||
| defer lock.Unlock() | ||
| result = append(result, merge) | ||
| m.MergeStackTraces(merge.Stacktraces, merge.FunctionNames) |
There was a problem hiding this comment.
Wouldn't it be better if we return the tree directly from the q.MergeByStacktraces ?
There was a problem hiding this comment.
This is a good point. I'd experiment with this. My idea was to merge samples to the tree (in batches) directly in MergeByStacktraces, and pass the merger as an argument. However, I wasn't 100% sure if this approach is more efficient, and I didn't want to delay shipping of this feature
cyriltovena
reviewed
May 24, 2023
cyriltovena
reviewed
May 24, 2023
| Result: phlaremodel.MergeBatchMergeStacktraces(result...), | ||
| Result: &ingestv1.MergeProfilesStacktracesResult{ | ||
| Format: ingestv1.StacktracesMergeFormat_MERGE_FORMAT_TREE, | ||
| TreeBytes: m.TreeBytes(r.GetMaxNodes()), |
cyriltovena
approved these changes
May 24, 2023
kolesnikovae
force-pushed
the
feat/proflie-tree-representation
branch
from
f814b2e to
449af86
Compare
simonswine
pushed a commit
to simonswine/pyroscope
that referenced
this pull request
Jun 30, 2023
…afana/phlare#702) * Add pyroscope tree to the MergeProfilesStacktracesResult type * Add support for multiple merge result formats * Refactor tree and flamegraph types to the model package * Update reference-configuration-parameters * Add stack truncation implementation notes * Implement tree merge * Fix tree merge * Fix stacktrace rewrite * Refine tree merge * Fix tests * Refactor unused functions * Remove dead code * Fix tree min value counting
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
The PR is aimed at decreasing querier-ingester latency. See #689: