Skip to content

internal/graph: Escape tag labels correctly for dot #683

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.

Sign up for GitHub

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

Closed
evanj wants to merge 2 commits into from
Closed

internal/graph: Escape tag labels correctly for dot #683

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ded1d62
internal/graph: Escape tag labels correctly for dot
evanj Feb 17, 2022
b63906b
Change escapeForDot to escape everything on to one line.
evanj Feb 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions internal/driver/testdata/pprof.cpu.flat.functions.call_tree.dot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@ digraph "testbinary" {
node [style=filled fillcolor="#f8f8f8"]
subgraph cluster_L { "File: testbinary" [shape=box fontsize=16 label="File: testbinary\lType: cpu\lDuration: 10s, Total samples = 1.12s (11.20%)\lShowing nodes accounting for 1.11s, 99.11% of 1.12s total\lDropped 3 nodes (cum <= 0.06s)\l\lSee https://git.io/JfYMW for how to read the graph\l" tooltip="testbinary"] }
N1 [label="line1000\n1s (89.29%)" id="node1" fontsize=24 shape=box tooltip="line1000 (1s)" color="#b20500" fillcolor="#edd6d5"]
N1_0 [label = "key1:tag1\nkey2:tag1" id="N1_0" fontsize=8 shape=box3d tooltip="1s"]
N1_0 [label = "key1:tag1\lkey2:tag1" id="N1_0" fontsize=8 shape=box3d tooltip="1s"]
Copy link
Copy Markdown
Collaborator

@aalexand aalexand Feb 18, 2022

Choose a reason for hiding this comment

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

I think I would expect the current behavior to be preserved - centered tags.

I'd also expect any non-printable characters like newlines to be rendered as their C escaped version, so that the tag value is always one line when rendered. Same for function names. The escaping was initially added in #557 for graph legend labels that come from the profile comments field and for that we did support newlines to render newlines from comments as newlines since some comments intentionally use that kind of formatting. I think for function names and tag values it should be different and we shouldn't allow any formatting sequences in those strings.

Copy link
Copy Markdown
Contributor Author

@evanj evanj Feb 18, 2022

Choose a reason for hiding this comment

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

This may be a significant change to how escapeForDot works. Previously, if an input contains newlines, it returns an output that will be displayed on separate lines on the dot graph. However, it could be that this is a better definition: it will now return an escaped string that will be displayed on a single

The challenge is now:

  • We have single key:value pairs that we want to escape so they display on a single line. To do this, we call escapeForDot on them.
  • We have a set of key:value pairs, that we want to display on separate lines, so we need to include "\n" in the dot output. This has to be done after escaping.
  • The set of key:value pairs are used as map keys, so we can't easily use []string for labels instead of string.

My hack was to change the definition of joinLabels to return a string escaped for dot. I think the final output is actually a good improvement, but I wish the escaping was only applied at the very end.

N1 -> N1_0 [label=" 1s" weight=100 tooltip="1s" labeltooltip="1s"]
N2 [label="line3000\n0 of 1.12s (100%)" id="node2" fontsize=8 shape=box tooltip="line3000 (1.12s)" color="#b20000" fillcolor="#edd5d5"]
N3 [label="line3001\n0 of 1.11s (99.11%)" id="node3" fontsize=8 shape=box tooltip="line3001 (1.11s)" color="#b20000" fillcolor="#edd5d5"]
N4 [label="line1000\n0.10s (8.93%)" id="node4" fontsize=14 shape=box tooltip="line1000 (0.10s)" color="#b28b62" fillcolor="#ede8e2"]
N4_0 [label = "key1:tag2\nkey3:tag2" id="N4_0" fontsize=8 shape=box3d tooltip="0.10s"]
N4_0 [label = "key1:tag2\lkey3:tag2" id="N4_0" fontsize=8 shape=box3d tooltip="0.10s"]
N4 -> N4_0 [label=" 0.10s" weight=100 tooltip="0.10s" labeltooltip="0.10s"]
N5 [label="line3002\n0.01s (0.89%)\nof 1.01s (90.18%)" id="node5" fontsize=10 shape=box tooltip="line3002 (1.01s)" color="#b20500" fillcolor="#edd6d5"]
N6 [label="line2000\n0 of 1s (89.29%)" id="node6" fontsize=8 shape=box tooltip="line2000 (1s)" color="#b20500" fillcolor="#edd6d5"]
Expand Down
4 changes: 2 additions & 2 deletions internal/driver/testdata/pprof.cpu.flat.functions.dot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@ digraph "testbinary" {
node [style=filled fillcolor="#f8f8f8"]
subgraph cluster_L { "File: testbinary" [shape=box fontsize=16 label="File: testbinary\lType: cpu\lDuration: 10s, Total samples = 1.12s (11.20%)\lShowing nodes accounting for 1.12s, 100% of 1.12s total\l\lSee https://git.io/JfYMW for how to read the graph\l" tooltip="testbinary"] }
N1 [label="line1000\n1.10s (98.21%)" id="node1" fontsize=24 shape=box tooltip="line1000 (1.10s)" color="#b20000" fillcolor="#edd5d5"]
N1_0 [label = "key1:tag1\nkey2:tag1" id="N1_0" fontsize=8 shape=box3d tooltip="1s"]
N1_0 [label = "key1:tag1\lkey2:tag1" id="N1_0" fontsize=8 shape=box3d tooltip="1s"]
N1 -> N1_0 [label=" 1s" weight=100 tooltip="1s" labeltooltip="1s"]
N1_1 [label = "key1:tag2\nkey3:tag2" id="N1_1" fontsize=8 shape=box3d tooltip="0.10s"]
N1_1 [label = "key1:tag2\lkey3:tag2" id="N1_1" fontsize=8 shape=box3d tooltip="0.10s"]
N1 -> N1_1 [label=" 0.10s" weight=100 tooltip="0.10s" labeltooltip="0.10s"]
N2 [label="line3000\n0 of 1.12s (100%)" id="node2" fontsize=8 shape=box tooltip="line3000 (1.12s)" color="#b20000" fillcolor="#edd5d5"]
N3 [label="line3001\n0 of 1.11s (99.11%)" id="node3" fontsize=8 shape=box tooltip="line3001 (1.11s)" color="#b20000" fillcolor="#edd5d5"]
Expand Down
10 changes: 5 additions & 5 deletions internal/graph/dotgraph.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ func (b *builder) addNodelets(node *Node, nodeID int) bool {
continue
}
weight := b.config.FormatValue(w)
nodelets += fmt.Sprintf(`N%d_%d [label = "%s" id="N%d_%d" fontsize=8 shape=box3d tooltip="%s"]`+"\n", nodeID, i, t.Name, nodeID, i, weight)
nodelets += fmt.Sprintf(`N%d_%d [label = "%s" id="N%d_%d" fontsize=8 shape=box3d tooltip="%s"]`+"\n", nodeID, i, escapeForDot(t.Name), nodeID, i, weight)
nodelets += fmt.Sprintf(`N%d -> N%d_%d [label=" %s" weight=100 tooltip="%s" labeltooltip="%s"]`+"\n", nodeID, nodeID, i, weight, weight, weight)
if nts := lnts[t.Name]; nts != nil {
nodelets += b.numericNodelets(nts, maxNodelets, flatTags, fmt.Sprintf(`N%d_%d`, nodeID, i))
Expand All @@ -274,7 +274,7 @@ func (b *builder) numericNodelets(nts []*Tag, maxNumNodelets int, flatTags bool,
}
if w != 0 {
weight := b.config.FormatValue(w)
nodelets += fmt.Sprintf(`N%s_%d [label = "%s" id="N%s_%d" fontsize=8 shape=box3d tooltip="%s"]`+"\n", source, j, t.Name, source, j, weight)
nodelets += fmt.Sprintf(`N%s_%d [label = "%s" id="N%s_%d" fontsize=8 shape=box3d tooltip="%s"]`+"\n", source, j, escapeForDot(t.Name), source, j, weight)
nodelets += fmt.Sprintf(`%s -> N%s_%d [label=" %s" weight=100 tooltip="%s" labeltooltip="%s"%s]`+"\n", source, source, j, weight, weight, weight, attr)
}
}
Expand Down Expand Up @@ -483,9 +483,9 @@ func escapeAllForDot(in []string) []string {
return out
}

// escapeForDot escapes double quotes and backslashes, and replaces Graphviz's
// "center" character (\n) with a left-justified character.
// See https://graphviz.org/doc/info/attrs.html#k:escString for more info.
// escapeForDot escapes double quotes and backslashes, and replaces newlines
// with left-justified newlines ("\l"), instead of centered ("\n").
// See https://graphviz.org/docs/attr-types/escString/ for more info.
func escapeForDot(str string) string {
return strings.ReplaceAll(strings.ReplaceAll(strings.ReplaceAll(str, `\`, `\\`), `"`, `\"`), "\n", `\l`)
}
24 changes: 24 additions & 0 deletions internal/graph/dotgraph_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,30 @@ func TestComposeWithNamesThatNeedEscaping(t *testing.T) {
compareGraphs(t, buf.Bytes(), "compose7.dot")
}

func TestComposeWithTagsThatNeedEscaping(t *testing.T) {
g := baseGraph()
a, c := baseAttrsAndConfig()
// test both named and numeric tags
g.Nodes[0].LabelTags["a"] = &Tag{
Name: `label"quote"` + "\nline2",
Cum: 10,
Flat: 10,
}
g.Nodes[0].NumericTags[""] = TagMap{
"b": &Tag{
Name: `numeric"quote"`,
Cum: 20,
Flat: 20,
Unit: "ms",
},
}

var buf bytes.Buffer
ComposeDot(&buf, g, a, c)

compareGraphs(t, buf.Bytes(), "compose8.dot")
}

func baseGraph() *Graph {
src := &Node{
Info: NodeInfo{Name: "src"},
Expand Down
4 changes: 3 additions & 1 deletion internal/graph/graph.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -519,6 +519,7 @@ func (g *Graph) TrimTree(kept NodePtrSet) {
g.RemoveRedundantEdges()
}

// joinLabels returns the labels as they should be displayed to the user (not escaped).
func joinLabels(s *profile.Sample) string {
if len(s.Label) == 0 {
return ""
Expand All @@ -531,7 +532,8 @@ func joinLabels(s *profile.Sample) string {
}
}
sort.Strings(labels)
return strings.Join(labels, `\n`)
// join labels with a newline: this can be a bit confusing for labels with newlines
Copy link
Copy Markdown
Collaborator

@aalexand aalexand Feb 18, 2022

Choose a reason for hiding this comment

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

nit: please make comments full sentences (titlecase, trailing dot).

Also here and in other places please make comments fit 80 columns, like the rest of the pprof codebase.

Copy link
Copy Markdown
Contributor Author

@evanj evanj Feb 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Okay! I've edited this since it needs to change a bit to accommodate the escaping change you mentioned below.

return strings.Join(labels, "\n")
}

// isNegative returns true if the node is considered as "negative" for the
Expand Down
15 changes: 15 additions & 0 deletions internal/graph/graph_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -531,3 +531,18 @@ func TestShortenFunctionName(t *testing.T) {
}
}
}

func TestJoinLabels(t *testing.T) {
input := &profile.Sample{
Label: map[string][]string{
"key1": {"v1", "v2"},
// value with an embedded newline
"key2": {"value line1\nline2"},
},
}
const expected = "key1:v1\nkey1:v2\nkey2:value line1\nline2"
output := joinLabels(input)
if output != expected {
t.Errorf("output=%#v != expected=%#v", output, expected)
}
}
11 changes: 11 additions & 0 deletions internal/graph/testdata/compose8.dot
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
digraph "testtitle" {
node [style=filled fillcolor="#f8f8f8"]
subgraph cluster_L { "label1" [shape=box fontsize=16 label="label1\llabel2\llabel3: \"foo\"\l" tooltip="testtitle"] }
N1 [label="src\n10 (10.00%)\nof 25 (25.00%)" id="node1" fontsize=22 shape=box tooltip="src (25)" color="#b23c00" fillcolor="#edddd5"]
N1_0 [label = "label\"quote\"\lline2" id="N1_0" fontsize=8 shape=box3d tooltip="10"]
N1 -> N1_0 [label=" 10" weight=100 tooltip="10" labeltooltip="10"]
NN1_0 [label = "numeric\"quote\"" id="NN1_0" fontsize=8 shape=box3d tooltip="20"]
N1 -> NN1_0 [label=" 20" weight=100 tooltip="20" labeltooltip="20"]
N2 [label="dest\n15 (15.00%)\nof 25 (25.00%)" id="node2" fontsize=24 shape=box tooltip="dest (25)" color="#b23c00" fillcolor="#edddd5"]
N1 -> N2 [label=" 10" weight=11 color="#b28559" tooltip="src -> dest (10)" labeltooltip="src -> dest (10)" minlen=2]
}