Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230105030434.255603-1-irogers@google.com> <CAP-5=fXi_9zdhTAoYApiFQoLURAvpEatFzU3uL23o3zs=z25ZQ@mail.gmail.com>
 <bd4a55a4-8e9c-82bf-92c8-1fa354289dce@redhat.com>
In-Reply-To: <bd4a55a4-8e9c-82bf-92c8-1fa354289dce@redhat.com>
From:   Ian Rogers <irogers@google.com>
Date:   Tue, 10 Jan 2023 11:51:26 -0800
Message-ID: <CAP-5=fX3LaJgft8T+nYV8SHpmoAOYZ9S5-pztKF9j--Um4HKDQ@mail.gmail.com>
Subject: Re: [PATCH v1] perf script flamegraph: Avoid d3-flame-graph package dependency
To:     Andreas Gerstmayr <agerstmayr@redhat.com>
Cc:     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
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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.
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.

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.

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
>