Add show_from profile filter. #372

lvng · 2018-05-03T17:25:05Z

This change adds the show_from filter, which drops frames before the first match in each stack, beginning from the root.

This fixes #363.

…is implemented as a separate function with it's own test.

…r raw and proto commands. updated the driver test case to match demangled function name. fixed code formatting.

codecov-io · 2018-05-03T21:48:16Z

Codecov Report

Merging #372 into master will increase coverage by 0.14%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #372      +/-   ##
==========================================
+ Coverage   66.73%   66.88%   +0.14%     
==========================================
  Files          36       36              
  Lines        7462     7495      +33     
==========================================
+ Hits         4980     5013      +33     
  Misses       2080     2080              
  Partials      402      402

Impacted Files	Coverage Δ
internal/driver/commands.go	`44.5% <ø> (ø)`	⬆️
internal/driver/driver.go	`70.43% <100%> (ø)`	⬆️
internal/driver/driver_focus.go	`85.24% <100%> (+0.37%)`	⬆️
profile/filter.go	`87.21% <100%> (+3.72%)`	⬆️

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 854c7e5...2e80e4e. Read the comment docs.

aalexand · 2018-05-04T00:28:15Z

internal/driver/commands.go

@@ -185,6 +185,10 @@ var pprofVariables = variables{
 		"Only show nodes matching regexp",
 		"If set, only show nodes that match this location.",
 		"Matching includes the function name, filename or object name.")},
