Re: [PATCH] trace2: log progress time and throughput

From: Junio C Hamano <gitster@pobox.com>
Date: May 15, 2020, 15:27 UTC
Message-ID: <xmqqzha9md5d.fsf@gitster.c.googlers.com>
In-Reply-To: <8f159f13-ed61-61ea-8e9a-c1ffbc5fddb3@gmail.com>

Derrick Stolee <stolee@gmail.com> writes:

Show 6 quoted lines

>> +	if (p_progress && *p_progress) {
>> +		trace2_data_intmax("progress", the_repository, "total_objects",
>> +				   (*p_progress)->total);
>
> Should this be "total_objects"? Progress can iterate over lots of things,
> such as cache entries. Perhaps leave it as "total"?

Good point.

I think the literal strings like "total_objects" we see in this patch can later be customized in the same future change that lets us give a more useful output than "Receiving objects" by having a handful of customizable strings in the progress struct as hinted by Emily under the three-dash line. So I do not mind leaving it as-is. If a reroll has to come without making these strings customizable, it may be an improvement to change it to "total" if we do not forget, but as long as we know where we are going in the longer term, I do not think it is a big deal.

Show 8 quoted lines

>> +
>> +		if ((*p_progress)->throughput)
>> +			trace2_data_intmax("progress", the_repository,
>> +					   "total_bytes",
>> +					   (*p_progress)->throughput->curr_total);
>
> I like the extra detail here for the specific kind of progress used in
> network transfer.

The curr_total field must be counted in bytes, by the fact that strbuf_humanise_bytes() is called by throughput_string() to show it, so "total_bytes" does make perfect sense. The field might want to get renamed to reflect this but obviously that's the kind of change that we would not want in the middle of a more meaningful change like this one.

Show 28 quoted lines

>> +	}
>> +
>> +	trace2_region_leave("progress",
>> +			    p_progress && *p_progress
>> +				? (*p_progress)->title
>> +				: NULL,
>> +			    the_repository);
>> +
>>  	stop_progress_msg(p_progress, _("done"));
>>  }
>
> This trace2_region_leave() needs to be conditional on the progress
> being non-null. The pattern used by consumers of the progress API is:
>
> 	if (my_progress_condition)
> 		start_progress(&progress);
>
> 	do {
> 		display_progress(&progress, count);
> 	} while (condition);
>
> 	stop_progress(&progress);
>
> The condition to show progress or not is conditional on an option that
> is external to the progress API.
>
> The fix for this patch is simple: make the trace2_region_leave() be
> conditional on "p_progress && *p_progress".

Makes sense.

Show 7 quoted lines

> This leads to an extra problem: if a user uses an option such as "--quiet",
> then the trace2 regions won't appear at all. This becomes even more important
> when thinking about scripts or tools that have stderr as a non-TTY, which
> disables progress most of the time.
>
> It's best to have trace2 data be consistent across commands. I think this can
> be accomplished, but it is a more invasive change to the rest of the codebase.

True, because "--quiet" means we cannot piggyback on the progress code.

Show 14 quoted lines

> It requires invoking the progress API in all cases, and having the progress
> API conditionally initialize the progress struct. The new pattern would look
> like this:
>
> 	start_progress(&progress, my_progress_condition);
>
> 	do {
> 		display_progress(&progress, count);
> 	} while (condition);
>
> 	stop_progress(&progress);
>
> Then, start_progress() (and variants) could always start the trace2 region,
> and stop_progress() could always end the trace2 region.

OK.  Good analysis.

Thanks.