Skip to content
This repository has been archived by the owner on Sep 30, 2024. It is now read-only.

lib/log: internalize logger flushing #34323

Merged
merged 2 commits into from
Apr 22, 2022
Merged

lib/log: internalize logger flushing #34323

merged 2 commits into from
Apr 22, 2022

Conversation

bobheadxi
Copy link
Member

@bobheadxi bobheadxi commented Apr 22, 2022

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 uber-go/zap#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

@bobheadxi bobheadxi requested a review from jhchabran April 22, 2022 04:38
@cla-bot cla-bot bot added the cla-signed label Apr 22, 2022
@sourcegraph-bot
Copy link
Contributor

Codenotify: Notifying subscribers in OWNERS files for diff 273d79b...e4b2fc7.

Notify File(s)
@sourcegraph/dev-experience lib/log/init.go
lib/log/internal/globallogger/globallogger.go
lib/log/logger.go
lib/log/logtest/logtest.go

@bobheadxi bobheadxi enabled auto-merge (squash) April 22, 2022 04:55
@bobheadxi bobheadxi merged commit 6153851 into main Apr 22, 2022
@bobheadxi bobheadxi deleted the zap-no-sync branch April 22, 2022 13:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants