Re: [PATCH 1/2] perf stat: Fix JSON metric printout for multiple metrics per line

[Michael Petlan <mpetlan@redhat.com>]

Hello Ian and Arnaldo,

I've recently got back to the JSON test failures. In January, I post two
patches within a patchset. I see that Ian has updated the testcase code,
so the second patch is probably no more needed. However I don't see this
one applied upstream yet, although it seemed on the list, it had got some
acks. Has it been forgotten somehow?

To remind, since the JSON printer engine wasn't ready to print multiple
metrics per line, the JSON output got screwed up in that situation. The
situation happened on some SPR CPUs, where Top-Down metrics are supported.

The patch below fixes the printer to handle this situation correctly.

I remember someone saying that the whole printer engine needs to be
refactored some day, but I'd think that before that happens, it'd be
nice to have it fixed...

What do you think?

Regards,
Michael

On Fri, 20 Jan 2023, Michael Petlan wrote:
> JSON printing engine used to always print metric (even when there should
> be none and other engines (std, csv) would not print it) and the metric
> used to always close the line by printing a closing curly bracket.
> 
> This caused invalid JSON output being generated for top-down metrics,
> when multiple metrics are printed on a single line, so the broken output
> might have looked like:
> 
>   ... "metric-value" : 15.564203, "metric-unit" : \
>   "Memory Bound"}"metric-value" : 14.007787, "metric-unit" : "Core Bound"}
> 
> To fix it, print always the separating comma BEFORE the key:value pairs
> and close the line outside of the JSON metric printing routine.
> 
> Fixes: df936cadfb58 ("perf stat: Add JSON output option")
> 
> Signed-off-by: Michael Petlan <mpetlan@redhat.com>
> ---
>  tools/perf/util/stat-display.c | 28 ++++++++++++++++------------
>  1 file changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index 8bd8b0142630..8f80f6b566d0 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -86,7 +86,7 @@ static void print_running_json(struct perf_stat_config *config, u64 run, u64 ena
>  
>  	if (run != ena)
>  		enabled_percent = 100 * run / ena;
> -	fprintf(config->output, "\"event-runtime\" : %" PRIu64 ", \"pcnt-running\" : %.2f, ",
> +	fprintf(config->output, ", \"event-runtime\" : %" PRIu64 ", \"pcnt-running\" : %.2f",
>  		run, enabled_percent);
>  }
>  
> @@ -121,7 +121,7 @@ static void print_noise_pct_csv(struct perf_stat_config *config,
>  static void print_noise_pct_json(struct perf_stat_config *config,
>  				 double pct)
>  {
> -	fprintf(config->output, "\"variance\" : %.2f, ", pct);
> +	fprintf(config->output, ", \"variance\" : %.2f", pct);
>  }
>  
>  static void print_noise_pct(struct perf_stat_config *config,
> @@ -165,7 +165,7 @@ static void print_cgroup_csv(struct perf_stat_config *config, const char *cgrp_n
>  
>  static void print_cgroup_json(struct perf_stat_config *config, const char *cgrp_name)
>  {
> -	fprintf(config->output, "\"cgroup\" : \"%s\", ", cgrp_name);
> +	fprintf(config->output, ", \"cgroup\" : \"%s\"", cgrp_name);
>  }
>  
>  static void print_cgroup(struct perf_stat_config *config, struct cgroup *cgrp)
> @@ -431,10 +431,11 @@ static void print_metric_json(struct perf_stat_config *config __maybe_unused,
>  	struct outstate *os = ctx;
>  	FILE *out = os->fh;
>  
> -	fprintf(out, "\"metric-value\" : %f, ", val);
> -	fprintf(out, "\"metric-unit\" : \"%s\"", unit);
> -	if (!config->metric_only)
> -		fprintf(out, "}");
> +	if (unit == NULL || fmt == NULL)
> +		return;
> +
> +	fprintf(out, ", \"metric-value\" : %f", val);
> +	fprintf(out, ", \"metric-unit\" : \"%s\"", unit);
>  }
>  
>  static void new_line_json(struct perf_stat_config *config, void *ctx)
> @@ -623,14 +624,14 @@ static void print_counter_value_json(struct perf_stat_config *config,
>  	const char *bad_count = evsel->supported ? CNTR_NOT_COUNTED : CNTR_NOT_SUPPORTED;
>  
>  	if (ok)
> -		fprintf(output, "\"counter-value\" : \"%f\", ", avg);
> +		fprintf(output, "\"counter-value\" : \"%f\"", avg);
>  	else
> -		fprintf(output, "\"counter-value\" : \"%s\", ", bad_count);
> +		fprintf(output, "\"counter-value\" : \"%s\"", bad_count);
>  
>  	if (evsel->unit)
> -		fprintf(output, "\"unit\" : \"%s\", ", evsel->unit);
> +		fprintf(output, ", \"unit\" : \"%s\"", evsel->unit);
>  
> -	fprintf(output, "\"event\" : \"%s\", ", evsel__name(evsel));
> +	fprintf(output, ", \"event\" : \"%s\"", evsel__name(evsel));
>  }
>  
>  static void print_counter_value(struct perf_stat_config *config,
> @@ -835,8 +836,11 @@ static void print_counter_aggrdata(struct perf_stat_config *config,
>  
>  	printout(config, os, uval, run, ena, avg, s);
>  
> -	if (!metric_only)
> +	if (!metric_only) {
> +		if (config->json_output)
> +			fputc('}', output);
>  		fputc('\n', output);
> +	}
>  }
>  
>  static void print_metric_begin(struct perf_stat_config *config,
> -- 
> 2.18.4
> 
>