Replace log.Println with slog for structured logging#1602
Replace log.Println with slog for structured logging#1602hqpho wants to merge 1 commit intodatacommonsorg:masterfrom
Conversation
Replaced all instances of `log.Println` with the `slog` structured logging library for info and warning level log messages. - Introduced a new `internal/log/log.go` file to centralize logger setup. - Updated `cmd/main.go`, `internal/server/v2/facet/contained_in.go`, `internal/server/v2/observation/contained_in.go`, `test/http_memprof/http_memprof.go`, and `test/setup.go` to use `slog.Info` and `slog.Warn` instead of `log.Println`. - Configured the logger to include source file and line number information.
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a great step towards modernizing logging by adopting the slog library for structured logging. The changes are well-implemented across the codebase, and centralizing the logger configuration is a good practice. My review focuses on a few areas for improvement within the test utility files: one critical fix for incorrect error logging, and a few suggestions to enhance the context of the new structured logs by including relevant variables, which will improve debuggability.
| if os.WriteFile( | ||
| path.Join(root, fname), bytes.TrimRight(buf.Bytes(), "\n"), 0644) != nil { | ||
| log.Printf("could not write golden files to %s", fname) | ||
| slog.Warn("could not write golden files to", "err", err) |
There was a problem hiding this comment.
This log is misleading. It's logging the err variable from the encoder.Encode call on line 300, not the error from os.WriteFile. The error from os.WriteFile is not being captured. The surrounding if block should be changed to capture and log the correct error, for example:
if err := os.WriteFile(
path.Join(root, fname), bytes.TrimRight(buf.Bytes(), "\n"), 0644); err != nil {
slog.Warn("could not write golden file", "file", fname, "err", err)
}| data, err := marshaller.Marshal(resp) | ||
| if err != nil { | ||
| log.Printf("marshaller.Marshal(%s) = %s", fname, err) | ||
| slog.Warn("marshaller.Marshal()", "err", err) |
There was a problem hiding this comment.
The new log message loses the context of which file (fname) failed. It's good practice to include such contextual information in structured logs for easier debugging.
| slog.Warn("marshaller.Marshal()", "err", err) | |
| slog.Warn("marshaller.Marshal() failed", "file", fname, "err", err) |
| jsonByte, err := json.MarshalIndent(rm, "", " ") | ||
| if err != nil { | ||
| log.Printf("json.MarshalIndent(%s) = %s", fname, err) | ||
| slog.Warn("json.MarshalIndent()", "err", err) |
There was a problem hiding this comment.
Similar to a previous comment, the filename context (fname) is lost in this log message. Including it would make debugging easier.
| slog.Warn("json.MarshalIndent()", "err", err) | |
| slog.Warn("json.MarshalIndent() failed", "file", fname, "err", err) |
| err = os.WriteFile(path.Join(root, fname), jsonByte, 0644) | ||
| if err != nil { | ||
| log.Printf("os.WriteFile(%s) = %s", fname, err) | ||
| slog.Warn("os.WriteFile", "err", err) |
There was a problem hiding this comment.
The filename context (fname) is lost here as well. Please add it to the structured log to provide more context for debugging.
| slog.Warn("os.WriteFile", "err", err) | |
| slog.Warn("os.WriteFile failed", "file", fname, "err", err) |
1 participant
Replaced all instances of
log.Printlnwith theslogstructured logging library for info and warning level log messages.Reasons to do this:
slogin particular is recommended for GolangSteps:
internal/log/log.gofile to centralize logger setup.cmd/main.go,internal/server/v2/facet/contained_in.go,internal/server/v2/observation/contained_in.go,test/http_memprof/http_memprof.go, andtest/setup.goto useslog.Infoandslog.Warninstead oflog.Println.