Re: [PATCH 3/8] perf/aggregate: implement codespeed JSON output

[Christian Couder <christian.couder@gmail.com>]

On Wed, Dec 13, 2017 at 9:33 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>>  my $resultsdir = "test-results";
>> +my $results_section = "";
>>  if (exists $ENV{GIT_PERF_SUBSECTION} and $ENV{GIT_PERF_SUBSECTION} ne "") {
>>       $resultsdir .= "/" . $ENV{GIT_PERF_SUBSECTION};
>> +     $results_section = $ENV{GIT_PERF_SUBSECTION};
>>  }
>
> ...
>
>> +     my $executable;
>> +     if ($results_section eq "") {
>> +             $executable = `uname -o -p`;
>> +     } else {
>> +             $executable = $results_section;
>> +             chomp $executable;
>> +     }
>
> Aside from portability of 'uname -o' Eric raised, I wonder if the
> platform information is still useful even when perf-subsection is
> specified.  With the above code, we can identify that a single
> result is for (say) MacOS only when we are not limiting to a single
> subsection, but wouldn't it be equally a valid desire to be able to
> track performance figures for a single subsection over time and
> being able to say "On MacOS, subsection A's performance dropped
> between release X and X+1 quite a bit, but on Linux x86-64, there
> was no such change" or somesuch?

Yeah, I agree that it would be useful. Unfortunately it looks like the
number of fields in Codespeed is limited and I am not sure what will
be more important for people in general. Another option would be to
have "MacOS" in the "environment" field. Or maybe there is a need for
a generic way to customize this. For now I just tried to come up with
something sensible for what AEvar and me want to do.

> IOW, shouldn't the "executable" label always contain the platform
> information, plus an optional subsection info when (and only when)
> the result is limited to a subsection?
>
> By the way, $results_section that is not an empty string at this
> point must have come from $ENV{GIT_PERF_SUBSECTION}, and from the
> way the environment variable is used in t/perf/run, e.g.
>
>                 (
>                         GIT_PERF_SUBSECTION="$subsec"
>                         export GIT_PERF_SUBSECTION
>                         echo "======== Run for subsection '$GIT_PERF_SUBSECTION' ========"
>                         run_subsection "$@"
>                 )
>
> I do not think we expect it to have a trailing LF.  What's that
> chomp doing there?

It's a silly mistake I made when I reorganized the patches just before
sending them. The chomp should be after "$executable = `uname -o -p`;"
(so in the other branch of the "if ... else"). I will fix this in the
next version.