Re: [RFC PATCH 1/8] trace2: log fsync stats in trace2 rather than wrapper

From: Victoria Dye <vdye@github.com>
Date: Jul 5, 2023, 18:22 UTC
Message-ID: <151e7426-e5b2-b078-7b51-63a4bd99dfed@github.com>
In-Reply-To: <CAFySSZCYpxNcK6Tfagk1ZBs6cOT9XvWm2b3DJhN0fSPsQCWR1g@mail.gmail.com>

Calvin Wan wrote:

Show 12 quoted lines

>> 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.

That may be true in your use cases, but it isn't in mine and may not be for others'. In fact, I was just using these exact fsync metrics a couple weeks ago to do some performance analysis; I could easily imagine doing something similar for another "low level" function. It's unreasonable - and unfair to future development - to make an absolute declaration about "what's useful vs. useless" and use that decision to justify severely limiting our future flexibility on the matter.

Show 6 quoted lines

> 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".

How do you make that determination? What about if/when someone realizes, somewhere down the line, that one of those "should never be logged" files would actually benefit from some aggregate metric, e.g. a Trace2 timer? This isn't a case of extracting an extraneous dependency (where a function really doesn't _need_ something it has access to); tracing & logging is a core functionality in Git, and should not be artificially constrained in the name of organization.

Show 11 quoted lines

>> 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.

If that allows you to meet your libification goals without limiting Trace2's accessibility throughout the codebase, that works for me.