From 4de7706662dde6d99095846ac249de5db018d36c Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Thu, 17 Aug 2023 08:00:35 -0700 Subject: [PATCH 1/5] ci: Don't fire double job on branch + PR (#1324) CI setup currently triggers build-test-lint jobs for pull requests and pushes to any and all branches. This has the negative effect of spinning up double jobs for pull requests made from branches in the uber-go/zap repository. This changes CI to only fire on pushes to master and for PRs. Caveat: This means that if someone pushes a test branch to uber-go/zap, it won't run CI until they create a PR (draft or otherwise). --- .github/workflows/go.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index 60a5d9b34..c18a5c986 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -2,7 +2,7 @@ name: Go on: push: - branches: ['*'] + branches: [master] tags: ['v*'] pull_request: branches: ['*'] From c94c2bbcd05fb635a777ef8e41a7e270e4163cd7 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Fri, 18 Aug 2023 11:29:29 -0700 Subject: [PATCH 2/5] Use golangci-lint for linting (#1323) Zap has been maintaining its own bespoke linter setup for a while and it's starting to show its age. For example, the upgrade in #1289 can't go through without either making a change which we disagree with, or configuring revive separately to ignore that linter check. This PR switches to using golangci-lint to run linters. staticcheck is included and enabled by default. For revive, this adds the relevant opt-outs with adequate justification. It also enables a couple govet linter checks that would otherwise be difficult to enable. The result also simplifies our Makefile greatly with the minor added complexity that it assumes golangci-lint is already installed. Another advantage of this setup is that lint runs in a separate parallel CI job while the tests are doing their thing. --- .github/workflows/go.yml | 31 +++++++++++++----- .golangci.yml | 53 ++++++++++++++++++++++++++++++ Makefile | 71 ++++++++++++++++++---------------------- exp/go.sum | 2 -- logger.go | 4 +-- sink_test.go | 5 ++- tools/go.mod | 18 +--------- tools/go.sum | 46 -------------------------- tools/tools.go | 2 -- writer.go | 10 +++--- 10 files changed, 117 insertions(+), 125 deletions(-) create mode 100644 .golangci.yml diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index c18a5c986..755aa19be 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -19,17 +19,15 @@ jobs: go: ["1.20.x", "1.21.x"] include: - go: 1.21.x - latest: true steps: - name: Checkout code uses: actions/checkout@v3 - name: Setup Go - uses: actions/setup-go@v3 + uses: actions/setup-go@v4 with: go-version: ${{ matrix.go }} - cache: true cache-dependency-path: '**/go.sum' - name: Download Dependencies @@ -39,16 +37,33 @@ jobs: (cd benchmarks && go mod download) (cd zapgrpc/internal/test && go mod download) - - name: Lint - if: matrix.latest - run: make lint - - name: Test run: make cover - name: Upload coverage to codecov.io uses: codecov/codecov-action@v3 + lint: + name: Lint + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@v3 + name: Check out repository + - uses: actions/setup-go@v4 + name: Set up Go + with: + go-version: 1.21.x + cache: false # managed by golangci-lint + + - uses: golangci/golangci-lint-action@v3 + name: Install golangci-lint + with: + version: latest + args: --version # make lint will run the linter + + - run: make lint + name: Lint + - name: vulncheck - if: matrix.latest run: make vulncheck diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 000000000..154951275 --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,53 @@ +output: + # Make output more digestible with quickfix in vim/emacs/etc. + sort-results: true + print-issued-lines: false + +linters: + # We'll track the golangci-lint default linters manually + # instead of letting them change without our control. + disable-all: true + enable: + # golangci-lint defaults: + # - errcheck TODO: enable errcheck + - gosimple + - govet + - ineffassign + - staticcheck + - unused + + # Our own extras: + - gofmt + - nolintlint # lints nolint directives + - revive + +linter-settings: + govet: + # These govet checks are disabled by default, but they're useful. + enable: + - niliness + - reflectvaluecompare + - sortslice + - unusedwrite + +issues: + # Print all issues reported by all linters. + max-issues-per-linter: 0 + max-same-issues: 0 + + # Don't ignore some of the issues that golangci-lint considers okay. + # This includes documenting all exported entities. + exclude-use-default: false + + exclude-rules: + # Don't warn on unused parameters. + # Parameter names are useful; replacing them with '_' is undesirable. + - linters: [revive] + text: 'unused-parameter: parameter \S+ seems to be unused, consider removing or renaming it as _' + + # staticcheck already has smarter checks for empty blocks. + # revive's empty-block linter has false positives. + # For example, as of writing this, the following is not allowed. + # for foo() { } + - linters: [revive] + text: 'empty-block: this block is empty, you can remove it' diff --git a/Makefile b/Makefile index 5bee00ad0..eb1cee53b 100644 --- a/Makefile +++ b/Makefile @@ -1,7 +1,9 @@ -export GOBIN ?= $(shell pwd)/bin +# Directory containing the Makefile. +PROJECT_ROOT = $(dir $(abspath $(lastword $(MAKEFILE_LIST)))) + +export GOBIN ?= $(PROJECT_ROOT)/bin +export PATH := $(GOBIN):$(PATH) -REVIVE = $(GOBIN)/revive -STATICCHECK = $(GOBIN)/staticcheck GOVULNCHECK = $(GOBIN)/govulncheck BENCH_FLAGS ?= -cpuprofile=cpu.pprof -memprofile=mem.pprof -benchmem @@ -11,47 +13,40 @@ MODULE_DIRS = . ./exp ./benchmarks ./zapgrpc/internal/test # Directories that we want to track coverage for. COVER_DIRS = . ./exp -# Many Go tools take file globs or directories as arguments instead of packages. -GO_FILES := $(shell \ - find . '(' -path '*/.*' -o -path './vendor' ')' -prune \ - -o -name '*.go' -print | cut -b3-) - .PHONY: all all: lint test .PHONY: lint -lint: $(REVIVE) $(STATICCHECK) - @rm -rf lint.log - @echo "Checking formatting..." - @gofmt -d -s $(GO_FILES) 2>&1 | tee lint.log - @echo "Checking vet..." - @$(foreach dir,$(MODULE_DIRS),(cd $(dir) && go vet ./... 2>&1) &&) true | tee -a lint.log - @echo "Checking lint..." - @$(foreach dir,$(MODULE_DIRS),(cd $(dir) && \ - $(REVIVE) -set_exit_status ./... 2>&1) &&) true | tee -a lint.log - @echo "Checking staticcheck..." - @$(foreach dir,$(MODULE_DIRS),(cd $(dir) && $(STATICCHECK) ./... 2>&1) &&) true | tee -a lint.log - @echo "Checking for unresolved FIXMEs..." - @git grep -i fixme | grep -v -e Makefile | tee -a lint.log - @echo "Checking for license headers..." - @./checklicense.sh | tee -a lint.log - @[ ! -s lint.log ] - @echo "Checking 'go mod tidy'..." - @make tidy - @if ! git diff --quiet; then \ - echo "'go mod tidy' resulted in changes or working tree is dirty:"; \ - git --no-pager diff; \ - fi - -$(REVIVE): - cd tools && go install github.com/mgechev/revive +lint: golangci-lint tidy-lint license-lint + +.PHONY: golangci-lint +golangci-lint: + @$(foreach mod,$(MODULE_DIRS), \ + (cd $(mod) && \ + echo "[lint] golangci-lint: $(mod)" && \ + golangci-lint run --path-prefix $(mod)) &&) true + +.PHONY: tidy +tidy: + @$(foreach dir,$(MODULE_DIRS), \ + (cd $(dir) && go mod tidy) &&) true + +.PHONY: tidy-lint +tidy-lint: + @$(foreach mod,$(MODULE_DIRS), \ + (cd $(mod) && \ + echo "[lint] tidy: $(mod)" && \ + go mod tidy && \ + git diff --exit-code -- go.mod go.sum) &&) true + + +.PHONY: license-lint +license-lint: + ./checklicense.sh $(GOVULNCHECK): cd tools && go install golang.org/x/vuln/cmd/govulncheck -$(STATICCHECK): - cd tools && go install honnef.co/go/tools/cmd/staticcheck - .PHONY: test test: @$(foreach dir,$(MODULE_DIRS),(cd $(dir) && go test -race ./...) &&) true @@ -76,10 +71,6 @@ updatereadme: rm -f README.md cat .readme.tmpl | go run internal/readme/readme.go > README.md -.PHONY: tidy -tidy: - @$(foreach dir,$(MODULE_DIRS),(cd $(dir) && go mod tidy) &&) true - .PHONY: vulncheck vulncheck: $(GOVULNCHECK) $(GOVULNCHECK) ./... diff --git a/exp/go.sum b/exp/go.sum index 9df6ac4b4..7e0c3e06e 100644 --- a/exp/go.sum +++ b/exp/go.sum @@ -19,8 +19,6 @@ go.uber.org/multierr v1.10.0 h1:S0h4aNzvfcFsC3dRF1jLoaov7oRaKqRGC/pUEJ2yvPQ= go.uber.org/multierr v1.10.0/go.mod h1:20+QtiLqy0Nd6FdQB9TLXag12DsQkrbs3htMFfDN80Y= go.uber.org/zap v1.24.0 h1:FiJd5l1UOLj0wCgbSE0rwwXHzEdAZS6hiiSnxJN/D60= go.uber.org/zap v1.24.0/go.mod h1:2kMP+WWQ8aoFoedH3T2sq6iJ2yDWpHbP0f6MQbS9Gkg= -golang.org/x/exp v0.0.0-20230425010034-47ecfdc1ba53 h1:5llv2sWeaMSnA3w2kS57ouQQ4pudlXrR0dCgw51QK9o= -golang.org/x/exp v0.0.0-20230425010034-47ecfdc1ba53/go.mod h1:V1LtkGg67GoY2N1AnLN78QLrzxkLyJw7RJb1gzOOz9w= golang.org/x/exp v0.0.0-20230801115018-d63ba01acd4b h1:r+vk0EmXNmekl0S0BascoeeoHk/L7wmaW2QF90K+kYI= golang.org/x/exp v0.0.0-20230801115018-d63ba01acd4b/go.mod h1:FXUEEKJgO7OQYeo8N01OfiKP8RXMtf6e8aTskBGqWdc= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= diff --git a/logger.go b/logger.go index 0e9548011..2bebb87b3 100644 --- a/logger.go +++ b/logger.go @@ -288,8 +288,8 @@ func (log *Logger) Name() string { } func (log *Logger) clone() *Logger { - copy := *log - return © + clone := *log + return &clone } func (log *Logger) check(lvl zapcore.Level, msg string) *zapcore.CheckedEntry { diff --git a/sink_test.go b/sink_test.go index 0dfa6162a..5fc37be91 100644 --- a/sink_test.go +++ b/sink_test.go @@ -68,13 +68,12 @@ func TestRegisterSink(t *testing.T) { require.NoError(t, RegisterSink(strings.ToUpper(memScheme), memFactory), "Failed to register scheme %q.", memScheme) require.NoError(t, RegisterSink(nopScheme, nopFactory), "Failed to register scheme %q.", nopScheme) - sink, close, err := Open( + sink, closeSink, err := Open( memScheme+"://somewhere", nopScheme+"://somewhere-else", ) require.NoError(t, err, "Unexpected error opening URLs with registered schemes.") - - defer close() + defer closeSink() assert.Equal(t, 1, memCalls, "Unexpected number of calls to memory factory.") assert.Equal(t, 1, nopCalls, "Unexpected number of calls to no-op factory.") diff --git a/tools/go.mod b/tools/go.mod index 9f5f8789b..aeb98554a 100644 --- a/tools/go.mod +++ b/tools/go.mod @@ -1,24 +1,8 @@ module go.uber.org/zap/tools -require ( - github.com/mgechev/revive v1.2.5 - golang.org/x/vuln v1.0.0 - honnef.co/go/tools v0.4.2 -) +require golang.org/x/vuln v1.0.0 require ( - github.com/BurntSushi/toml v1.2.1 // indirect - github.com/chavacava/garif v0.0.0-20221024190013-b3ef35877348 // indirect - github.com/fatih/color v1.14.1 // indirect - github.com/fatih/structtag v1.2.0 // indirect - github.com/mattn/go-colorable v0.1.13 // indirect - github.com/mattn/go-isatty v0.0.17 // indirect - github.com/mattn/go-runewidth v0.0.9 // indirect - github.com/mgechev/dots v0.0.0-20210922191527-e955255bf517 // indirect - github.com/mitchellh/go-homedir v1.1.0 // indirect - github.com/olekukonko/tablewriter v0.0.5 // indirect - github.com/pkg/errors v0.9.1 // indirect - golang.org/x/exp/typeparams v0.0.0-20221208152030-732eee02a75a // indirect golang.org/x/mod v0.10.0 // indirect golang.org/x/sync v0.1.0 // indirect golang.org/x/sys v0.7.0 // indirect diff --git a/tools/go.sum b/tools/go.sum index d3c914a01..d1f5c7a8b 100644 --- a/tools/go.sum +++ b/tools/go.sum @@ -1,59 +1,13 @@ -github.com/BurntSushi/toml v1.2.1 h1:9F2/+DoOYIOksmaJFPw1tGFy1eDnIJXg+UHjuD8lTak= -github.com/BurntSushi/toml v1.2.1/go.mod h1:CxXYINrC8qIiEnFrOxCa7Jy5BFHlXnUU2pbicEuybxQ= -github.com/chavacava/garif v0.0.0-20221024190013-b3ef35877348 h1:cy5GCEZLUCshCGCRRUjxHrDUqkB4l5cuUt3ShEckQEo= -github.com/chavacava/garif v0.0.0-20221024190013-b3ef35877348/go.mod h1:f/miWtG3SSuTxKsNK3o58H1xl+XV6ZIfbC6p7lPPB8U= -github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= -github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= -github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= -github.com/fatih/color v1.14.1 h1:qfhVLaG5s+nCROl1zJsZRxFeYrHLqWroPOQ8BWiNb4w= -github.com/fatih/color v1.14.1/go.mod h1:2oHN61fhTpgcxD3TSWCgKDiH1+x4OiDVVGH8WlgGZGg= -github.com/fatih/structtag v1.2.0 h1:/OdNE99OxoI/PqaW/SuSK9uxxT3f/tcSZgon/ssNSx4= -github.com/fatih/structtag v1.2.0/go.mod h1:mBJUNpUnHmRKrKlQQlmCrh5PuhftFbNv8Ys4/aAZl94= github.com/google/go-cmdtest v0.4.1-0.20220921163831-55ab3332a786 h1:rcv+Ippz6RAtvaGgKxc+8FQIpxHgsF+HBzPyYL2cyVU= github.com/google/go-cmp v0.5.8 h1:e6P7q2lk1O+qJJb4BtCQXlK8vWEO8V1ZeuEdJNOqZyg= github.com/google/renameio v0.1.0 h1:GOZbcHa3HfsPKPlmyPyN2KEohoMXOhdMbHrvbpl2QaA= -github.com/mattn/go-colorable v0.1.13 h1:fFA4WZxdEF4tXPZVKMLwD8oUnCTTo08duU7wxecdEvA= -github.com/mattn/go-colorable v0.1.13/go.mod h1:7S9/ev0klgBDR4GtXTXX8a3vIGJpMovkB8vQcUbaXHg= -github.com/mattn/go-isatty v0.0.16/go.mod h1:kYGgaQfpe5nmfYZH+SKPsOc2e4SrIfOl2e/yFXSvRLM= -github.com/mattn/go-isatty v0.0.17 h1:BTarxUcIeDqL27Mc+vyvdWYSL28zpIhv3RoTdsLMPng= -github.com/mattn/go-isatty v0.0.17/go.mod h1:kYGgaQfpe5nmfYZH+SKPsOc2e4SrIfOl2e/yFXSvRLM= -github.com/mattn/go-runewidth v0.0.9 h1:Lm995f3rfxdpd6TSmuVCHVb/QhupuXlYr8sCI/QdE+0= -github.com/mattn/go-runewidth v0.0.9/go.mod h1:H031xJmbD/WCDINGzjvQ9THkh0rPKHF+m2gUSrubnMI= -github.com/mgechev/dots v0.0.0-20210922191527-e955255bf517 h1:zpIH83+oKzcpryru8ceC6BxnoG8TBrhgAvRg8obzup0= -github.com/mgechev/dots v0.0.0-20210922191527-e955255bf517/go.mod h1:KQ7+USdGKfpPjXk4Ga+5XxQM4Lm4e3gAogrreFAYpOg= -github.com/mgechev/revive v1.2.5 h1:UF9AR8pOAuwNmhXj2odp4mxv9Nx2qUIwVz8ZsU+Mbec= -github.com/mgechev/revive v1.2.5/go.mod h1:nFOXent79jMTISAfOAasKfy0Z2Ejq0WX7Qn/KAdYopI= -github.com/mitchellh/go-homedir v1.1.0 h1:lukF9ziXFxDFPkA1vsr5zpc1XuPDn/wFntq5mG+4E0Y= -github.com/mitchellh/go-homedir v1.1.0/go.mod h1:SfyaCUpYCn1Vlf4IUYiD9fPX4A5wJrkLzIz1N1q0pr0= -github.com/olekukonko/tablewriter v0.0.5 h1:P2Ga83D34wi1o9J6Wh1mRuqd4mF/x/lgBS7N7AbDhec= -github.com/olekukonko/tablewriter v0.0.5/go.mod h1:hPp6KlRPjbx+hW8ykQs1w3UBbZlj6HuIJcUGPhkA7kY= -github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4= -github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= -github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= -github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= -github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= -github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw= -github.com/stretchr/objx v0.5.0/go.mod h1:Yh+to48EsGEfYuaHDzXPcE3xhTkx73EhmCGUpEOglKo= -github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= -github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU= -github.com/stretchr/testify v1.8.1 h1:w7B6lhMri9wdJUVmEZPGGhZzrYTPvgJArz7wNPgYKsk= -github.com/stretchr/testify v1.8.1/go.mod h1:w2LPCIKwWwSfY2zedu0+kehJoqGctiVI29o6fzry7u4= -golang.org/x/exp/typeparams v0.0.0-20221208152030-732eee02a75a h1:Jw5wfR+h9mnIYH+OtGT2im5wV1YGGDora5vTv/aa5bE= -golang.org/x/exp/typeparams v0.0.0-20221208152030-732eee02a75a/go.mod h1:AbB0pIl9nAr9wVwH+Z2ZpaocVmF5I4GyWCDIsVjR0bk= golang.org/x/mod v0.10.0 h1:lFO9qtOdlre5W1jxS3r/4szv2/6iXxScdzjoBMXNhYk= golang.org/x/mod v0.10.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= golang.org/x/sync v0.1.0 h1:wsuoTGHzEhffawBOhz5CYhcrV4IdKZbEyZjBMuTp12o= golang.org/x/sync v0.1.0/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= -golang.org/x/sys v0.0.0-20220811171246-fbc7d0a398ab/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.7.0 h1:3jlCCIQZPdOYu1h8BkNvLz8Kgwtae2cagcG/VamtZRU= golang.org/x/sys v0.7.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/tools v0.8.1-0.20230421161920-b9619ee54b47 h1:fQlOhMJ24apqitZX8S4hbCbHU1Z9AvyWkN3BYI55Le4= golang.org/x/tools v0.8.1-0.20230421161920-b9619ee54b47/go.mod h1:JxBZ99ISMI5ViVkT1tr6tdNmXeTrcpVSD3vZ1RsRdN4= golang.org/x/vuln v1.0.0 h1:tYLAU3jD9LQr98Y+3el06lWyGMCnvzw06PIWP3LIy7g= golang.org/x/vuln v1.0.0/go.mod h1:V0eyhHwaAaHrt42J9bgrN6rd12f6GU4T0Lu0ex2wDg4= -gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= -gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= -gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= -gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= -honnef.co/go/tools v0.4.2 h1:6qXr+R5w+ktL5UkwEbPp+fEvfyoMPche6GkOpGHZcLc= -honnef.co/go/tools v0.4.2/go.mod h1:36ZgoUOrqOk1GxwHhyryEkq8FQWkUO2xGuSMhUCcdvA= diff --git a/tools/tools.go b/tools/tools.go index ffe3e42ff..3d1dc14c0 100644 --- a/tools/tools.go +++ b/tools/tools.go @@ -25,7 +25,5 @@ package tools import ( // Tools we use during development. - _ "github.com/mgechev/revive" _ "golang.org/x/vuln/cmd/govulncheck" - _ "honnef.co/go/tools/cmd/staticcheck" ) diff --git a/writer.go b/writer.go index f08728e1e..625b06760 100644 --- a/writer.go +++ b/writer.go @@ -48,19 +48,19 @@ import ( // os.Stdout and os.Stderr. When specified without a scheme, relative file // paths also work. func Open(paths ...string) (zapcore.WriteSyncer, func(), error) { - writers, close, err := open(paths) + writers, closeAll, err := open(paths) if err != nil { return nil, nil, err } writer := CombineWriteSyncers(writers...) - return writer, close, nil + return writer, closeAll, nil } func open(paths []string) ([]zapcore.WriteSyncer, func(), error) { writers := make([]zapcore.WriteSyncer, 0, len(paths)) closers := make([]io.Closer, 0, len(paths)) - close := func() { + closeAll := func() { for _, c := range closers { c.Close() } @@ -77,11 +77,11 @@ func open(paths []string) ([]zapcore.WriteSyncer, func(), error) { closers = append(closers, sink) } if openErr != nil { - close() + closeAll() return nil, nil, openErr } - return writers, close, nil + return writers, closeAll, nil } // CombineWriteSyncers is a utility that combines multiple WriteSyncers into a From c50301e942eb86ce46f1d5bcf5c8a65c175e937b Mon Sep 17 00:00:00 2001 From: Jeremy Quirke Date: Fri, 18 Aug 2023 16:42:31 -0700 Subject: [PATCH 3/5] Add benchmarks for chained Withs (#1326) Add benchmarks to show the performance of chaining child loggers via 'With' and whether the logger is used or not used. This is intended to establish a baseline for a proposed optimisation in PR https://github.com/uber-go/zap/pull/1319. --- logger_bench_test.go | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/logger_bench_test.go b/logger_bench_test.go index bcf501a3f..c7207afd5 100644 --- a/logger_bench_test.go +++ b/logger_bench_test.go @@ -23,6 +23,7 @@ package zap import ( "errors" "runtime" + "strconv" "sync" "testing" "time" @@ -198,6 +199,37 @@ func BenchmarkAddCallerAndStacktrace(b *testing.B) { } }) } +func Benchmark5WithsUsed(b *testing.B) { + benchmarkWithUsed(b, 5, true) +} + +// This benchmark will be used in future as a +// baseline for improving +func Benchmark5WithsNotUsed(b *testing.B) { + benchmarkWithUsed(b, 5, false) +} + +func benchmarkWithUsed(b *testing.B, N int, use bool) { + keys := make([]string, N) + values := make([]string, N) + for i := 0; i < N; i++ { + keys[i] = "k" + strconv.Itoa(i) + values[i] = "v" + strconv.Itoa(i) + } + + b.ResetTimer() + + withBenchedLogger(b, func(log *Logger) { + for i := 0; i < N; i++ { + log = log.With(String(keys[i], values[i])) + } + if use { + log.Info("used") + return + } + runtime.KeepAlive(log) + }) +} func Benchmark10Fields(b *testing.B) { withBenchedLogger(b, func(log *Logger) { From 02ebf0f4a6dbf4df0743791f22443ffea3dfedb3 Mon Sep 17 00:00:00 2001 From: Jeremy Quirke Date: Fri, 18 Aug 2023 16:43:05 -0700 Subject: [PATCH 4/5] Clarify argument capturing behaviour of With (#1325) Make it clear that the contract with the With and logging methods is such that any arguments that could change are captured at the time of the With and logging method, and that any other behaviour introduced in future is intentionally contrary to this contract. There are proposed (https://github.com/uber-go/zap/pull/1319) future opt-in, changes that will explicitly relax this contract. --------- Co-authored-by: Abhinav Gupta --- logger.go | 5 ++++- logger_test.go | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/logger.go b/logger.go index 2bebb87b3..9b45b07b0 100644 --- a/logger.go +++ b/logger.go @@ -173,7 +173,8 @@ func (log *Logger) WithOptions(opts ...Option) *Logger { } // With creates a child logger and adds structured context to it. Fields added -// to the child don't affect the parent, and vice versa. +// to the child don't affect the parent, and vice versa. Any fields that +// require evaluation (such as Objects) are evaluated upon invocation of With. func (log *Logger) With(fields ...Field) *Logger { if len(fields) == 0 { return log @@ -199,6 +200,8 @@ func (log *Logger) Check(lvl zapcore.Level, msg string) *zapcore.CheckedEntry { // Log logs a message at the specified level. The message includes any fields // passed at the log site, as well as any fields accumulated on the logger. +// Any Fields that require evaluation (such as Objects) are evaluated upon +// invocation of Log. func (log *Logger) Log(lvl zapcore.Level, msg string, fields ...Field) { if ce := log.check(lvl, msg); ce != nil { ce.Write(fields...) diff --git a/logger_test.go b/logger_test.go index d4af57512..8f0b01bb5 100644 --- a/logger_test.go +++ b/logger_test.go @@ -140,6 +140,43 @@ func TestLoggerWith(t *testing.T) { }) } +func TestLoggerWithCaptures(t *testing.T) { + enc := zapcore.NewJSONEncoder(zapcore.EncoderConfig{ + MessageKey: "m", + }) + + var bs ztest.Buffer + logger := New(zapcore.NewCore(enc, &bs, DebugLevel)) + + x := 0 + arr := zapcore.ArrayMarshalerFunc(func(enc zapcore.ArrayEncoder) error { + enc.AppendInt(x) + return nil + }) + + // Demonstrate the arguments are captured when With() and Info() are invoked. + logger = logger.With(Array("a", arr)) + x = 1 + logger.Info("hello", Array("b", arr)) + x = 2 + logger = logger.With(Array("c", arr)) + logger.Info("world") + + if lines := bs.Lines(); assert.Len(t, lines, 2) { + assert.JSONEq(t, `{ + "m": "hello", + "a": [0], + "b": [1] + }`, lines[0], "Unexpected output from first log.") + + assert.JSONEq(t, `{ + "m": "world", + "a": [0], + "c": [2] + }`, lines[1], "Unexpected output from second log.") + } +} + func TestLoggerLogPanic(t *testing.T) { for _, tt := range []struct { do func(*Logger) From d2aeb27df8fec9eb22c57548e8b5868e35cd0dd0 Mon Sep 17 00:00:00 2001 From: Jeremy Quirke Date: Fri, 18 Aug 2023 16:43:50 -0700 Subject: [PATCH 5/5] Strengthen TestLoggerWith to handle chained Withs (#1328) Strengthen the cross talk tests to cover the case of chained Withs, creating three levels of logger hierarchy. This will be used to harden against possible regressions in a proposed optimisation in https://github.com/uber-go/zap/pull/1319. --- logger_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/logger_test.go b/logger_test.go index 8f0b01bb5..5b1d77b97 100644 --- a/logger_test.go +++ b/logger_test.go @@ -130,11 +130,13 @@ func TestLoggerWith(t *testing.T) { // shouldn't stomp on each other's fields or affect the parent's fields. logger.With(String("one", "two")).Info("") logger.With(String("three", "four")).Info("") + logger.With(String("five", "six")).With(String("seven", "eight")).Info("") logger.Info("") assert.Equal(t, []observer.LoggedEntry{ {Context: []Field{Int("foo", 42), String("one", "two")}}, {Context: []Field{Int("foo", 42), String("three", "four")}}, + {Context: []Field{Int("foo", 42), String("five", "six"), String("seven", "eight")}}, {Context: []Field{Int("foo", 42)}}, }, logs.AllUntimed(), "Unexpected cross-talk between child loggers.") })