-
Notifications
You must be signed in to change notification settings - Fork 717
Allow retrieving heatmap from spans/individual profiles #4736
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
Conversation
ba47839 to
eb96f68
Compare
Label intersection when reusing labelSets was not run, also empty label set used.
aleks-p
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
pkg/model/heatmap/range.go
Outdated
| } | ||
|
|
||
| // createYAxisBuckets creates evenly spaced Y-axis buckets | ||
| func createYAxisBuckets(minValue, maxValue uint64, numBuckets int) []yBucket { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use []int64 for Y buckets like for time buckets or does tracking min and max make some things easier?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mistake was made in the Exemplar proto API, not too sure this should either be int64 or straight a double: int64 is used in parquet and that is using it because of pprof. Our API Repsonse is using doubles (so is SelectSeries.
I would argue those values should be an int64. Will provide a PR in main after this merges
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will provide a PR in main first, before fixing it here
marcsanmi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job! 👏
pkg/model/heatmap/range.go
Outdated
|
|
||
| // findTimeBucket finds the index of the time bucket for a given timestamp | ||
| func findTimeBucket(timestamp int64, buckets []int64) int { | ||
| for i, bucket := range buckets { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would a binary search be faster for the already sorted buckets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depends on the amount of points, it should really be calcuated
| } | ||
|
|
||
| // This attribute table is used to store the attribute values for the heatmap. It might be reused for other queries at a later time. | ||
| type attributeTable struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created a specific attributetable model in my PR. If it makes sense, we might want to reuse it once merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes let's do that whatever merges first
This PR implements a complete heatmap query API for visualizing profile data distribution over time and value dimensions, with support for both individual profiles and span-level data.
Key Changes: