Skip to content
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

Call View Panel #4678

Merged
merged 24 commits into from
Jun 4, 2024
Merged

Call View Panel #4678

merged 24 commits into from
Jun 4, 2024

Conversation

manojVivek
Copy link
Contributor

@manojVivek manojVivek commented May 28, 2024

Screencast:

call-view-1.mov

Copy link

alwaysmeticulous bot commented May 28, 2024

🤖 Meticulous spotted visual differences in 241 of 336 screens tested: view and approve differences detected.

Last updated for commit 3a1b402. This comment will update as new commits are pushed.

Copy link
Member

@brancz brancz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should do a few things differently:

  1. we should not go further down the path where the function name is the only identifying factor of a line (going further down this path means we won't be able to do group bys)
  2. we can get the caller callee relationships easier than implemented here, we can just keep the "previous row number" when we iterate through the stack and then at every step adjust the callees of the previous row
  3. we should build the relationships directly as row numbers instead of function names

@manojVivek manojVivek requested a review from brancz May 29, 2024 07:19
@@ -187,8 +192,8 @@ type tableBuilder struct {
cumulative int64
addresses map[string]map[uint64]int
functions map[string]int
callers map[string]map[string]bool
callees map[string]map[string]bool
callers map[int]map[int]bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for bool here, just make this a set by using map[int]struct{}

pkg/query/table.go Outdated Show resolved Hide resolved
pkg/query/table.go Outdated Show resolved Hide resolved
pkg/query/table_test.go Outdated Show resolved Hide resolved
pkg/query/table_test.go Outdated Show resolved Hide resolved
pkg/query/table.go Outdated Show resolved Hide resolved
@manojVivek manojVivek changed the title wip: Call view Call view Panel Jun 4, 2024
@manojVivek manojVivek changed the title Call view Panel Call View Panel Jun 4, 2024
}

return tb
}

func (tb *tableBuilder) populateCallerAndCalleeData() {
for i := range tb.builderFunctionName.Len() {
if len(tb.callers) > i {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need a comment explaining these if statements, if I understand correctly, I think we need it to protect against the case where we didn't expand the callers to the row number (let's say the last row in the flamegraph table doesn't have callers or callees)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, added comments on these conditions.

@manojVivek manojVivek merged commit 694a7a4 into main Jun 4, 2024
35 checks passed
@manojVivek manojVivek deleted the call-view branch June 4, 2024 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants