Add column numbers to profile.proto. #818

snehasish · 2023-11-09T19:45:58Z

This change adds column numbers to the line message in profile.proto. This allows users to distinguish between souce code locations on the same line. Only the llvm based addr2line is capable of reading column number information from dwarf and PE debug information. Other changes include:

Add "columns" option for output granularity.
Account for column numbers during profile merge.
Update the encoder and decoder.
Update golden test files for legacy profiles.

Fixes #687

codecov-commenter · 2023-11-27T18:42:06Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (960ae82) 66.83% compared to head (8f8a0f9) 66.86%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #818      +/-   ##
==========================================
+ Coverage   66.83%   66.86%   +0.03%     
==========================================
  Files          44       44              
  Lines        9800     9824      +24     
==========================================
+ Hits         6550     6569      +19     
- Misses       2790     2794       +4     
- Partials      460      461       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

This change adds column numbers to the line message in profile.proto. This allows users to distinguish between souce code locations on the same line. Only the llvm based addr2line is capable of reading column number information from dwarf and PE debug information. Other changes include: * Add "columns" option for output granularity. * Account for column numbers during profile merge. * Update the encoder and decoder. * Update golden test files for legacy profiles. Fixes google#687

aalexand

Looks good for the most part, a couple questions.

aalexand · 2023-12-03T20:26:57Z

internal/driver/config.go