+	"show_from": &variable{stringKind, "", "", helpText(
+		"Drops nodes above the highest matched node of every sample.",


I'd replace "node" with "frame" since the rest of the text uses that. Maybe try to make the description also a bit more like description of prune_from in style since these two are symmetric in a way.

aalexand · 2018-05-04T00:29:10Z

internal/driver/driver.go

@@ -193,6 +195,9 @@ func applyCommandOverrides(cmd []string, v variables) variables {
 		v.set("hide", "")
 		v.set("show", "")
 	}
+	if !showFrom {
+		v.set("show_from", "")


Why adding showFrom var and separate check and not just put this line after v.set("show", "")?

I wasn't sure whether to include it in the focus or hide section. it seems those flags are semantically equivalent so i combined focus, hide, and showFrom into a single flag "filter".

aalexand · 2018-05-04T00:30:53Z

internal/driver/driver_test.go

@@ -247,6 +248,7 @@ func testSourceURL(port int) string {

 // solutionFilename returns the name of the solution file for the test
 func solutionFilename(source string, f *testFlags) string {
+	fmt.Printf("solution flags: %v\n", f)


Debug leftover?

yup. fixed.

aalexand · 2018-05-04T00:31:59Z

profile/filter.go

@@ -74,6 +74,97 @@ func (p *Profile) FilterSamplesByName(focus, ignore, hide, show *regexp.Regexp)
 	return
 }

+// ShowFrom drops all stack frames before the first match from the root and


Nit: we say "first" here but "highest" in the documentation for the option. I'd try to be consistent in the terminology.

changed to "highest"

aalexand · 2018-05-04T00:37:16Z

profile/filter.go

+// returns whether a match was found. If showFrom is nil it returns false and
+// does not modify the profile.
+//
+// Example: consider a sample with frames [A, B, C], where A is the root.


It would be good to have example with multiple matches, otherwise you do not demonstrate the "highest" / "first" point.

Added a duplicate B frame to illustrate matching multiple frames.

aalexand · 2018-05-04T01:18:00Z

profile/filter.go

+	showFromLocs := make(map[uint64]bool)
+
+	// Apply to locations.
+	for _, location := range p.Location {


Nit: location -> loc like in other functions in this file, to be consistent.

aalexand · 2018-05-04T01:19:41Z

profile/filter_test.go

@@ -392,6 +414,113 @@ func TestFilter(t *testing.T) {
 	}
 }

+func TestShowFrom(t *testing.T) {
+	for _, tc := range []struct {
+		// name is the name of the test case.


This and next two comments on the fields are not informative, I'd drop them.

aalexand · 2018-05-04T01:21:20Z

profile/filter_test.go

+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			p := tc.profile.Copy()
+			gotMatch := p.ShowFrom(tc.showFrom)


Merge the assignment into the if statement on next line.

aalexand · 2018-05-04T01:21:39Z

profile/filter_test.go

+			gotMatch := p.ShowFrom(tc.showFrom)
+
+			if gotMatch != tc.wantMatch {
+				t.Errorf("match got %+v want %+v", gotMatch, tc.wantMatch)


comma before "want"

aalexand · 2018-05-04T01:23:32Z

profile/filter_test.go

@@ -392,6 +414,113 @@ func TestFilter(t *testing.T) {
 	}
 }

+func TestShowFrom(t *testing.T) {
+	for _, tc := range []struct {


Do you have test cases that check that "show from" shows frames from the highest match? It is not obvious from the test names that any test does that. Need a test for both locations and lines.

added two test cases. one for matching multiple frames without inlines, and one with inlines.

lvng · 2018-05-04T19:32:43Z

PTAL.

aalexand · 2018-05-04T19:36:03Z

internal/driver/driver.go

@@ -150,11 +150,11 @@ func generateReport(p *profile.Profile, cmd []string, vars variables, o *plugin.
 }

 func applyCommandOverrides(cmd []string, v variables) variables {
-	trim, focus, tagfocus, hide := v["trim"].boolValue(), true, true, true
+	trim, tagfocus, filter := v["trim"].boolValue(), true, true


Maybe rename "tagfocus" to "tagfilter", I think it will be clearer, too.

aalexand · 2018-05-04T19:38:25Z

profile/filter.go

+// filterShowFromLocation tests a showFrom regex against a location, removes
+// lines after the last match and returns whether a match was found. If the
+// mapping is matched, then all lines are kept.
+func filterShowFromLocation(loc *Location, showFrom *regexp.Regexp) (matched bool) {


I think named return parameter doesn't give here much. I'd just return a bool and then do "return true" and return false" as needed.

aalexand · 2018-05-04T19:39:24Z

profile/filter.go

+	}
+	if i := loc.lastMatchedLineIndex(showFrom); i >= 0 {
+		matched = true
+		loc.Line = loc.Line[0 : i+1]


Nit: specifying zero is not required: loc.Line[:i+1]

aalexand · 2018-05-07T19:23:05Z

@lvng Could you also include the new flag in the docs in https://github.com/google/pprof/tree/master/doc#options?

foo

aalexand · 2018-05-07T21:51:01Z

doc/README.md

@@ -94,6 +94,8 @@ Some common pprof options are:
  *regex*.
 * **-ignore= _regex_:** Do not include samples that include a report entry matching
  *regex*.
+* **-show\_from= _regex_:** Do not show entries above the first frame that 


Maybe say "above the first one" as it doesn't look like the text here uses term "frame".

* 'master' of github.com:google/pprof: (32 commits) record diff base profile label key/value constant (google#384) apply additional command overrides based on report format (google#381) profile: fix legacy format Go heap profile parsing (google#382) add -diff flag for better profile comparision (google#369) Use -u flag in pprof installation command so that it updates if needed. (google#376) Add GetBase support for ASLR kernel mappings (google#371) Add show_from profile filter. (google#372) Update README.md due to 8dff45d (google#375) Remove stale docs, move useful ones. (google#374) internal/driver: skip tests requiring tcp on js (google#373) Add "trim path" option which can be used to relocate sources. (google#366) Move update_d3flamegraph.sh script from the root directory. (google#370) Skip unsymbolizable mapping during symbolz pass. (google#368) remove -positive_percentages flag (google#365) Render icicle graph in "Flame Graph" view (google#367) Add command-line editing support for interactive pprof (google#362) Improve profile filter unit tests, fix show filtering of mappings (google#354) Fake mappings should be merged into a single one during merging (google#357) Hack the code so that both existing Go versions and current Go master format it the same (google#358) moved filter tests into their own test file which matches the implementation file, filter.go. (google#356) ...

lvng added 2 commits May 2, 2018 13:44

moved the showFrom filter from branch filter2 to branch filters3. It …

0776b46

…is implemented as a separate function with it's own test.

added show_from command line flag and driver test.

331aae2

googlebot added the cla: yes label May 3, 2018

aalexand mentioned this pull request May 3, 2018

Add ShowFrom and HideFrom filters to profile filter lib #361

Closed

added show_from handling to command overrides to avoid applying it fo…

75fee66

…r raw and proto commands. updated the driver test case to match demangled function name. fixed code formatting.

aalexand reviewed May 4, 2018

View reviewed changes

review feedback round 1.

30bb8e2

Merge branch 'master' into filters3

35a7878

aalexand reviewed May 4, 2018

View reviewed changes

lvng added 2 commits May 4, 2018 15:32

review feedback round 2.

5e56ca2

Merge branch 'filters3' of github.com:lvng/pprof into filters3

271c8f3

aalexand previously approved these changes May 4, 2018

View reviewed changes

Merge branch 'master' into filters3

df73391

lvng added 2 commits May 7, 2018 14:42

Add show_from to pprof doc.

7af823d

Merge branch 'filters3' of github.com:lvng/pprof into filters3

397e732

foo

lvng dismissed aalexand’s stale review via 397e732 May 7, 2018 21:44

aalexand reviewed May 7, 2018

View reviewed changes

change language of show_from doc.

2e80e4e

aalexand approved these changes May 8, 2018

View reviewed changes

aalexand merged commit a50cffb into google:master May 8, 2018

gmarin13 pushed a commit to gmarin13/pprof that referenced this pull request Dec 17, 2020

Add show_from profile filter. (google#372)

faf2502

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add show_from profile filter. #372

Add show_from profile filter. #372

lvng commented May 3, 2018 •

edited by aalexand

Loading

codecov-io commented May 3, 2018 •

edited

Loading

aalexand May 4, 2018

lvng May 4, 2018

aalexand May 4, 2018

lvng May 4, 2018

aalexand May 4, 2018

lvng May 4, 2018

aalexand May 4, 2018

lvng May 4, 2018

aalexand May 4, 2018

lvng May 4, 2018

aalexand May 4, 2018

lvng May 4, 2018

aalexand May 4, 2018

lvng May 4, 2018

aalexand May 4, 2018

lvng May 4, 2018

aalexand May 4, 2018

lvng May 4, 2018

aalexand May 4, 2018

lvng May 4, 2018

lvng commented May 4, 2018

aalexand May 4, 2018

lvng May 4, 2018

aalexand May 4, 2018

lvng May 4, 2018

aalexand May 4, 2018

lvng May 4, 2018

aalexand commented May 7, 2018

aalexand May 7, 2018

lvng May 7, 2018

Add show_from profile filter. #372

Add show_from profile filter. #372

Conversation

lvng commented May 3, 2018 • edited by aalexand Loading

codecov-io commented May 3, 2018 • edited Loading

Codecov Report

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lvng commented May 4, 2018

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 commented May 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lvng commented May 3, 2018 •

edited by aalexand

Loading

codecov-io commented May 3, 2018 •

edited

Loading