lkml.org 
[lkml]   [2023]   [Jan]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: [PATCH v1] perf script flamegraph: Avoid d3-flame-graph package dependency
From
On 10.01.23 20:51, Ian Rogers wrote:
> On Tue, Jan 10, 2023 at 10:02 AM Andreas Gerstmayr
> <agerstmayr@redhat.com> wrote:
>>
>> On 05.01.23 10:24, Ian Rogers wrote:
>>> On Wed, Jan 4, 2023 at 7:04 PM Ian Rogers <irogers@google.com> wrote:
>>>>
>>>> Currently flame graph generation requires a d3-flame-graph template to
>>>> be installed. Unfortunately this is hard to come by for things like
>>>> Debian [1]. If the template isn't installed warn and download it from
>>>> jsdelivr CDN. If downloading fails generate a minimal flame graph
>>>> again with the javascript coming from jsdelivr CDN.
>>>>
>>>> [1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=996839
>>>>
>>>> Signed-off-by: Ian Rogers <irogers@google.com>
>>
>> I'm not entirely convinced that this perf script should make a network
>> request. In my opinion
>>
>> * best would be if this template gets packaged in Debian
>> * alternatively print instructions how to install the template:
>>
>> sudo mkdir /usr/share/d3-flame-graph
>> sudo wget -O /usr/share/d3-flame-graph/d3-flamegraph-base.html
>> https://cdn.jsdelivr.net/npm/d3-flame-graph@4/dist/templates/d3-flamegraph-base.html
>>
>> * or eventually just embed the entire template (66 KB) in the Python
>> file (not sure if this is permissible though, it's basically a minified
>> HTML/JS blob)?
>> * if the above options don't work, I'd recommend asking the user before
>> making the network request, and eventually persist the template
>> somewhere so it doesn't need to be downloaded every time
>>
>> What do you think?
>
> So the script following this patch is going to try 3 things:
> 1) look for a local template HTML,
> 2) if a local template isn't found warn and switch to downloading the
> CDN version,
> 3) if the CDN version fails to download then use a minimal (embedded
> in the script) HTML file with JS dependencies coming from the CDN.
>
> For (1) there are references in the generated HTML to www.w3.org,
> meaning the HTML isn't fully hermetic - although these references may
> not matter.

The references to w3.org are XML namespace names. As far as I'm aware
they do not matter, as they are merely identifiers and are never
accessed [1,2]. Therefore the generated HTML file in its current form is
hermetic.

This was a design decision from the start - the flame graph generation
and the resulting HTML file should not use any external resources loaded
over the network (the external host could be inaccessible or compromised
at any time). I understand that this requirement may not apply to every
user or company.

> For (2) there will be a download from the CDN of the template during
> the execution of flamegraph. The template is better than (3) as it
> features additional buttons letting you zoom out, etc. The HTML will
> contain CDN references.
> For (3) the generated HTML isn't hermetic and will use the CDN.
>
> For (2) we could give a prompt before trying to download the template,
> or we could change it so that we give the wget command. I wouldn't
> have the wget command require root privileges, I'd say that the
> template could be downloaded and then the path of the download given
> as an option to the flamegraph program. Downloading the file and then
> adding an option to flamegraph seems inconvenient to the user,
> something that the use of urllib in the patch avoids - it is
> essentially just doing this for the user without storing the file on
> disk. I think I prefer to be less inconvenient, and so the state of
> the code at the moment looks best to me. Given that no choice results
> in a fully hermetic HTML file, they seem similar in this regard. Is it
> okay to try a download with urllib? Should there be a prompt? I think
> the warning is enough and allows scripts, etc. using flamegraph to
> more easily function across differing distributions.

I fully agree, your changes make the flame graph generation more
convenient in case the template is not installed. Also, downloading the
template to a local directory and having to specify the path to the
template every time is an inconvenience.

I think it is a tradeoff between convenience and security/privacy.
jsdelivr can get compromised, serving malicious JS (well, in that case,
printing instructions to jsdelivr is also flawed :|).
In the end it's up to the maintainers to decide.

> An aside, Namhyung pointed out to me that there is a "perf report -g
> folded" option, that was added in part to simplify creating
> flamegraphs:
> http://lkml.kernel.org/r/1447047946-1691-2-git-send-email-namhyung@kernel.org
> Building off of perf report may improve/simplify the flamegraph code,
> or avoid the requirement that libpython be linked against perf.

Yep, in this case another tool is required to generate the flame graph
visualization, for example [3].


[1] https://www.w3.org/TR/xml-names11/
[2] https://developer.mozilla.org/en-US/docs/Web/SVG/Namespaces_Crash_Course
[3] https://github.com/brendangregg/FlameGraph


Cheers,
Andreas

