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

pprof: fix incorrect parsing for locations #62

Merged
merged 1 commit into from Jul 15, 2017

Conversation

Projects
None yet
5 participants
@irfansharif
Copy link
Contributor

irfansharif commented Jul 12, 2017

Fixes #61.

Extrapolating from
https://github.com/google/pprof/blob/master/profile/profile.go#L360 and
https://github.com/uber/go-torch/issues/61 we can have locations of the
following format:

<id>: 0x<address> M=<mapping id>

Note: This doesn't address all malformed inputs, malformed inputs that
may have gotten through before will still do so. This patch only
addresses #61.

Added the raw pprof out as an additional test for one of our binaries
that failed prior.

@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Jul 12, 2017

CLA assistant check
All committers have signed the CLA.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 12, 2017

Coverage Status

Coverage increased (+0.02%) to 95.455% when pulling 77a54e5 on irfansharif:fix-61 into 55a555d on uber:master.

@tbg

This comment has been minimized.

Copy link
Contributor

tbg commented Jul 13, 2017

Nice, thanks @irfansharif!

@prashantv
Copy link
Contributor

prashantv left a comment

Thank you for the contribution!

Can you please add a unit test to parser_test.go as well? I'd like to keep the raw pprof profiles as full integration tests, while individually unit testing each function as well. Thanks!

// Do not error when there is only id and address in the line.
if len(parts) != 2 {
p.setError(fmt.Errorf("malformed location line: %v", line))
if len(parts) == 2 || len(parts) == 3 && strings.HasPrefix(parts[2], "M=") {

This comment has been minimized.

@prashantv

prashantv Jul 14, 2017

Contributor

Can we clean this up into separate cases:

if len(parts) < 4 {
  switch {
  case len(parts) == 2:
    // some lines just have an ID and address, we can ignore those
    return
  case len(parts) == 3 && strings.HasPrefix(...):
    // [...]
    return
  }
  // malformed location line error
}

This comment has been minimized.

@irfansharif

irfansharif Jul 15, 2017

Contributor

sure, done.

pprof: fix incorrect parsing for locations
Fixes #61.

Extrapolating from
https://github.com/google/pprof/blob/master/profile/profile.go#L360 and
https://github.com/uber/go-torch/issues/61 we can have locations of the
following format:

    <id>: 0x<address> M=<mapping id>

Note: This doesn't address all malformed inputs, malformed inputs that
may have gotten through before will still do so. This patch only
addresses #61.

Added the raw pprof out as an additional test for one of our binaries
that failed prior.

pprof: add unit test for location parsing

@irfansharif irfansharif force-pushed the irfansharif:fix-61 branch from 77a54e5 to 6c7051e Jul 15, 2017

@irfansharif

This comment has been minimized.

Copy link
Contributor

irfansharif commented Jul 15, 2017

Can you please add a unit test to parser_test.go as well?

done.

FYI TestParseSampleCountMismatch doesn't do what you think. The following patch passes:

 func TestParseSampleCountMismatch(t *testing.T) {
        contents := `Samples:
        samples/count cpu/nanoseconds alloc_objects/count
-          2   10000000: 1
+          100   10000000: 1
        Locations:
           1: 0xaaaaa funcName :0 s=0
 `
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 15, 2017

Coverage Status

Coverage increased (+0.01%) to 95.443% when pulling 6c7051e on irfansharif:fix-61 into 55a555d on uber:master.

@prashantv
Copy link
Contributor

prashantv left a comment

Thanks again @irfansharif

I'll look at the TestParseSampleCountMismatch issue separately.

@prashantv prashantv merged commit 31163c5 into uber:master Jul 15, 2017

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 95.443%
Details
license/cla Contributor License Agreement is signed.
Details
@prashantv

This comment has been minimized.

Copy link
Contributor

prashantv commented Jul 15, 2017

Just looked at TestParseSampleCountMismatch -- it's not checking the actual number of samples, but the count of values vs headers for the sample. E.g., we have samples/count cpu/nanoseconds and allocobjects/count, but we're only specifying 2 sample values, 2 and 10000000. If you change 2 10000000: 1 to 2 10000000 3: 1, the test fails as expected.

So that test seems fine, thanks for checking though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment