Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: > Fix the overly verbose trace2 logging added in 9a4987677d3 (trace2: > add stats for fsync operations, 2022-03-30) (first released with > v2.36.0). > > Since that change every single "git" command invocation has included > these "data" events, even though we'll only make use of these with > core.fsyncMethod=batch, and even then only have non-zero values if > we're writing object data to disk. See c0f4752ed2f (core.fsyncmethod: > batched disk flushes for loose-objects, 2022-04-04) for that feature. > > The result was that e.g. "git version" would previously emit a total > of 6 trace2 events with the GIT_TRACE2_EVENT target (version, start, > cmd_ancestry, cmd_name, exit, atexit), but afterwards would emit > 8. We'd emit 2 "data" events before the "exit" event. > > The reason we didn't catch this was that the trace2 unit tests added > in a15860dca3f (trace2: t/helper/test-trace2, t0210.sh, t0211.sh, > t0212.sh, 2019-02-22) would omit any "data" events that weren't the > ones it cared about. Before this change to the C code 6/7 of our > "t/t0212-trace2-event.sh" tests would fail if this change was applied > to "t/t0212/parse_events.perl". > > Let's make the trace2 testing more strict, and further append any new > events types we don't know about in "t/t0212/parse_events.perl". Since > we only invoke the "test-tool trace2" there's no guarantee that we'll > catch other overly verbose events in the future, but we'll at least > notice if we start emitting new events that are issues every time we > log anything with trace2's JSON target. > > We exclude the "data_json" event type, we'd otherwise would fail on > both "win test" and "win+VS test" CI due to the logging added in > 353d3d77f4f (trace2: collect Windows-specific process information, > 2019-02-22). It looks like that logging should really be using > trace2_cmd_ancestry() instead, which was introduced later in > 2f732bf15e6 (tr2: log parent process name, 2021-07-21), but let's > leave it for now. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> > --- > > This is a fix for a v2.36.0 regression/change, so perhaps it's > categorically out for v2.37.0, but given how isolated the fix is & how > verbose the new trace2 data is perhaps it's worth including anyway. > > Passing CI at: https://github.com/avar/git/actions/runs/2548732003 > > t/t0212/parse_events.perl | 17 +++++++++++------ > wrapper.c | 3 +++ > 2 files changed, 14 insertions(+), 6 deletions(-) > > diff --git a/t/t0212/parse_events.perl b/t/t0212/parse_events.perl > index b6408560c0c..389cf6f61c4 100644 > --- a/t/t0212/parse_events.perl > +++ b/t/t0212/parse_events.perl > @@ -216,12 +216,17 @@ > > elsif ($event eq 'data') { > my $cat = $line->{'category'}; > + my $key = $line->{'key'}; > + my $value = $line->{'value'}; > + $processes->{$sid}->{'data'}->{$cat}->{$key} = $value; > + } > + > + elsif ($event eq 'data_json') { > + # Ignore due to compat/win32/trace2_win32_process_info.c > + } > + > + else { > + push @{$processes->{$sid}->{$event}} => $line->{value}; > } The special casing is unfortunate, but if it is "let's leave it for now", probably we should leave a NEEDSWORK comment here to remind us, instead of giving a false impresion that abusing data_json is OK? > diff --git a/wrapper.c b/wrapper.c > index 1c3c970080b..eebd764131f 100644 > --- a/wrapper.c > +++ b/wrapper.c > @@ -618,6 +618,9 @@ int git_fsync(int fd, enum fsync_action action) > > void trace_git_fsync_stats(void) > { > + if (!count_fsync_writeout_only && !count_fsync_hardware_flush) > + return; > + OK, both counters are unconditionally incremented whenever the corresponding action triggers, so this implementation is probably OK for now, but I suspect that it would become a maintenance burden if we were to add more fsync actions later. > trace2_data_intmax("fsync", the_repository, "fsync/writeout-only", count_fsync_writeout_only); > trace2_data_intmax("fsync", the_repository, "fsync/hardware-flush", count_fsync_hardware_flush); If each of these is guarded with "show only when non-zero", that would become less problematic, though. A bigger issue is when somebody is debugging the callers of git_fsync() wondering why they do not trigger. Getting log entries that say "no git_fsync() call with these actions have been made" may be useful (i.e. explicitly negative answer). I am not sure if it is a good solution in the longer term, but this implementation may be a good-enough way to squelch the immediate loudness issue. Thanks.