>
> Thanks,
> Ian
>
>
>
>
>>
>> Cheers,
>> Andreas
>>
>>>> ---
>>>> tools/perf/scripts/python/flamegraph.py | 63 ++++++++++++++++++-------
>>>> 1 file changed, 45 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/tools/perf/scripts/python/flamegraph.py b/tools/perf/scripts/python/flamegraph.py
>>>> index b6af1dd5f816..808b0e1c9be5 100755
>>>> --- a/tools/perf/scripts/python/flamegraph.py
>>>> +++ b/tools/perf/scripts/python/flamegraph.py
>>>> @@ -25,6 +25,27 @@ import io
>>>> import argparse
>>>> import json
>>>> import subprocess
>>>> +import urllib.request
>>>> +
>>>> +minimal_html = """<head>
>>>> + <link rel="stylesheet" type="text/css" href="https://cdn.jsdelivr.net/npm/d3-flame-graph@4.1.3/dist/d3-flamegraph.css">
>>>
>>> (hopefully fixed Martin Spier's e-mail address)
>>>
>>> The @4.1.3 comes from the README.md:
>>> https://github.com/spiermar/d3-flame-graph/blob/master/README.md
>>> Does it make sense just to drop it or use @latest ? It'd be nice not
>>> to patch this file for every d3-flame-graph update.
>>>
>>> Thanks,
>>> Ian
>>>
>>>> +</head>
>>>> +<body>
>>>> + <div id="chart"></div>
>>>> + <script type="text/javascript" src="https://d3js.org/d3.v7.js"></script>
>>>> + <script type="text/javascript" src="https://cdn.jsdelivr.net/npm/d3-flame-graph@4.1.3/dist/d3-flamegraph.min.js"></script>
>>>> + <script type="text/javascript">
>>>> + const stacks = [/** @flamegraph_json **/];
>>>> + // Note, options is unused.
>>>> + const options = [/** @options_json **/];
>>>> +
>>>> + var chart = flamegraph();
>>>> + d3.select("#chart")
>>>> + .datum(stacks[0])
>>>> + .call(chart);
>>>> + </script>
>>>> +</body>
>>>> +"""
>>>>
>>>> # pylint: disable=too-few-public-methods
>>>> class Node:
>>>> @@ -50,15 +71,18 @@ class FlameGraphCLI:
>>>> self.args = args
>>>> self.stack = Node("all", "root")
>>>>
>>>> - if self.args.format == "html" and \
>>>> - not os.path.isfile(self.args.template):
>>>> - print("Flame Graph template {} does not exist. Please install "
>>>> - "the js-d3-flame-graph (RPM) or libjs-d3-flame-graph (deb) "
>>>> - "package, specify an existing flame graph template "
>>>> - "(--template PATH) or another output format "
>>>> - "(--format FORMAT).".format(self.args.template),
>>>> - file=sys.stderr)
>>>> - sys.exit(1)
>>>> + if self.args.format == "html":
>>>> + if os.path.isfile(self.args.template):
>>>> + self.template = f"file://{self.args.template}"
>>>> + else:
>>>> + print(f"""
>>>> +Warning: Flame Graph template '{self.args.template}'
>>>> +does not exist, a template will be downloaded via http. To avoid this
>>>> +please install a package such as the js-d3-flame-graph or
>>>> +libjs-d3-flame-graph, specify an existing flame graph template
>>>> +(--template PATH) or another output format (--format FORMAT).
>>>> +""", file=sys.stderr)
>>>> + self.template = "https://cdn.jsdelivr.net/npm/d3-flame-graph@4.1.3/dist/templates/d3-flamegraph-base.html"
>>>>
>>>> @staticmethod
>>>> def get_libtype_from_dso(dso):
>>>> @@ -129,15 +153,18 @@ class FlameGraphCLI:
>>>> options_json = json.dumps(options)
>>>>
>>>> try:
>>>> - with io.open(self.args.template, encoding="utf-8") as template:
>>>> - output_str = (
>>>> - template.read()
>>>> - .replace("/** @options_json **/", options_json)
>>>> - .replace("/** @flamegraph_json **/", stacks_json)
>>>> - )
>>>> - except IOError as err:
>>>> - print("Error reading template file: {}".format(err), file=sys.stderr)
>>>> - sys.exit(1)
>>>> + with urllib.request.urlopen(self.template) as template:
>>>> + output_str = '\n'.join([
>>>> + l.decode('utf-8') for l in template.readlines()
>>>> + ])
>>>> + except Exception as err:
>>>> + print(f"Error reading template {self.template}: {err}\n"
>>>> + "a minimal flame graph will be generated", file=sys.stderr)
>>>> + output_str = minimal_html
>>>> +
>>>> + output_str = output_str.replace("/** @options_json **/", options_json)
>>>> + output_str = output_str.replace("/** @flamegraph_json **/", stacks_json)
>>>> +
>>>> output_fn = self.args.output or "flamegraph.html"
>>>> else:
>>>> output_str = stacks_json
>>>> --
>>>> 2.39.0.314.g84b9a713c41-goog
>>>>
>>>
>>
>> --
>> Red Hat Austria GmbH, Registered seat: A-1200 Vienna, Millennium Tower,
>> 26.floor, Handelskai 94-96, Austria
>> Commercial register: Commercial Court Vienna, FN 479668w
>>
>

--
Red Hat Austria GmbH, Registered seat: A-1200 Vienna, Millennium Tower,
26.floor, Handelskai 94-96, Austria
Commercial register: Commercial Court Vienna, FN 479668w

\
 
 \ /
  Last update: 2023-03-26 23:36    [W:0.066 / U:0.104 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site