From: Andreas Gerstmayr <agerstmayr@redhat.com>
To: Ian Rogers <irogers@google.com>,
Peter Zijlstra <peterz@infradead.org>,
Ingo Molnar <mingo@redhat.com>,
Arnaldo Carvalho de Melo <acme@kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
Alexander Shishkin <alexander.shishkin@linux.intel.com>,
Jiri Olsa <jolsa@kernel.org>, Namhyung Kim <namhyung@kernel.org>,
linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org,
996839@bugs.debian.org, Brendan Gregg <brendan@intel.com>,
spiermar@gmail.com
Subject: Re: [PATCH v1] perf script flamegraph: Avoid d3-flame-graph package dependency
Date: Tue, 10 Jan 2023 19:02:26 +0100 [thread overview]
Message-ID: <bd4a55a4-8e9c-82bf-92c8-1fa354289dce@redhat.com> (raw)
In-Reply-To: <CAP-5=fXi_9zdhTAoYApiFQoLURAvpEatFzU3uL23o3zs=z25ZQ@mail.gmail.com>
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?
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
next prev parent reply other threads:[~2023-01-10 18:06 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-05 3:04 [PATCH v1] perf script flamegraph: Avoid d3-flame-graph package dependency Ian Rogers
2023-01-05 9:24 ` Ian Rogers
2023-01-06 15:31 ` Martin Spier
2023-01-09 19:42 ` Ian Rogers
2023-01-10 18:02 ` Andreas Gerstmayr [this message]
2023-01-10 19:51 ` Ian Rogers
2023-01-11 13:18 ` Andreas Gerstmayr
2023-01-11 17:13 ` Ian Rogers
2023-01-11 18:08 ` Andreas Gerstmayr
2023-01-12 22:28 ` Ian Rogers
2023-01-14 15:03 ` Bug#996839: " Salvatore Bonaccorso
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=bd4a55a4-8e9c-82bf-92c8-1fa354289dce@redhat.com \
--to=agerstmayr@redhat.com \
--cc=996839@bugs.debian.org \
--cc=acme@kernel.org \
--cc=alexander.shishkin@linux.intel.com \
--cc=brendan@intel.com \
--cc=irogers@google.com \
--cc=jolsa@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.org \
--cc=peterz@infradead.org \
--cc=spiermar@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).