-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: logger initialization before flags parsing #7372
fix: logger initialization before flags parsing #7372
Conversation
pkg/commands/app.go
Outdated
// Initialize the logger for `flag` and `app` packages. | ||
// To display logs from these packages in Trivy format. | ||
// We will set the `debug` and `disable` format after parsing the `--debug` and `--quiet` flags. | ||
log.InitLogger(false, false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I simply added init logger before parsing flags.
But there is a problem logs from app
and flags
packages:
if the --quiet
flag is used - we still show the logs or don't show debug logs:
➜ ./trivy -q image alpine -c config.yaml --vuln-type os
2024-08-22T16:17:11+06:00 WARN '--vuln-type' is deprecated. Use '--pkg-types' instead.
2024-08-22T16:17:11+06:00 INFO Loaded file_path="config.yaml"
alpine (alpine 3.20.2)
As another way - I added DeferredLogger
to save log messages and print them on command (in our case after logger initialization)
See 6ad1098
With this solution:
➜ ./trivy -q image alpine -c config.yaml --vuln-type os
alpine (alpine 3.20.2)
Total: 0 (UNKNOWN: 0, LOW: 0, MEDIUM: 0, HIGH: 0, CRITICAL: 0)
@knqyf263 Can you take a look, when you have time, please?
pkg/log/deferred.go
Outdated
}) | ||
} | ||
|
||
func (d *deferredLogger) PrintLogs() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add this into:
Lines 38 to 42 in 6ad1098
func InitLogger(debug, disable bool) { | |
level := lo.Ternary(debug, slog.LevelDebug, slog.LevelInfo) | |
out := lo.Ternary(disable, io.Discard, io.Writer(os.Stderr)) | |
slog.SetDefault(New(NewHandler(out, &Options{Level: level}))) | |
} |
But perhaps it won't be entirely obvious
Signed-off-by: knqyf263 <knqyf263@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of DeferredLogger, but the current implementation allows us to miss log emissions before logger initialization. For example, although LoadAll()
is called before parsing CLI flags, we forgot to replace it with log.DeferredLogger
.
Line 281 in 88ba460
m.logger.WarnContext(ctx, "Plugin load error", log.Err(err)) |
Thus, I think we should replace the default logger or handler.
Also, we need to handle WithPrefix
and WithAttrs
, but the current implementation doesn't work with them.
Line 65 in 88ba460
logger: log.WithPrefix("plugin"), |
To resolve these problems, I implemented DeferredHandler
. PTAL, @DmitriyLewen.
Nice upgrade of my idea! |
Description
We need to init logger before flags parsing to avoid wrong log messages format.
before:
after
Related issues
Checklist