Skip to content

lib/log: internalize logger flushing

Warren Gifford requested to merge zap-no-sync into main

Created by: bobheadxi

This PR removes Sync() from the logger interface and adds a callback returned by log.Init which should be called before application exit. The removal of Sync() from the Logger interface helps indicate that not every logger from log.Scoped needs a call to Sync(). For logtest, we add a t.Cleanup func to sync the global logger.

From https://github.com/sourcegraph/sourcegraph/pull/34320 I've noticed that log.Fatal is used quite liberally. Even if we switch to error propagation, there's still the issue that os.Exit will bypass any defer calls and hence potentially miss Sync(). To accommodate this pattern, I've decided to add back Fatal to Logger, which internally calls Sync() before os.Exit(1). Also see https://github.com/uber-go/zap/issues/207 for more rationale for supporting Fatal.

Background: All loggers are children of the root global logger that is instantiated once. As such, it is sufficient to just call Sync() on the root logger - all child loggers just write to underlying loggers until they reach the root, which as far as I can tell is an os.File that has Sync(), which gets called by Zap, so it's not a no-op even if Zap itself does not do any buffering (at the moment, the global logger is not configured to do so)

Aside: in practice, it's honestly probably not a huge deal to never call Sync(), but we should anyway just in case, so hopefully this balances the probable need to call Sync() with ergonomics.

Part of https://github.com/sourcegraph/sourcegraph/issues/33192

Test plan

n/a

Merge request reports

Loading