-
Notifications
You must be signed in to change notification settings - Fork 222
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
ft: Filter by function
functionality added to ProfileView
#2062
Conversation
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.
This looks good to me! As mentioned on Discord, I'm working on the visualization top bar redesign, which may slightly shift some things around in terms of UI (which we can of course discuss), but the functionality looks on point!
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 like this feature a lot! A few small comments, but I think we're very close!
pkg/query/columnquery.go
Outdated
@@ -196,6 +221,7 @@ func (q *ColumnQueryAPI) renderReport(ctx context.Context, p *profile.Profile, t | |||
Report: &pb.QueryResponse_Top{Top: top}, | |||
}, nil | |||
case pb.QueryRequest_REPORT_TYPE_CALLGRAPH: | |||
fmt.Println("callgraph") |
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.
debugging leftover I assume?
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.
Ahh, yes, my bad.
pkg/query/columnquery.go
Outdated
func filterProfileData(p *profile.Profile, filterQuery string) *profile.Profile { | ||
filteredSamples := []*profile.SymbolizedSample{} | ||
for _, s := range p.Samples { | ||
var lines []profile.LocationLine |
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.
We don't need to copy here. I would suggest the inside of this loop should be extracted to a separate function that's just called keepSample
and if it returns true, then we append to filteredSamples
. All the keepSample
function then does is loop over all lines of all locations and on the first match it returns true
and otherwise if no location line contained the search string, then it returns false.
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.
Done
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.
Love it! lgtm
No description provided.