Re: [RFC PATCH 1/8] trace2: log fsync stats in trace2 rather than wrapper
From: Victoria Dye <hidden>
Date: 2023-06-28 02:05:56
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.
...
quoted hunk ↗ jump to hunk
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); +
This function does not belong in 'trace2.h', IMO. The purpose of that file is to contain the generic API for Trace2 (e.g., 'trace2_printf()', 'trace2_region_(enter|exit)'), whereas this function is effectively a wrapper around a specific invocation of that API. You note in the commit message that "wrapper.c should not directly log trace2 statistics" with the reasoning of "[it's] a library boundary," but I suspect the unstated underlying reason is "because it tracks 'count_fsync_*' in static variables." This case would be better handled, then, by replacing the usage in 'wrapper.c' with a new Trace2 counter (API introduced in [1]). That keeps this usage consistent with the API already established for Trace2, rather than starting an unsustainable trend of creating ad-hoc, per-metric wrappers in 'trace2.[c|h]'. An added note re: the commit message - it's extremely important that functions _anywhere in Git_ are able to use the Trace2 API directly. A developer could reasonably want to measure performance, keep track of an interesting metric, log when a region is entered in the larger trace, capture error information, etc. for any function, regardless of where in falls in the internal library organization. To that end, I think either the commit message should be rephrased to remove that statement (if the issue is really "we're using a static variable and we want to avoid that"), or the libification effort should be updated to accommodate use of Trace2 anywhere in Git. [1] https://lore.kernel.org/git/pull.1373.v4.git.1666618868.gitgitgadget@gmail.com/ (local)