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 Nodelet/tag names for Dot as well #568

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

NHDaly
Copy link
Contributor

@NHDaly NHDaly commented Oct 5, 2020

This escaping requirement was previously leaking into the implementation
of graph.go, by writing \n directly instead of "\n". This meant that
if it was displayed in a different program besides dot, it would have
been potentially incorrectly escaped.

Now, this commit moves all the escaping directly into dotgraph, which is
more encapsulated, and also allows us to escape other characters in the
name which need escaping.

This required adding a justify option to escapeForDot to preserve old behavior.
This allows tag newlines to stay center justified, and legend label
newlines to stay left justified, as they were before we started escaping
them for Dot.

--

This was meant to be opened as a follow-up PR to #564, but apparently I can't adjust it to use a fork's branch as a diffbase. that's disappointing. I will close this in favor of NHDaly#1 for now.

In some programming languages, e.g. JuliaLang, function names can
contain arbitrary characters. These are represented via the string macro
`var"..."`, which allows constructing identifiers that wouldn't
otherwise parse.

These names are handled correctly by `pprof` in the FlameGraph view, but
before this commit, they would produce an invalid dot file.

This fixes the dot graph export for names that contain `"`.
…otentially harmful string labels.

Remove mistaken `escapeStringForDot()` around tag names
This escaping requirement was previously leaking into the implementation
of `graph.go`, by writing `\n` directly instead of "\n". This meant that
if it was displayed in a different program besides `dot`, it would have
been potentially incorrectly escaped.

Now, this commit moves all the escaping directly into dotgraph, which is
more encapsulated, and also allows us to escape other characters in the
name which need escaping.
This allows tag newlines to stay center justified, and legend label
newlines to stay left justified, as they were before we started escaping
them for Dot.

Also ran gofmt
@google-cla google-cla bot added the cla: yes label Oct 5, 2020
@NHDaly
Copy link
Contributor Author

NHDaly commented Oct 5, 2020

This was meant to be opened as a follow-up PR to #564, but apparently I can't adjust it to use a fork's branch as a diffbase. that's disappointing. I will close this in favor of NHDaly#1 for now.

@NHDaly NHDaly reopened this Oct 5, 2020
@NHDaly NHDaly closed this Oct 5, 2020
@NHDaly NHDaly deleted the nhd-dotgraph-escape-labels--escape-Tags branch February 17, 2022 04:51
@NHDaly NHDaly restored the nhd-dotgraph-escape-labels--escape-Tags branch February 17, 2022 04:51
@NHDaly NHDaly reopened this Feb 17, 2022
@NHDaly
Copy link
Contributor Author

NHDaly commented Feb 17, 2022

i've just realized that i left this PR unmerged and forgot about it after merging #564...

I'm reopening it as a reminder to myself to look into it, and see whether there's still anything here we need to do.. thanks

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.

1 participant