From: Jeff Hostetler <git@jeffhostetler.com>
Date: Tue, 11 Jul 2023 20:07:15 GMT
Subject: Re: [RFC PATCH 1/8] trace2: log fsync stats in trace2 rather than wrapper
Message-ID: <7c8d6012-5bb5-9ee5-eda1-80e46c4b6981@jeffhostetler.com>
In-Reply-To: <20230627195251.1973421-2-calvinwan@google.com>



On 6/27/23 3:52 PM, Calvin Wan wrote:
> As a library boundary, wrapper.c should not directly log trace2
> statistics, but instead provide those statistics upon
> request. Therefore, move the trace2 logging code to trace2.[ch.]. This
> also allows wrapper.c to not be dependent on trace2.h and repository.h.
> 
> Signed-off-by: Calvin Wan <calvinwan@google.com>
> ---
>   trace2.c  | 13 +++++++++++++
>   trace2.h  |  5 +++++
>   wrapper.c | 17 ++++++-----------
>   wrapper.h |  4 ++--
>   4 files changed, 26 insertions(+), 13 deletions(-)
> 
> diff --git a/trace2.c b/trace2.c
> index 0efc4e7b95..f367a1ce31 100644
> --- a/trace2.c
> +++ b/trace2.c
> @@ -915,3 +915,16 @@ const char *trace2_session_id(void)
>   {
>   	return tr2_sid_get();
>   }
> +
> +static void log_trace_fsync_if(const char *key)
> +{
> +	intmax_t value = get_trace_git_fsync_stats(key);
> +	if (value)
> +		trace2_data_intmax("fsync", the_repository, key, value);
> +}
> +
> +void trace_git_fsync_stats(void)
> +{
> +	log_trace_fsync_if("fsync/writeout-only");
> +	log_trace_fsync_if("fsync/hardware-flush");
> +}
> diff --git a/trace2.h b/trace2.h
> index 4ced30c0db..689e9a4027 100644
> --- a/trace2.h
> +++ b/trace2.h
> @@ -581,4 +581,9 @@ void trace2_collect_process_info(enum trace2_process_info_reason reason);
>   
>   const char *trace2_session_id(void);
>   
> +/*
> + * Writes out trace statistics for fsync
> + */
> +void trace_git_fsync_stats(void);
> +
>   #endif /* TRACE2_H */

Sorry to be late to this party, but none of this belongs
in trace2.[ch].

As Victoria stated, you can/should use the new "timers and counters"
feature in Trace2 to collect and log these stats.

And then you don't need specific log_trace_* functions or wrappers
-- just use the trace2_timer_start()/stop() or trace2_counter_add()
functions as necessary around the various fsync operations.


I haven't really followed the lib-ification effort, so I'm just going
to GUESS that all of the Trace2_ and tr2_ prefixed functions and data
structures will need to be in the lowest-level .a so that it can be
called from the main .exe and any other .a's between them.

Jeff