*: Icicle charts support #5383
*: Icicle charts support #5383
Conversation
|
✅ Meticulous spotted zero visual differences across 166 screens tested: view results. Meticulous simulated ~4 hours of user flows against your PR. Expected differences? Click here. Last updated for commit 5924c82. This comment will update as new commits are pushed. |
There was a problem hiding this comment.
If I'm understanding this right, we don't yet have the ability to merge stacks that are "next to each other", correct? I'm not sure yet whether we will need to re-think whether flamegraphs and flamecharts can be rendered with the same code since one cares about order and the other doesn't.
pkg/query/flamegraph_arrow.go
Outdated
| durationStr := strconv.FormatInt(r.Duration.Value(i), 10) | ||
| _, _ = tsHasher.Write([]byte(tsStr)) | ||
| _, _ = tsHasher.Write([]byte(durationStr)) | ||
| tsHash = tsHasher.Sum64() |
There was a problem hiding this comment.
I don't think we need to do any of this hashing. Timestamp is already an int64, so just do uint64(timestamp).
pkg/query/flamegraph_arrow.go
Outdated
| tsHash = tsHasher.Sum64() | ||
|
|
||
| sampleTsRow := row | ||
| if row, ok := fb.rootsRow[tsHash]; ok { |
There was a problem hiding this comment.
for flamecharts having a root for a timestamp that already exists means we can't possibly build a correct flamechart, we should error on this case
There was a problem hiding this comment.
I'm sorry I don't understand why this has to be errored.
So what I'm trying to here is convert the following records
[{
ts: timestamp-1
samples: ....
},
{
ts: timestamp-2
samples: ....
},
{
ts: timestamp-1
samples: .....
}]
to
[{
ts:timestamp-1
samples: ....
},
{
ts: tiemstamp-2
samples: ....
}]
So that we'll have all samples with the same timestamp are grouped under a single root, that is easier for rendering in the UI.
Isn't this similar to the group by label here:
There was a problem hiding this comment.
There can't ever be two samples at the same timestamp for the same thread or CPU. Merging should happen when end timestamp == next timestamp (within some error of margin because clocks aren't perfectly precise, maybe 1-10 ms).
There was a problem hiding this comment.
And end timestamp = timestamp + value
| @@ -284,6 +325,10 @@ func generateFlamegraphArrowRecord(ctx context.Context, mem memory.Allocator, tr | |||
| if fb.aggregationConfig.aggregateByLocationAddress { | |||
| key = hashCombine(key, r.Address.Value(j)) | |||
| } | |||
| if fb.aggregationConfig.aggregateByTimestamp { | |||
| key = hashCombine(key, uint64(r.Timestamp.Value(i))) | |||
| key = hashCombine(key, uint64(r.Duration.Value(i))) | |||
There was a problem hiding this comment.
given the other comments, we shoudn't need the duration to be part of the hash
| case profile.ColumnTimestamp: | ||
| ts := r.Timestamp.Value(sampleRow) | ||
| b.Append([]byte(fmt.Sprint(ts))) | ||
| case profile.ColumnDuration: |
There was a problem hiding this comment.
same (we can use value to tell us how wide the frame should be)
Yes merging of stacks is not implemented yet.
This is handled in the UI where we only change the way we render the root node, so that the ordering is handled for flamecharts. Or am I missing something here? Please let me know. |
|
Rendering is a bit overused of a term, I just meant in regards to the backend code rendering the API response. That the UI does the sorting is ok-ish for now, but I'm unsure that this will scale. |
No description provided.