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;
Message-ID: <68e8840e-d894-c211-184d-91a06c87579c@redhat.com>
Date:   Wed, 11 Jan 2023 19:08:11 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.0
Subject: Re: [PATCH v1] perf script flamegraph: Avoid d3-flame-graph package
 dependency
Content-Language: en-US
To:     Ian Rogers <irogers@google.com>,
        Michael Petlan <mpetlan@redhat.com>,
        Ben Hutchings <ben@decadent.org.uk>,
        Brendan Gregg <brendan@intel.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, spiermar@gmail.com
References: <20230105030434.255603-1-irogers@google.com>
 <CAP-5=fXi_9zdhTAoYApiFQoLURAvpEatFzU3uL23o3zs=z25ZQ@mail.gmail.com>
 <bd4a55a4-8e9c-82bf-92c8-1fa354289dce@redhat.com>
 <CAP-5=fX3LaJgft8T+nYV8SHpmoAOYZ9S5-pztKF9j--Um4HKDQ@mail.gmail.com>
 <928c76bf-39bc-cbc0-373e-70c5561cd5b0@redhat.com>
 <CAP-5=fWyE_cqumLFxrj_tncnhVSDpiN0cMij3iB_FMY9qmjNWw@mail.gmail.com>
From:   Andreas Gerstmayr <agerstmayr@redhat.com>
In-Reply-To: <CAP-5=fWyE_cqumLFxrj_tncnhVSDpiN0cMij3iB_FMY9qmjNWw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 11.01.23 18:13, Ian Rogers wrote:
> On Wed, Jan 11, 2023 at 5:18 AM Andreas Gerstmayr <agerstmayr@redhat.com> wrote:
>>
>> 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.
> 
> Agreed. It is something of an issue with the perf tool, I think noted
> by Arnaldo and Linus, that it has too many dependencies. We also have
> a problem that a number of dependencies aren't license compatible for
> distribution with perf's GPLv2. flamegraph.py is always installed but
> it carries a dependency that can't be satisfied on Debian and
> everything deriving from it. The prompting to install a package
> solution, as is generally used in the build, is one approach. Using
> http rather than files is the approach this change introduces, and is
> an approach advocated by the d3 flamegraph README.md. Perhaps package
> maintainers like Michael and Ben have opinions here.
> 
>>> 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].
> 
> Thanks, perhaps [3] needs updating to use it as current processing
> appears to be done using perf script:
> https://github.com/brendangregg/FlameGraph/blob/master/stackcollapse-perf.pl
> 
> I think users end up trying out flamegraph as they want something
> easier to read than perf report and the command ships with the tool.
> flamegraph is unique in being like this and it is a pity that the
> Debian half of the world has difficulty making it work.

Maybe someone with Debian packager rights could package the template 
[1]? Then it'd work the same way like with RPM-based distros, and it'll 
also work in environments without network access.

> Fwiw, I'd like
> to start adding documentation to the wiki about how to do easier
> visualizations with other tools, such as the firefox profiler. Perhaps
> some help from the tool also makes sense.

+1
I just tested the steps in the firefox profiler docs [2]. It looks great 
for power users, offering lots of different views, filtering and a 
timeline. There's also speedscope [3], which imho sits between the perf 
flamegraph script and the firefox profiler regarding functionality.


[1] 
https://src.fedoraproject.org/rpms/js-d3-flame-graph/blob/f9efd0ec91e9b88a4f6d47c0e4817c758187b8d2/f/js-d3-flame-graph.spec#_78-85
[2] https://profiler.firefox.com/docs/#/./guide-perf-profiling
[3] https://github.com/jlfwong/speedscope


Cheers,
Andreas

> 
> Thanks,
> Ian
> 
> 
> 
>>
>> [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
>>
> 

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