Added an interactive web interface #154

ghemawat · 2017-07-10T06:43:30Z

The web interface is triggered by passing -http=port on the command line.
The interface is available by visiting localhost:port in a browser.

Requirements:

Graphviz must be installed.
Browser must support Javascript.
Tested in recent stable versions of chrome and firefox.

Features:

The entry point is a dot graph display (equivalent to "web" output).
Nodes in the graph can be selected by clicking.
A regular expression can also be typed in for selection.
The current selection (either list of nodes or a regexp)
can be focused, ignored, shown, or hidden.
Source code or disassembly of the current selection can be displayed.

on the command line. The interface is available by visiting localhost:port in a browser. Requirements: * Graphviz must be installed. * Browser must support Javascript. * Tested in recent stable versions of chrome and firefox. Features: * The entry point is a dot graph display (equivalent to "web" output). * Nodes in the graph can be selected by clicking. * A regular expression can also be typed in for selection. * The current selection (either list of nodes or a regexp) can be focused, ignored, or hidden. * Source code or disassembly of the current selection can be displayed.

codecov-io · 2017-07-10T20:08:09Z

Codecov Report

Merging #154 into master will decrease coverage by 1.02%.
The diff coverage is 14.45%.

@@            Coverage Diff            @@
##           master    #154      +/-   ##
=========================================
- Coverage   54.12%   53.1%   -1.03%     
=========================================
  Files          30      32       +2     
  Lines        5916    6096     +180     
=========================================
+ Hits         3202    3237      +35     
- Misses       2372    2508     +136     
- Partials      342     351       +9

Impacted Files	Coverage Δ
internal/driver/commands.go	`44.5% <0%> (ø)`	⬆️
internal/driver/webui.go	`10.41% <10.41%> (ø)`
internal/report/report.go	`23.44% <100%> (+1.19%)`	⬆️
internal/driver/driver.go	`69.27% <33.33%> (-0.01%)`	⬇️
internal/graph/dotgraph.go	`90.11% <33.33%> (-0.8%)`	⬇️
internal/driver/cli.go	`75.75% <50%> (-0.81%)`	⬇️
profile/legacy_java_profile.go	`73.83% <0%> (-0.87%)`	⬇️
fuzz/main.go	`0% <0%> (ø)`
... and 1 more

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 4dbd915...a5f0d1f. Read the comment docs.

usage message.

the page corresponding to command line arguments.

aalexand · 2017-07-12T05:15:32Z

internal/driver/cli.go

