internal/graph: Escape tag labels correctly for dot #683
Conversation
Fixes syntax errors when trying to render pprof profiles that have
double quotes in tags. These can be created with Go's pprof labels
feature, for example with:
pprof.Labels("key", "label \"double quote\"\nline two")
Without this change, trying to render a diagram will fail with:
Error: <stdin>: syntax error in line 5 near 'quote'
The problem is that the double quote ('"') was never escaped in the
label strings. This required me changing how multiple tags are
joined. Previously they were joined with the two character `\n`
sequence, which is a centered newline in dot. This changes it to
a single character newline "\n", which is then escaped as
left-justified strings "\l" in dot.
* Add a new graph test for labels.
* Add a test for joinLabels.
* Update the tests in driver which converts \n to \l.
Codecov Report
@@ Coverage Diff @@
## master #683 +/- ##
==========================================
+ Coverage 68.13% 68.19% +0.05%
==========================================
Files 41 41
Lines 7385 7385
==========================================
+ Hits 5032 5036 +4
+ Misses 1911 1906 -5
- Partials 442 443 +1
Continue to review full report at Codecov.
|
|
The Here is what the label nodes look like with
|
internal/graph/graph.go
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Okay! I've edited this since it needs to change a bit to accommodate the escaping change you mentioned below.
| @@ -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"] | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
escapeForDoton 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
[]stringfor labels instead ofstring.
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.
This was suggested by code review. This requires a bit of a hack in joinLabels, so we have the right combination of escaped and not escaped strings.
|
I do not like that |
|
Hello! Is there anything else I should do to get this merged? I ran into another profile today that failed to render because of this bug. Thanks! |
|
I tested the change a bit and I think it breaks how profile comments with newline characters in them are rendered - they are not displayed as newline characters anymore. |
|
Great find, thanks for pointing it out! I've added a test for this case, and fixed a related escaping bug in this feature in #688 ; Once that is merged, I will fix this change, since the test in that pull request will now change and fail with this change. |
|
Closing in favour of #689 |
Fixes syntax errors when trying to render pprof profiles that have
double quotes in tags. These can be created with Go's pprof labels
feature, for example with:
Without this change, trying to render a diagram will fail with:
The problem is that the double quote ('"') was never escaped in the
label strings. This required me changing how multiple tags are
joined. Previously they were joined with the two character
\nsequence, which is a centered newline in dot. This changes it to
a single character newline "\n", which is then escaped as
left-justified strings "\l" in dot.