> 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]'. The underlying reason is for removing the trace2 dependency from wrapper.c so that when git-std-lib is compiled, there isn't a missing object for trace_git_fsync_stats(), resulting in a compilation error. However I do agree that the method I chose to do so by creating an ad-hoc wrapper is unsustainable and I will come up with a better method for doing so. > > 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. I don't quite agree that functions _anywhere in Git_ are able to use the Trace2 API directly for the same reason that we don't have the ability to log functions in external libraries -- logging common, low-level functionality creates an unnecessary amount of log churn and those logs generally contain practically useless information. However, that does not mean that all of the functions in git-std-lib fall into that category (usage has certain functions definitely worth logging). This means that files like usage.c could instead be separated into its own library and git-std-lib would only contain files that we deem "should never be logged". > 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. Besides potentially redrawing the boundaries of git-std-lib to accommodate Trace2, we're also looking into the possibility of stubbing out tracing in git-std-lib so that it and other libraries can be built and tested, and then when Trace2 is turned into a library, it's full functionality can be linked to.