@@ -58,6 +59,7 @@ func parseFlags(o *plugin.Options) (*source, []string, error) {
 	flagTools := flag.String("tools", os.Getenv("PPROF_TOOLS"), "Path for object tool pathnames")

 	flagTimeout := flag.Int("timeout", -1, "Timeout in seconds for fetching a profile")
+	flagHTTPPort := flag.Int("http", 0, "Present interactive web based UI at the specified http port")


Would it be possible to make the "http" another command here -

pprof/internal/driver/commands.go

Line 87 in 1b8137e

var pprofCommands = commands{

?

I felt this would integrate more naturally, but maybe I am missing something.

That's what I had tried initially, but it became awkward for a few reasons:

The usage message for the new command showed up under Output formats which seemed odd.

Commands only support string parameters, whereas I wanted a port number.

Making it a command would allow http _port_ as a command in the interactive shell, which I wasn't sure we wanted to support.

So possible, but not sure it would be better/simpler than the current approach.

I see, makes sense to me.

I'm OK either way, but I think having http be listed under Output Formats wouldn't be terrible: we have (web|evince|gv) in there already. We could always explicitly disable the command in the UI if we really wanted to (is there really a problem with it?).

In fact, how about calling this "web" instead of "http", and eliminating the current "web"?
There's always "svg" for people that really want a static svg.

I think reusing the name "web" will be problematic since it will change the behavior substantially without warning. E.g., consider somebody who has a script that does "pprof web" a few times to open a bunch of views in a row. The interactive UI will block on the first call. I also am not so excited about "There's always "svg" for people that really want a static svg." If we do that, we are going to force some people to learn a new behavior after they get confused a bit about why things don't work as they used to; and I don't think the win here is big enough to justify that.

W.r.t. putting "http" under "Output Formats", I still think that it is better to keep things as they are for the reasons mentioned above (usage message, port number not a string, interactive command misuse).

So unless I hear a stronger complaint than "I'm OK either way", I will keep "http" out of Output Formats.

aalexand · 2017-07-12T17:19:51Z

README.md

+## Run pprof via a web interface
+
+If the `-http=port` option is specified, pprof starts a web server at
+the specified port that you can visit in your browser to get an interactive


Maybe mention here that the browser will auto-open.

aalexand · 2017-07-12T17:21:34Z

README.md

+```
+pprof -http=[port] [main_binary] profile.pb.gz
+
+This will run a server that can be visited in the browser.


Nit: this line seems redundant given the above text.

aalexand · 2017-07-12T17:23:04Z

internal/driver/cli.go

@@ -58,6 +59,7 @@ func parseFlags(o *plugin.Options) (*source, []string, error) {
 	flagTools := flag.String("tools", os.Getenv("PPROF_TOOLS"), "Path for object tool pathnames")

 	flagTimeout := flag.Int("timeout", -1, "Timeout in seconds for fetching a profile")
+	flagHTTPPort := flag.Int("http", 0, "Present interactive web based UI at the specified http port")


I see, makes sense to me.

aalexand · 2017-07-12T17:27:36Z

internal/driver/webui.go

+	})
+}
+
+// disasm generates a web page containing an svg diagram..


"disasm" -> "dot"
also fix double full stop at the end.

aalexand · 2017-07-12T17:38:38Z

internal/driver/webui.go

+	// Convert to svg.
+	svg, err := dotToSvg(dot.Bytes())
+	if err != nil {
+		reportHandlerError(w, "Failed to execute dot. Is Graphviz installed?", err)


Maybe change to "failed to execute dot: is Graphviz installed?" to start with lowercase like in other messages.

aalexand · 2017-07-12T17:43:00Z

internal/driver/webui.go

+
+// getFromLegend returns the suffix of an entry in legend that starts
+// with param.  It returns def if no such entry is found.
+func getFromLegend(legend []string, param, def string) string {


Extracting the data from the legend seems a bit fragile. Maybe make pprof fail early in tests if the parameter to extract is not found? Or something else that the tests would detect.

I augmented webui_test.go to check that the binary name and profile type are in the generated output.

I didn't do more than that because I was thinking that it would be best to eventually change lower-level code to produce legend info in a struct and leave the stringification to a helper routine. In this code we would avoid the stringification altogether and just get the pieces we need directly from the struct. E.g., I think the header we generate here could be augmented to include info about the displayed counts vs. the overall count.

rauls5382 · 2017-07-12T18:30:16Z

internal/driver/webhtml.go

+<button id="disasm">Disasm</button>
+<input id="searchbox" type="text" placeholder="Search regexp" autocomplete="off" autocapitalize="none" size=40/>
+<button id="focus">Focus</button>
+<button id="show">Show</button>


In other UIs we've ordered these as focus, ignore, hide, show, to make it clearer that focus/ignore and hide/show are related pairs.

rauls5382 · 2017-07-12T18:32:20Z

internal/driver/webui.go

+		}
+	}
+	// No visualizer succeeded, so just print URL.
+	fmt.Fprintln(os.Stderr, "Visit", u.String(), "in a web browser")


Use UI.PrintErr (from plugin.Options). Same for other prints to stderr.
Also, having the URL alone in a single line is easier for cut/paste.

rauls5382 · 2017-07-12T18:36:58Z

internal/driver/webui.go

+	vars["show"].value = req.URL.Query().Get("s")
+	vars["ignore"].value = req.URL.Query().Get("i")
+	vars["hide"].value = req.URL.Query().Get("h")
+	_, rpt, err := generateRawReport(ui.prof, args, vars, ui.options)


Why manually generate the dot and svg instead of using the existing support for pprof -svg? Seems to me that pushing the report.Generate from generateReport into generateRawReport would eliminate some duplication.

For instance, there is some duplication between dotToSvg() and invokeDot()

-svg (and -web etc.) generate a standalone svg document, whereas what we want here is an svg that we can embed inside an html.

The regular svg handling code calls Massage, which strips out useful information like width, height, viewBox. We want to avoid that information loss since we need it when the svg is embedded in html.

rauls5382 · 2017-07-12T23:33:29Z

internal/driver/webui.go

+// webUI holds the state needed for serving a browser based interface.
+type webUI struct {
+	prof    *profile.Profile
+	options *plugin.Options


The motivation behind the UI field in plugin.Options is that it would allow errors to be redirected to an interactive UI instead of always printing to stderr.

Would it be possible to redefine options.UI so that it would show such errors somewhere in the UI (either a pop-up or maybe in red in a corner)?

For instance, if you type a function name that doesn't exist and click "focus" we get a print on stderr saying "Focus expression matched no samples". It would be better if that showed up on the browser.

Done. Instead of getting fancy with the display, I just print the errors below the title, which is prominent enough. It is non-dismissable, which I think is fine, and perhaps better if somebody takes a screenshot.

1. Capture and display errors like "Focus expression matched no samples". 2. Re-ordered buttons to match other interfaces. 3. Use UI.PrintErr to print error messages.

ghemawat · 2017-07-13T03:02:37Z

internal/driver/webhtml.go

+<button id="disasm">Disasm</button>
+<input id="searchbox" type="text" placeholder="Search regexp" autocomplete="off" autocapitalize="none" size=40/>
+<button id="focus">Focus</button>
+<button id="show">Show</button>


ghemawat · 2017-07-13T03:32:06Z

internal/driver/webui.go

+// webUI holds the state needed for serving a browser based interface.
+type webUI struct {
+	prof    *profile.Profile
+	options *plugin.Options


Done. Instead of getting fancy with the display, I just print the errors below the title, which is prominent enough. It is non-dismissable, which I think is fine, and perhaps better if somebody takes a screenshot.

ghemawat · 2017-07-13T03:35:35Z

internal/driver/webui.go

+		}
+	}
+	// No visualizer succeeded, so just print URL.
+	fmt.Fprintln(os.Stderr, "Visit", u.String(), "in a web browser")


ghemawat · 2017-07-13T03:40:09Z

internal/driver/webui.go

+	vars["show"].value = req.URL.Query().Get("s")
+	vars["ignore"].value = req.URL.Query().Get("i")
+	vars["hide"].value = req.URL.Query().Get("h")
+	_, rpt, err := generateRawReport(ui.prof, args, vars, ui.options)


-svg (and -web etc.) generate a standalone svg document, whereas what we want here is an svg that we can embed inside an html.

The regular svg handling code calls Massage, which strips out useful information like width, height, viewBox. We want to avoid that information loss since we need it when the svg is embedded in html.

ghemawat · 2017-07-13T03:50:16Z

internal/driver/webui.go

+	// Convert to svg.
+	svg, err := dotToSvg(dot.Bytes())
+	if err != nil {
+		reportHandlerError(w, "failed to execute dot. Is Graphviz installed?", err)


Actually, I switched this back to "Failed..." so it matches commands.go. Also, this messag is printed out as a sentence on its own so it looks better capitalized.

josef-jelinek

HTML/JS

josef-jelinek · 2017-07-13T06:06:45Z

internal/driver/webhtml.go

+    var enable = (currentRe != "" || selected.size != 0)
+    if (buttonsEnabled == enable) return
+    buttonsEnabled = enable
+    var d = enable ? false : true


Why not equivalent "!enable" instead

Remnant of older code. Fixed to use !enable instead.

josef-jelinek · 2017-07-13T06:16:39Z

internal/driver/webhtml.go

+  var graph0 = document.getElementById("graph0")
+  var svg = graph0.parentElement
+
+  var currentRe = ""


FYI, Map is an ES6 feature. Relying on it means that the entire code can utilize ES6 (const, let, lambdas, for-of, destructuring, etc.). The same goes for the const at the top, also ES6 only.

Switched to const where possible, let elsewhere.
Switched to "for..of" where possible.
I didn't spot any place where lambdas or destructuring assingment would make things easier to read.

josef-jelinek · 2017-07-13T06:19:30Z

internal/driver/webhtml.go

+  var panLastX = 0      // Last event X coordinate
+  var panLastY = 0      // Last event Y coordinate
+  var moved = false     // Have we seen significant movement
+  var touchid = null;   // Current touch identifier


Although the code tries to avoid semicolons, there are still some left, which are not necessary (the only necessary semicolon is when the next line starts with "(" or "[" AFAIR).

Removed semicolons.

josef-jelinek · 2017-07-13T06:27:11Z

internal/driver/webhtml.go

+    searchAlarm = setTimeout(doSearch, 300)
+  }
+
+  doSearch = function() {


missing "var" (I recommend using "use strict" directive in global functions, which catch this). The same e.g. for the select and unselect functions below.

Added 'use strict'; very helpful. I put it just inside my two global functions instead of the top-level, since I presume you had some reason in mind when you mentioned global functions,

josef-jelinek · 2017-07-13T06:35:22Z

internal/driver/webhtml.go

+
+// Make svg pannable and zoomable.
+// Call clickHandler(t) if a click event is caught by the pan event handlers.
+function initPanAndZoom(svg, clickHandler) {


function statements are used here for global functions, but assignment of function expressions are used for the inner functions, is there any reason?
Unless reassigning a variable with another function, the statements have the advantage of being order independent and can be called before they are defined in the source, so the code can be top-down. The assignments have to be in a correct order and defined before used (unless used in another function and called later), which enforced bottom-up order).

I didn't realize functions could be nested. Fixed.
I could clean up the order of some things now, but I would rather not disturb the diff for very little gain.

josef-jelinek · 2017-07-13T06:40:28Z

internal/driver/webhtml.go

+    var dx = t1.clientX - t2.clientX
+    var dy = t1.clientY - t2.clientY
+    return Math.sqrt(Math.pow(dx, 2) + Math.pow(dy, 2))
+  }


There is Math.hypot() (ES6+)

Switched to Math.hypot.

josef-jelinek · 2017-07-13T06:50:54Z

internal/driver/webhtml.go

+var graphTemplate = template.Must(template.New("graph").Parse(
+	`<!DOCTYPE html>
+<html>
+<head>


nit: trailing "/" in tags is a HTML error (that is being gracefully ignored, it was used only for now dead XHTML). Only pair tags have to be paired.

Removed trailing "/" from tags.

josef-jelinek · 2017-07-13T06:56:17Z

internal/driver/webhtml.go

+</div>
+</body>
+</html>
+<script>


script should be before closing body (browsers generally ignore closing body and html and insert then at the end anyway, but AFAIK IE 10 for example did not and could ignore the script.

Moved <script> inside

Also added pprof binary to .gitignore.

ghemawat

Thanks. Your comments have been very helpful since I am a Javascript novice.

ghemawat · 2017-07-13T16:47:07Z

internal/driver/webhtml.go

+var graphTemplate = template.Must(template.New("graph").Parse(
+	`<!DOCTYPE html>
+<html>
+<head>


Removed trailing "/" from tags.

ghemawat · 2017-07-13T16:48:04Z

internal/driver/webhtml.go

+</div>
+</body>
+</html>
+<script>


Moved <script> inside

ghemawat · 2017-07-13T16:56:23Z

internal/driver/webhtml.go

+
+// Make svg pannable and zoomable.
+// Call clickHandler(t) if a click event is caught by the pan event handlers.
+function initPanAndZoom(svg, clickHandler) {


I didn't realize functions could be nested. Fixed.
I could clean up the order of some things now, but I would rather not disturb the diff for very little gain.

ghemawat · 2017-07-13T16:57:11Z

internal/driver/webhtml.go

+  var panLastX = 0      // Last event X coordinate
+  var panLastY = 0      // Last event Y coordinate
+  var moved = false     // Have we seen significant movement
+  var touchid = null;   // Current touch identifier


Removed semicolons.

ghemawat · 2017-07-13T17:00:43Z

internal/driver/webhtml.go

+    var dx = t1.clientX - t2.clientX
+    var dy = t1.clientY - t2.clientY
+    return Math.sqrt(Math.pow(dx, 2) + Math.pow(dy, 2))
+  }


Switched to Math.hypot.

ghemawat · 2017-07-13T17:24:46Z

internal/driver/webhtml.go

+  var graph0 = document.getElementById("graph0")
+  var svg = graph0.parentElement
+
+  var currentRe = ""


Switched to const where possible, let elsewhere.
Switched to "for..of" where possible.
I didn't spot any place where lambdas or destructuring assingment would make things easier to read.

ghemawat · 2017-07-13T17:26:27Z

internal/driver/webhtml.go

+    searchAlarm = setTimeout(doSearch, 300)
+  }
+
+  doSearch = function() {


Added 'use strict'; very helpful. I put it just inside my two global functions instead of the top-level, since I presume you had some reason in mind when you mentioned global functions,

ghemawat · 2017-07-13T17:29:10Z

internal/driver/webhtml.go

+    var enable = (currentRe != "" || selected.size != 0)
+    if (buttonsEnabled == enable) return
+    buttonsEnabled = enable
+    var d = enable ? false : true


Remnant of older code. Fixed to use !enable instead.

rauls5382

LGTM. I think we could factor out the common parts of the SVG generation to avoid some duplication, but we can do that separately in the future.

This reverts commit f83a3d8.

* Added an interactive web interface triggered by passing -http=port on the command line. The interface is available by visiting localhost:port in a browser. Requirements: * Graphviz must be installed. * Browser must support Javascript. * Tested in recent stable versions of chrome and firefox. Features: * The entry point is a dot graph display (equivalent to "web" output). * Nodes in the graph can be selected by clicking. * A regular expression can also be typed in for selection. * The current selection (either list of nodes or a regexp) can be focused, ignored, or hidden. * Source code or disassembly of the current selection can be displayed. * Remove unused function. * Skip graph generation test if graphviz is not installed. * Added -http port and the various modes of using pprof to the usage message. * Web interface now supports "show" option. * Web interface automatically opens the browser pointed at the page corresponding to command line arguments. * Some tweaks for firefox. * Handle review comments (better usage message, more testing). * Handled review comments: 1. Capture and display errors like "Focus expression matched no samples". 2. Re-ordered buttons to match other interfaces. 3. Use UI.PrintErr to print error messages. * Handle javascript code review comments (a bunch of cleanups). Also added pprof binary to .gitignore.

googlebot added the cla: yes label Jul 10, 2017

ghemawat added 2 commits July 9, 2017 23:48

Remove unused function.

eaf747a

Skip graph generation test if graphviz is not installed.

6f8ba57

ghemawat added 4 commits July 10, 2017 18:53

Added -http port and the various modes of using pprof to the

a7128c2

usage message.

Web interface now supports "show" option.

1efa645

Web interface automatically opens the browser pointed at

6bf26d0

the page corresponding to command line arguments.

Some tweaks for firefox.

0c58082

aalexand mentioned this pull request Jul 11, 2017

Update legend to show show/hide/focus/ignore settings #151

Merged

aalexand requested a review from rauls5382 July 12, 2017 05:04

aalexand reviewed Jul 12, 2017

View reviewed changes

Handle review comments (better usage message, more testing).

f57df72

rauls5382 reviewed Jul 12, 2017

View reviewed changes

Handled review comments:

c684429

1. Capture and display errors like "Focus expression matched no samples". 2. Re-ordered buttons to match other interfaces. 3. Use UI.PrintErr to print error messages.

ghemawat commented Jul 13, 2017

View reviewed changes

josef-jelinek reviewed Jul 13, 2017

View reviewed changes

Handle javascript code review comments (a bunch of cleanups).

a5f0d1f

Also added pprof binary to .gitignore.

ghemawat commented Jul 13, 2017

View reviewed changes

rauls5382 approved these changes Jul 14, 2017

View reviewed changes

aalexand merged commit f83a3d8 into google:master Jul 14, 2017

nolanmar511 added a commit to nolanmar511/pprof that referenced this pull request Jul 20, 2017

Revert "Added an interactive web interface (google#154)"

af3ad34

This reverts commit f83a3d8.

ghemawat deleted the webui branch September 5, 2017 15:17

Added an interactive web interface #154

Added an interactive web interface #154

Conversation

ghemawat commented Jul 10, 2017 • edited

codecov-io commented Jul 10, 2017 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghemawat Jul 13, 2017 • edited

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

ghemawat Jul 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josef-jelinek 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

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

ghemawat 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

rauls5382 left a comment

Choose a reason for hiding this comment

ghemawat commented Jul 10, 2017 •

edited

codecov-io commented Jul 10, 2017 •

edited

ghemawat Jul 13, 2017 •

edited

ghemawat Jul 13, 2017 •

edited