@@ -124,7 +125,7 @@ func init() {
 	// take on one of a bounded set of values.
 	choices := map[string][]string{
 		"sort":        {"cum", "flat"},
-		"granularity": {"functions", "filefunctions", "files", "lines", "addresses"},
+		"granularity": {"functions", "filefunctions", "files", "lines", "columns", "addresses"},


Would it make sense to instead make this an option like columns similar to noinlines? But I guess pprof -lines -columns would look weird.

I did make this an option in internal/driver/commands.go. Can you take a look?

We should remove it from the granularity list here then?

aalexand · 2023-12-03T20:32:02Z

profile/profile_test.go

@@ -997,7 +1075,7 @@ func locationHash(s *Sample) string {
 	var tb string
 	for _, l := range s.Location {
 		for _, ln := range l.Line {
-			tb = tb + fmt.Sprintf("%s:%d@%d ", ln.Function.Name, ln.Line, l.Address)
+			tb = tb + fmt.Sprintf("%s:%d:%d@%d ", ln.Function.Name, ln.Line, ln.Column, l.Address)


When columns are present, are they 1-based? If yes, maybe we can omit the ":0" part when the column is zero? This would make the pprof -raw output less chatty for the case when the column information is missing.

The downside is that if the column information is only partially present then the -raw output would partially have and partially haven't the column number which can be obscure to the user.

Yes, column numbers are 1-based when available (see https://dwarfstd.org/doc/DWARF5.pdf, Sec 6.2.2 pg 157). The value 0 is reserved to indicate that a statement begins at the “left edge” of the line, but implementations don't use this as far as I know.

IMO it's clearer to retain the value, an additional zero per line is not too chatty. Happy to reconsider if you feel strongly.

In the end I chose to suppress the zero column numbers while reporting to minimize updates to the tests.

I don't see it being suppressed, am I missing something?

This is the test-only locationHash helper. Here I think we need the extra resolution here. My comment about suppressing the column number is implemented in graph.go, see L178-181 which updated the NameComponents method.

s := fmt.Sprintf("%s:%d", i.File, i.Lineno) if i.Columnno != 0 { s += fmt.Sprintf(":%d", i.Columnno) }

aalexand

Could you check that pprof reports properly output the column when line granularity is used and the columns are enabled? I'm thinking scenarios like:

-top -lines -showcolumns
-svg -lines -showcolumns
-http $HOSTNAME:8080 -lines -showcolumns then checking all views.

aalexand · 2023-12-12T05:20:28Z

internal/driver/config.go

@@ -157,6 +158,7 @@ func init() {
 		"sort":                 "sort",
 		"granularity":          "g",
 		"noinlines":            "noinlines",
+		"columns":              "columns",


Maybe name it "showcolumns" so that -lines -showcolumns looks a bit better?

aalexand · 2023-12-12T05:21:09Z

internal/driver/driver.go

@@ -246,6 +246,11 @@ func aggregate(prof *profile.Profile, cfg config) error {
 		function = true
 		filename = true
 		linenumber = true
+	case "columns":


I think this shouldn't be in the granularity switch anymore and should be handled as option instead?

aalexand · 2023-12-12T05:22:19Z

internal/driver/config.go

@@ -124,7 +125,7 @@ func init() {
 	// take on one of a bounded set of values.
 	choices := map[string][]string{
 		"sort":        {"cum", "flat"},
-		"granularity": {"functions", "filefunctions", "files", "lines", "addresses"},
+		"granularity": {"functions", "filefunctions", "files", "lines", "columns", "addresses"},


We should remove it from the granularity list here then?

aalexand · 2023-12-12T05:22:46Z

internal/driver/config.go

@@ -51,6 +51,7 @@ type config struct {
 	TagShow      string  `json:"tagshow,omitempty"`
 	TagHide      string  `json:"taghide,omitempty"`
 	NoInlines    bool    `json:"noinlines,omitempty"`
+	Columns      bool    `json:"columns,omitempty"`


Maybe ShowColumns? See another comment.

snehasish · 2023-12-20T22:40:11Z

Could you check that pprof reports properly output the column when line granularity is used and the columns are enabled? I'm thinking scenarios like:

-top -lines -showcolumns

-svg -lines -showcolumns

-http $HOSTNAME:8080 -lines -showcolumns then checking all views.

Here is the output from top for a sample app (when non-zero column numbers are available via llvm-symbolizer):

$ go run pprof.go -top -lines -showcolumns sample profile.pb.gz

Type: cycles:u_event
Showing nodes accounting for 12157325838, 99.93% of 12165793058 total
Dropped 13 nodes (cum <= 60828965)
      flat  flat%   sum%        cum   cum%
2413918557 19.84% 19.84% 2413918557 19.84%  main /tmp/sample.c:33:20
2289792795 18.82% 38.66% 2289792795 18.82%  main /tmp/sample.c:33:9
2051102236 16.86% 55.52% 2051102236 16.86%  compute_flag /tmp/sample.c:13:9
1185046082  9.74% 65.26% 1185046082  9.74%  main /tmp/sample.c:34:15

Also added a couple of screenshots which show column numbers in the web views.

snehasish · 2024-01-08T16:54:29Z

Post-holiday ping :)

aalexand · 2024-01-12T20:39:24Z

internal/symbolizer/symbolizer_test.go

@@ -242,23 +242,27 @@ func checkSymbolizedLocation(a uint64, got []profile.Line) error {
 		if g.Line != int64(w.Line) {
 			return fmt.Errorf("want lineno: %d, got %d", w.Line, g.Line)


nit: "line" here or "columnno" below?

aalexand · 2024-01-12T20:41:20Z

profile/profile_test.go

@@ -997,7 +1075,7 @@ func locationHash(s *Sample) string {
 	var tb string
 	for _, l := range s.Location {
 		for _, ln := range l.Line {
-			tb = tb + fmt.Sprintf("%s:%d@%d ", ln.Function.Name, ln.Line, l.Address)
+			tb = tb + fmt.Sprintf("%s:%d:%d@%d ", ln.Function.Name, ln.Line, ln.Column, l.Address)


I don't see it being suppressed, am I missing something?

P403n1x87 · 2024-01-17T16:11:09Z

CPython >= 3.11 allows extracting line end and column end information. Would it be conceivable to have these extra fields to also carry this information in pprof files?

aalexand · 2024-01-17T17:25:55Z

Unlikely. We are hesitant to add fields that are too niche. Even adding the column number was controversial enough. I don't anticipate adding more fields like that.

This was referenced Nov 12, 2023

Proposal: add Discriminator field to Line message #768

Closed

Proposal: Add Column field to Line message #687

Closed

snehasish force-pushed the column branch from 0b0dfe1 to 3d8bf76 Compare November 16, 2023 00:13

snehasish force-pushed the column branch from 3d8bf76 to 66a113c Compare November 28, 2023 22:05

snehasish force-pushed the column branch from 66a113c to 0531560 Compare November 29, 2023 17:09

aalexand reviewed Dec 3, 2023

View reviewed changes

aalexand and others added 3 commits December 3, 2023 12:34

Merge branch 'main' into column

10a1914

Merge branch 'main' into column

a675f89

Update the expecatations in the test to match testdata.

2120b2e

aalexand reviewed Dec 12, 2023

View reviewed changes

Update report generation and driver for column numbers.

073c7cb

Merge branch 'main' into column

b8fe7fd

aalexand reviewed Jan 12, 2024

View reviewed changes

Address comments

8f8a0f9

aalexand approved these changes Jan 17, 2024

View reviewed changes

aalexand merged commit 35fc243 into google:main Jan 17, 2024
31 checks passed

petethepig added a commit to petethepig/opentelemetry-proto that referenced this pull request Jan 23, 2024

ports changes from original pprof: google/pprof#818

7ec7a8d

aalexand mentioned this pull request Feb 12, 2024

Introduces Profiling Data Model v2 open-telemetry/oteps#239

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add column numbers to profile.proto. #818

Add column numbers to profile.proto. #818

snehasish commented Nov 9, 2023 •

edited

codecov-commenter commented Nov 27, 2023 •

edited

aalexand left a comment

aalexand Dec 3, 2023

snehasish Dec 5, 2023

aalexand Dec 12, 2023

aalexand Dec 3, 2023

snehasish Dec 5, 2023

snehasish Dec 20, 2023

aalexand Jan 12, 2024

snehasish Jan 16, 2024

aalexand left a comment

aalexand Dec 12, 2023

snehasish Dec 20, 2023

aalexand Dec 12, 2023

snehasish Dec 20, 2023

aalexand Dec 12, 2023

aalexand Dec 12, 2023

snehasish Dec 20, 2023

snehasish commented Dec 20, 2023

snehasish commented Jan 8, 2024

aalexand Jan 12, 2024

snehasish Jan 16, 2024

aalexand Jan 12, 2024

P403n1x87 commented Jan 17, 2024

aalexand commented Jan 17, 2024

		@@ -242,23 +242,27 @@ func checkSymbolizedLocation(a uint64, got []profile.Line) error {
		if g.Line != int64(w.Line) {
		return fmt.Errorf("want lineno: %d, got %d", w.Line, g.Line)

Add column numbers to profile.proto. #818

Add column numbers to profile.proto. #818

Conversation

snehasish commented Nov 9, 2023 • edited

codecov-commenter commented Nov 27, 2023 • edited

Codecov Report

aalexand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aalexand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snehasish commented Dec 20, 2023

snehasish commented Jan 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

P403n1x87 commented Jan 17, 2024

aalexand commented Jan 17, 2024

snehasish commented Nov 9, 2023 •

edited

codecov-commenter commented Nov 27, 2023 •

edited