Skip to content
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.

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

Escape labels when generating graph #557

Merged
merged 1 commit into from
Sep 5, 2020
Merged

Conversation

auxiliary
Copy link
Contributor

Profile comments can have various special symbols which are then inserted as labels when generating graphs. If symbols are not escaped, graph generation can fail. This PR fixes that.

@google-cla google-cla bot added the cla: yes label Sep 2, 2020
internal/graph/dotgraph.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Sep 3, 2020

Codecov Report

Merging #557 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #557      +/-   ##
==========================================
+ Coverage   68.44%   68.46%   +0.01%     
==========================================
  Files          78       78              
  Lines       16098    16108      +10     
==========================================
+ Hits        11018    11028      +10     
  Misses       4225     4225              
  Partials      855      855              
Impacted Files Coverage Δ
internal/graph/dotgraph.go 91.42% <100.00%> (+0.17%) ⬆️
internal/report/report.go 30.83% <100.00%> (ø)
...gh.neting.cc/google/pprof/internal/graph/dotgraph.go 91.42% <0.00%> (+0.17%) ⬆️

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 1a94d86...2926496. Read the comment docs.

@auxiliary auxiliary force-pushed the master branch 3 times, most recently from db29f0d to a94a50b Compare September 3, 2020 05:48
internal/graph/dotgraph_test.go Outdated Show resolved Hide resolved
internal/graph/dotgraph.go Outdated Show resolved Hide resolved
internal/graph/dotgraph.go Outdated Show resolved Hide resolved
internal/graph/dotgraph.go Outdated Show resolved Hide resolved
// 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.
func escapeForDot(s []string) []string {
Copy link
Collaborator

@aalexand aalexand Sep 4, 2020

Choose a reason for hiding this comment

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

Could this function construct and return a new slice? Its current signature suggests that it does that (because it returns a slice) but in fact it changes the input slice in place. The combination of in-place modification and returning a value is imo not good. Also, I can't say just from this local context if it's OK to modify caller's labels slice content, so I'd prefer not modifying its contents.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, makes sense. Done.

@aalexand aalexand merged commit acf8798 into google:master Sep 5, 2020
giordano added a commit to JuliaPackaging/Yggdrasil that referenced this pull request Nov 13, 2020
* Update pprof to latest revision

Bump from 20191205061153 => 20201109224723

My personal interest is to pull in google/pprof#564, which adds support for displaying names with `"` in them, which julia functions sometimes have (e.g. `var"#foo#23"`)

Includes:
- google/pprof#564
- google/pprof#575
- google/pprof#574
- google/pprof#571
- google/pprof#572
- google/pprof#570
- google/pprof#562
- google/pprof#561
- google/pprof#565
- google/pprof#560
- google/pprof#563
- google/pprof#557
- google/pprof#554
- google/pprof#552
- google/pprof#545
- google/pprof#549
- google/pprof#547
- google/pprof#541
- google/pprof#534
- google/pprof#542
- google/pprof#535
- google/pprof#531
- google/pprof#530
- google/pprof#528
- google/pprof#522
- google/pprof#525
- google/pprof#527
- google/pprof#519
- google/pprof#520
- google/pprof#517
- google/pprof#518
- google/pprof#514
- google/pprof#513
- google/pprof#510
- google/pprof#508
- google/pprof#506
- google/pprof#509
- google/pprof#504

* Update P/pprof/build_tarballs.jl - use a real version number

Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>

* Remove now unused `timestamp`

* [pprof] Use `GitSource`

Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>
gmarin13 pushed a commit to gmarin13/pprof that referenced this pull request Dec 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants