refactor: replace zap with slog #6466

knqyf263 · 2024-04-07T21:12:15Z

Description

Replace zap with log/slog.

Related issues

refactor: migrate from zap logger to log/slog #6451

Related PRs

refactor: use slog trivy-kubernetes#326

Checklist

I've read the guidelines for contributing to this repository.
I've followed the conventions in the PR title.
I've added tests that prove my fix is effective or that my feature works.
I've updated the documentation with the relevant information (if needed).
I've added usage information (if the PR introduces new options)
I've included a "before" and "after" example to the description (if the PR is a user interface change).

Signed-off-by: knqyf263 <[email protected]>

knqyf263 · 2024-04-08T14:30:21Z

@simar7 @nikpivkin @chen-keinan The changes are extensive, so we will probably find something wrong. I'd like as many people as possible to review it.

pkg/cloud/aws/scanner/scanner.go

pkg/dependency/parser/java/jar/sonatype/log.go

pkg/dependency/parser/java/pom/parse.go

chen-keinan · 2024-04-09T11:43:23Z

pkg/k8s/commands/cluster.go

 	"github.com/aquasecurity/trivy/pkg/types"
 )

 // clusterRun runs scan on kubernetes cluster
 func clusterRun(ctx context.Context, opts flag.Options, cluster k8s.Cluster) error {
+	// TODO: replace with log.Logger
+	logger, _ := zap.NewProduction()


trivy-kubernetes v0.6.6 is available and include PR for slog you made

Great, but it seems to include a breaking change.
aquasecurity/trivy-kubernetes#325

Therefore, I'll not remove zap from trivy-kubernetes in this PR. After you apply the above change in Trivy, I'll remove them.

chen-keinan · 2024-04-09T12:14:04Z

pkg/licensing/classifier.go

@@ -25,7 +25,7 @@ func initGoogleClassifier() error {
 	// This loading is expensive and should be called only when the license classification is needed.
 	var err error
 	classifierOnce.Do(func() {
-		log.Logger.Debug("Loading the default license classifier...")
+		log.Debug("Loading the default license classifier...")


general nit comment, I see we have many logging some are fully lower-case and some are capital first letter.
it will be great to have one convension

Suggested change

log.Debug("Loading the default license classifier...")

log.Debug("loading the default license classifier...")

We try to follow this convention.
https://google.github.io/styleguide/go/decisions.html#error-strings

Error strings should not be capitalized (unless beginning with an exported name, a proper noun or an acronym) and should not end with punctuation. This is because error strings usually appear within other context before being printed to the user.

On the other hand, the style for the full displayed message (logging, test failure, API response, or other UI) depends, but should typically be capitalized.

Therefore, the log message should be capitalized. If you find a log message in lowercase, please fix it.

Co-authored-by: Nikita Pivkin <[email protected]>

Signed-off-by: knqyf263 <[email protected]>

pkg/fanal/analyzer/analyzer.go

Co-authored-by: simar7 <[email protected]>

Signed-off-by: knqyf263 <[email protected]>

nikpivkin

LGTM

simar7 · 2024-04-11T05:53:40Z

I'm still going through the PR as it's very big so I'll ask something that might already be taken care of:

I wonder if we should allow (or disallow) a mix of strong and loose typed key/value pairs in our logs. For example if we enforce it via something like .LogAttr() it would help prevent getting in the situation where by accident we miss adding attributes.

Alternatively to solve the missed attribute problem, we could run a linter check to highlight such cases. On that note, maybe we should include sloglint (part of golangci-lint)?

knqyf263 · 2024-04-11T06:12:54Z

For example if we enforce it via something like .LogAttr() it would help prevent getting in the situation where by accident we miss adding attributes.

Yes, but there are no leveled attr functions like WarnAttr and ErrorAttr, right?

Alternatively to solve the missed attribute problem, we could run a linter check to highlight such cases. On that note, maybe we should include sloglint (part of golangci-lint)?

Sounds nice! I'll enable it. Thanks.

knqyf263 · 2024-04-11T06:26:31Z

I've not looked into this error, but the linter seems unstable.

ERRO [runner] Panic: sloglint: package "analyzer" (isInitialPkg: true, needAnalyzeSource: true): runtime error: invalid memory address or nil pointer dereference: goroutine 615329 [running]:
runtime/debug.Stack()
        runtime/debug/stack.go:24 +0x64
github.com/golangci/golangci-lint/pkg/goanalysis.(*action).analyzeSafe.func1()
        github.com/golangci/golangci-lint/pkg/goanalysis/runner_action.go:108 +0x214
panic({0x101276d80?, 0x101be17b0?})
        runtime/panic.go:770 +0x124
go/types.writeFuncName(0x140c85d1f20?, 0x14000600908?, 0x1405477a970?)
        go/types/object.go:601 +0x18
go/types.(*Func).FullName(0x0)
        go/types/object.go:390 +0x38
go-simpler.org/sloglint.badKeyNames(0x140c85d1f20, 0x1013d7be8, {0x0?, 0x18?, 0x1012a28c0?}, {0x1405477a970, 0x1, 0x14082436620?})
        go-simpler.org/[email protected]/sloglint.go:321 +0x32c
go-simpler.org/sloglint.run.func1({0x1013e3df0?, 0x1404a546f40})
        go-simpler.org/[email protected]/sloglint.go:225 +0x9e4
golang.org/x/tools/go/ast/inspector.(*Inspector).Preorder(0x1407f1e9050, {0x140088e0c10?, 0x101c0eb60?, 0x10040a32c?}, 0x14003a05c20)
        golang.org/x/[email protected]/go/ast/inspector/inspector.go:82 +0x90
go-simpler.org/sloglint.run(0x1404e440270, 0x14000a670c0)
        go-simpler.org/[email protected]/sloglint.go:148 +0xa0
go-simpler.org/sloglint.New.func1(0x101269240?)
        go-simpler.org/[email protected]/sloglint.go:61 +0x194
github.com/golangci/golangci-lint/pkg/goanalysis.(*action).analyze(0x14005c8ae90)
        github.com/golangci/golangci-lint/pkg/goanalysis/runner_action.go:190 +0x8c4
github.com/golangci/golangci-lint/pkg/goanalysis.(*action).analyzeSafe.func2()
        github.com/golangci/golangci-lint/pkg/goanalysis/runner_action.go:112 +0x20
github.com/golangci/golangci-lint/pkg/timeutils.(*Stopwatch).TrackStage(0x1400360d090, {0x100ee7877, 0x8}, 0x140088e0f30)
        github.com/golangci/golangci-lint/pkg/timeutils/stopwatch.go:111 +0x44
github.com/golangci/golangci-lint/pkg/goanalysis.(*action).analyzeSafe(0x3?)
        github.com/golangci/golangci-lint/pkg/goanalysis/runner_action.go:111 +0x78
github.com/golangci/golangci-lint/pkg/goanalysis.(*loadingPackage).analyze.func2(0x14005c8ae90)
        github.com/golangci/golangci-lint/pkg/goanalysis/runner_loadingpackage.go:80 +0xb0
created by github.com/golangci/golangci-lint/pkg/goanalysis.(*loadingPackage).analyze in goroutine 1343
        github.com/golangci/golangci-lint/pkg/goanalysis/runner_loadingpackage.go:75 +0x184
WARN [runner] Can't run linter goanalysis_metalinter: goanalysis_metalinter: sloglint: package "analyzer" (isInitialPkg: true, needAnalyzeSource: true): runtime error: invalid memory address or nil pointer dereference
ERRO Running error: can't run linter goanalysis_metalinter
goanalysis_metalinter: sloglint: package "analyzer" (isInitialPkg: true, needAnalyzeSource: true): runtime error: invalid memory address or nil pointer dereference
Error: running "golangci-lint run --timeout 5m --fix" failed with exit code 3

knqyf263 · 2024-04-11T07:25:01Z

I'll open another PR for sloglint. Wdyt? @simar7

simar7 · 2024-04-11T15:17:36Z

I'll open another PR for sloglint. Wdyt? @simar7

Yeah that's fine!

knqyf263 · 2024-04-11T18:58:59Z

Thanks @nikpivkin
go-simpler/sloglint#35

Signed-off-by: knqyf263 <[email protected]> Co-authored-by: Nikita Pivkin <[email protected]> Co-authored-by: simar7 <[email protected]>

- Fix registry version aquasecurity/trivy#6219; \n- Fix replace zap with slog aquasecurity/trivy#6466; \n - The fix with slog used a zap to slog bridge (official from zap, but exp). It didn't have a license file, so I hardcoded a commit version that had; \n- Adopt opts.Align() to validate options object;

* Bump trivy to v0.49.1 * Bump trivy to v0.51.4 - Fix registry version aquasecurity/trivy#6219; - Fix replace zap with slog aquasecurity/trivy#6466; - The fix with slog used a zap to slog bridge (official from zap, but exp). It didn't have a license file, so I hardcoded a commit version that had; - Adopt opts.Align() to validate options object; * Bump trivy to v0.52.2 * Temp change the workflow trigger to test changes * Free up space on runner * Bump trivy to v0.53.0 - Fix go clear cache aquasecurity/trivy#7010 * Bump trivy to v0.54.1 - Fix --vuln-type flag renamed into --pkg-types aquasecurity/trivy#7104; - Adopt package relationships aquasecurity/trivy#7237 * Rollback CI run on target * Clean 'scan cache clean' code and add timeout to it

knqyf263 self-assigned this Apr 7, 2024

knqyf263 force-pushed the slog branch 3 times, most recently from a6288d5 to 295a6e9 Compare April 8, 2024 08:16

refactor: replace zap with slog

b773f1c

Signed-off-by: knqyf263 <[email protected]>

knqyf263 force-pushed the slog branch from 295a6e9 to 6f2a18f Compare April 8, 2024 13:08

Merge branch 'main' into slog

e8e8e0e

knqyf263 force-pushed the slog branch from 6f2a18f to e8e8e0e Compare April 8, 2024 13:23

knqyf263 marked this pull request as ready for review April 8, 2024 14:20

knqyf263 requested review from chen-keinan, simar7, nikpivkin and DmitriyLewen as code owners April 8, 2024 14:20

nikpivkin reviewed Apr 9, 2024

View reviewed changes

pkg/cloud/aws/scanner/scanner.go Outdated Show resolved Hide resolved

nikpivkin reviewed Apr 9, 2024

View reviewed changes

pkg/dependency/parser/java/jar/sonatype/log.go Outdated Show resolved Hide resolved

nikpivkin reviewed Apr 9, 2024

View reviewed changes

pkg/dependency/parser/java/pom/parse.go Outdated Show resolved Hide resolved

nikpivkin reviewed Apr 9, 2024

View reviewed changes

pkg/dependency/parser/java/pom/parse.go Outdated Show resolved Hide resolved

chen-keinan reviewed Apr 9, 2024

View reviewed changes

knqyf263 and others added 4 commits April 9, 2024 21:58

Update pkg/dependency/parser/java/pom/parse.go

a32b00e

Co-authored-by: Nikita Pivkin <[email protected]>

Update pkg/dependency/parser/java/pom/parse.go

bb444c7

Co-authored-by: Nikita Pivkin <[email protected]>

refactor: define a custom handler

192c8bc

Signed-off-by: knqyf263 <[email protected]>

fix: use a prefixed logger

9342836

Signed-off-by: knqyf263 <[email protected]>

knqyf263 requested review from chen-keinan and nikpivkin April 9, 2024 19:58

simar7 reviewed Apr 11, 2024

View reviewed changes

pkg/fanal/analyzer/analyzer.go Outdated Show resolved Hide resolved

simar7 reviewed Apr 11, 2024

View reviewed changes

pkg/fanal/analyzer/analyzer.go Outdated Show resolved Hide resolved

knqyf263 and others added 2 commits April 11, 2024 09:34

Update pkg/fanal/analyzer/analyzer.go

e127396

Co-authored-by: simar7 <[email protected]>

fix: log message

fdb559f

Signed-off-by: knqyf263 <[email protected]>

nikpivkin approved these changes Apr 11, 2024

View reviewed changes

simar7 self-requested a review April 11, 2024 15:17

simar7 approved these changes Apr 11, 2024

View reviewed changes

knqyf263 added this pull request to the merge queue Apr 11, 2024

Merged via the queue into aquasecurity:main with commit 94d6e8c Apr 11, 2024
12 checks passed

knqyf263 deleted the slog branch April 11, 2024 19:23

fl0pp5 pushed a commit to altlinux/trivy that referenced this pull request May 6, 2024

refactor: replace zap with slog (aquasecurity#6466)

acffb25

Signed-off-by: knqyf263 <[email protected]> Co-authored-by: Nikita Pivkin <[email protected]> Co-authored-by: simar7 <[email protected]>

romulets mentioned this pull request Aug 7, 2024

Bump trivy to v0.54.1 and fix breaking changes elastic/cloudbeat#2399

Merged

romulets mentioned this pull request Aug 13, 2024

Bump Trivy to v0.54.1 elastic/cloudbeat#2427

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: replace zap with slog #6466

refactor: replace zap with slog #6466

knqyf263 commented Apr 7, 2024 •

edited

Loading

knqyf263 commented Apr 8, 2024

chen-keinan Apr 9, 2024

knqyf263 Apr 9, 2024

chen-keinan Apr 9, 2024

knqyf263 Apr 9, 2024

knqyf263 Apr 9, 2024

nikpivkin left a comment

simar7 commented Apr 11, 2024

knqyf263 commented Apr 11, 2024

knqyf263 commented Apr 11, 2024 •

edited

Loading

knqyf263 commented Apr 11, 2024

simar7 commented Apr 11, 2024

knqyf263 commented Apr 11, 2024

	log.Debug("Loading the default license classifier...")
	log.Debug("loading the default license classifier...")

refactor: replace zap with slog #6466

refactor: replace zap with slog #6466

Conversation

knqyf263 commented Apr 7, 2024 • edited Loading

Description

Related issues

Related PRs

Checklist

knqyf263 commented Apr 8, 2024

chen-keinan Apr 9, 2024

Choose a reason for hiding this comment

knqyf263 Apr 9, 2024

Choose a reason for hiding this comment

chen-keinan Apr 9, 2024

Choose a reason for hiding this comment

knqyf263 Apr 9, 2024

Choose a reason for hiding this comment

knqyf263 Apr 9, 2024

Choose a reason for hiding this comment

nikpivkin left a comment

Choose a reason for hiding this comment

simar7 commented Apr 11, 2024

knqyf263 commented Apr 11, 2024

knqyf263 commented Apr 11, 2024 • edited Loading

knqyf263 commented Apr 11, 2024

simar7 commented Apr 11, 2024

knqyf263 commented Apr 11, 2024

knqyf263 commented Apr 7, 2024 •

edited

Loading

knqyf263 commented Apr 11, 2024 •

edited

Loading