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

feat: CallGraph API #3178

Closed
wants to merge 2 commits into from
Closed

feat: CallGraph API #3178

wants to merge 2 commits into from

Conversation

kolesnikovae
Copy link
Collaborator

@kolesnikovae kolesnikovae commented Apr 8, 2024

Resolves #3219

The PR introduces a new endpoint for querying profiling data in the call graph format. The format is greatly inspired by the pprof's implementation: https://github.com/grafana/pyroscope/blob/main/pkg/frontend/dot/graph/graph.go#L50

Despite the fact that a call graph can be built from a flame graph or a profile in pprof format, the new use-case specific API will significantly help us integrate call graphs into the Pyroscope UI.

The issue lies in the fact that truncating flame graphs/pprof profiles is not suitable for call graphs: the heuristics we use to trim insignificant nodes make the call graph incorrect. A workaround could be keeping all the nodes (disabling the truncation); however, this will not work well with large profiles. Thus, it's preferable to build call graphs in the backend, trimming nodes in a way optimized for call graphs; otherwise, we will be juggling between incorrect call graph structures/numbers and very poor performance.

TODO:

  • Define the call graph DTO
  • Add RPC service endopoint
  • Implement the endpoint in query-frontend

@kolesnikovae kolesnikovae changed the title draft CallGraph API types feat: CallGraph API Apr 8, 2024
Comment on lines +165 to +171
message CallGraphNode {
// Values associated to this node. Self is exclusive to this node,
// total includes all descendents.
uint64 total = 1;
uint64 self = 2;
string name = 3;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note that we use strings without indirection as in flame graph or pprof formats. The reason is that call graph nodes never duplicate; therefore, the names are meant to be unique.

In contrast to the call graph implementation in the pprof tool, we only keep the display name of the function. Function details such as file name, line number, versions, etc. should be retrieved via separate API (shared with flame graphs).

@kolesnikovae
Copy link
Collaborator Author

/cc @petethepig @aocenas

@aleks-p
Copy link
Contributor

aleks-p commented Apr 10, 2024

Could we use this to replace what was done in #2808?

@kolesnikovae
Copy link
Collaborator Author

Could we use this to replace what was done in #2808?

Yeah, ideally we should be using the new API there as well

@aocenas
Copy link
Member

aocenas commented May 22, 2024

For my 2 cents here, this looks good. Structurally the same shape as what we send to NodeGraph already so would not need too much transformation.

We can also either "hide" it behind the flamegraph query so the user won't know there are 2 API calls being made.

@kolesnikovae
Copy link
Collaborator Author

kolesnikovae commented Jun 27, 2024

Closing it for now as it is not in active development yet

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.

Add CallGraph query API
3 participants