*: filter profiling data using binary name #4554
Conversation
|
🤖 Meticulous spotted visual differences in 259 of 435 screens tested: view and approve differences detected. Last updated for commit 95e799a. This comment will update as new commits are pushed. |
yomete
changed the title
| @@ -257,6 +257,9 @@ message QueryRequest { | |||
|
|
|||
| // which runtime frames to filter out, often interpreter frames like python or ruby are not super useful by default | |||
| optional RuntimeFilter runtime_filter = 10; | |||
|
|
|||
| // which binary to filter by | |||
| optional string binary_filter = 11; | |||
There was a problem hiding this comment.
this should be a repeated field, and I think we can future proof here a bit, and make this a repeated "filter" and a filter can then be of varying kinds, and the first one would be the binary filter, that way we can then define it to be multiple binaries (which in the Go code we would turn into a set and in the actual filter don't do a byte-match but instead check if the binary is in the set)
| // keep track of how many values we need to keep to reserve the space upfront | ||
| newLen++ | ||
| } | ||
| keepValues[indices.(*array.Int32).Value(i)]++ |
There was a problem hiding this comment.
you're missing this line for the Uint32 case
|
|
||
| var indices interface{} | ||
|
|
||
| switch arr.Indices().(type) { |
There was a problem hiding this comment.
you can do something much simpler here:
switch arr := arr.Indices().(type) {
then arr is already typed to the case and you no longer need to type assert anything
| @@ -111,15 +131,26 @@ func CompactDictionary(mem memory.Allocator, arr *array.Dictionary) (*array.Dict | |||
| } else { | |||
| indexBuilder = array.NewUint64Builder(mem) | |||
| } | |||
| indexBuilder.Reserve(indices.Len()) | |||
|
|
|||
| switch arr.Indices().(type) { | |||
There was a problem hiding this comment.
same as the first type switch
nevermind, .Len exists on array.Array, this type switch shouldn't be necessary at all
| @@ -41,15 +40,36 @@ func CompactDictionary(mem memory.Allocator, arr *array.Dictionary) (*array.Dict | |||
|
|
|||
| newLen := 0 | |||
| keepValues := make([]int, arr.Dictionary().Len()) | |||
| for i := 0; i < indices.Len(); i++ { | |||
|
|
|||
| var indices interface{} | |||
There was a problem hiding this comment.
we don't need this at all, it complicated the code further down as well
| b.Append(uint64(newValueIndex)) | ||
| } | ||
| } | ||
| case *array.Uint32: |
| // keep track of how many values we need to keep to reserve the space upfront | ||
| newLen++ | ||
| } | ||
| keepValues[indices.(*array.Int32).Value(i)]++ |
There was a problem hiding this comment.
this is very important and is missing in both switch cases
There was a problem hiding this comment.
I have no idea why but the linter/precommit keeps removing this line
pkg/parcacol/querier.go
Outdated
|
|
||
| newValues := compactedDict.Dictionary().(*array.Binary) | ||
|
|
||
| for i := 0; i < valueDict.Len(); i++ { |
There was a problem hiding this comment.
ughhh thanks for spotting that
pkg/parcacol/querier.go
Outdated
| } | ||
|
|
||
| val := stringCol.Value(i) | ||
| seen[strings.TrimPrefix(val, "labels.")] = struct{}{} |
pkg/query/columnquery.go
Outdated
| @@ -905,3 +917,32 @@ func sliceRecord(r arrow.Record, indices []int64) []arrow.Record { | |||
| slices = append(slices, r.NewSlice(cur.Start, cur.End)) | |||
| return slices | |||
| } | |||
|
|
|||
| func (q *ColumnQueryAPI) GetMappingFiles( | |||
|
|
||
| // invert_call_stack inverts the call stacks in the flamegraph | ||
| optional bool invert_call_stack = 11; | ||
|
|
||
| // a varying set of filters to apply to the query request | ||
| Filters filters = 12; |
There was a problem hiding this comment.
Oh apologies for missing this earlier, this should be repeated, it should be possible to set multiple filters.
| // FunctionNameStackFilter is a filter for filtering by function names | ||
| message FunctionNameStackFilter { | ||
| // filter is the list of function names to filter by | ||
| repeated string functions_to_filter = 1; |
There was a problem hiding this comment.
this shouldn't be repeated, the top level filter should be repeated (what I commented in the other comment)
| } | ||
|
|
||
| // Filters is a set of filters to apply to the query request | ||
| message Filters { |
This PR adds functionality to filter the icicle graph data by a binary. This was achieved in two parts:
by adding a
frameFilterparameter to the query request which is an array of strings so we can filter by one or many binaries.by implementing a new metadata call to fetch all mapping files (binary) attached to a query as well as other pprof labels. The new metadata call is implemented using a new
REPORT_TYPE,REPORT_TYPE_PROFILE_METADATAon the query request. This was needed because if we filter the icicle graph data using the binary, after filtering we won't know the "total" set of binaries anymore, only those that we filtered by. But with this new call, we can always display all the binaries, irrespective of what it is being currently filtered by.Additional things included: