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: optimized pprof symbolication #2679

Merged
merged 11 commits into from
Nov 23, 2023
Merged

feat: optimized pprof symbolication #2679

merged 11 commits into from
Nov 23, 2023

Conversation

kolesnikovae
Copy link
Collaborator

@kolesnikovae kolesnikovae commented Nov 14, 2023

The main goal of the change is to make generated pprof proto type the only profile representation we deal with in the backend:

  • Use of profile.Profile does not allow implementing node truncation.
  • Tree representation we currently use for building a flamegraph does not allow including location information. Synthetic benchmarks show that use of pprof proto Profile decreases CPU time needed to built a profile by up to 300% (1.5s -> 350ms), and allocated memory by up to 100%.

Next steps:

  1. Add support for max_nodes parameter to pprof query #2728
  2. Profile truncation should preserve top-N stack traces with largest "self"  #2729
  3. Switch to pprof in SelectMergeStacktraces API

Dependent issues:

  1. Add locations to the flamegraph #2727
  2. Function metric #2228

@kolesnikovae kolesnikovae changed the title WIP: feat: optimized pprof symbolication feat: optimized pprof symbolication Nov 20, 2023
@kolesnikovae kolesnikovae marked this pull request as ready for review November 20, 2023 09:55
@kolesnikovae kolesnikovae requested a review from a team as a code owner November 20, 2023 09:55
@knylander-grafana
Copy link
Contributor

@kolesnikovae This looks like an interesting feature. Should we add doc for this?

@cyriltovena
Copy link
Contributor

3. Switch to pprof in SelectMergeStacktraces API

This is more an performance optimisation so far so not much to document yet.

Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

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

LGTM

@kolesnikovae
Copy link
Collaborator Author

@kolesnikovae This looks like an interesting feature. Should we add doc for this?

The change only affects internals with no impact on the end user in terms of the system behaviour and mechanics. This is a preparation step for some of the features we want to implement (for which we will need docs, indeed)

@kolesnikovae kolesnikovae merged commit 104465c into main Nov 23, 2023
17 of 19 checks passed
@kolesnikovae kolesnikovae deleted the feat/pprof-truncate branch November 23, 2023 11:06
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.

None yet

3 participants