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

Do not drop legacy profile alloc metrics when they are equal to in-use ones #433

Closed
wants to merge 1 commit into from

Conversation

aalexand
Copy link
Collaborator

Fixes #432.

In #432 the user has a number of collected heap profiles in the legacy
format that they ask pprof to merge and open and the merge fails.

The reason for the error is that one of the profiles has the alloc
metric values equal to the in-use ones in the header line. Since pprof
legacy profile parser drops the alloc metrics in this case, the profiles
can't be merged since the merge requires that the sample types are the
same between merged profiles.

This PR makes the parser stop dropping the alloc metrics in this case.
This behavior seems more correct anyway as there doesn't seem to be a
strong reason for such implicit behavior and in case of parsing
profile.proto profiles there isn't such implicit behavior either. But
may be I am missing some history here, so please review.

@codecov-io
Copy link

codecov-io commented Oct 28, 2018

Codecov Report

Merging #433 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #433      +/-   ##
==========================================
+ Coverage   67.09%   67.14%   +0.05%     
==========================================
  Files          37       37              
  Lines        7594     7594              
==========================================
+ Hits         5095     5099       +4     
+ Misses       2096     2093       -3     
+ Partials      403      402       -1
Impacted Files Coverage Δ
profile/legacy_profile.go 77.31% <100%> (+0.67%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ef8d84...169f465. Read the comment docs.

rauls5382
rauls5382 previously approved these changes Oct 29, 2018
Copy link
Contributor

@rauls5382 rauls5382 left a comment

Choose a reason for hiding this comment

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

This check can be tracked down to a similar check on the old pprof from gperftools:
https://github.com/gperftools/gperftools/blob/e9ab4c53041ac62feefbbb076d326e9a77dd1567/src/pprof#L4108

It was for dealign with very old profiles on the legacy format. I'm not sure if gperftools is still generating profiles that matches this. If so, that may be a concern as this change would make profiles before and after this change incompatible. Please confirm that before submitting.

The best solution would be to update the profile source to use profile.proto directly, instead of the legacy format.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pprof: problem fetching source profiles: inconsistent samples type count: 2 != 4
4 participants