-
Notifications
You must be signed in to change notification settings - Fork 209
standard log output and context handling #439
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
base: main
Are you sure you want to change the base?
Conversation
4ef83ee to
bbfb9c0
Compare
Continues the work from #417 in two import ways. First, this finalizes the move towards passing only a `context.Context` argument to the inner `InfoXXX.load()` methods instead of the old `pkg/option.Options` struct. Using a `context.Context` is more modern Go idiomatic and aligned with modern Go packages like `log/slog`. Each `InfoXXX.load()` method ensures a `context.Context` is created to store various options and context that is passed between modules in `ghw`. The function signature for the main module constructors like `pkg/cpu.CPU()` or `pkg/block.Block()` have all been changed from this: ```go func CPU(opt ...option.Option) (*Info, error) ``` to ```go func CPU(args ...any) (*Info, error) ``` which is a backwards compatible API change. We then examine each of the `args` arguments and set various keys on the `context.Context`. The `context.Context` is examined by other modules like `pkg/linuxpath` or `pkg/linuxdmi` instead of the prior `pkg/option.Options` struct. The second major part of this patch is the addition of log/output formatting to use `log/slog` and `log/slog.Handler` overrides to control the output of warning and debug log lines. The default log output in `ghw` only writes WARN-level messages to `stderr` in a simple `WARN: <msg>` log record format: ``` $ ghwc baseboard WARN: Unable to read board_serial: open /sys/class/dmi/id/board_serial: permission denied baseboard vendor=System76 version=thelio-mira-b4.1 product=Thelio Mira ``` You can control a number of log output options programmatically or by using environs variables. To change the log level `ghw` uses, set the `GHW_LOG_LEVEL` environs variable: ``` $ GHW_LOG_LEVEL=debug ghwc baseboard DEBUG: reading from "/sys/class/dmi/id/board_asset_tag" DEBUG: reading from "/sys/class/dmi/id/board_serial" WARN: Unable to read board_serial: open /sys/class/dmi/id/board_serial: permission denied DEBUG: reading from "/sys/class/dmi/id/board_vendor" DEBUG: reading from "/sys/class/dmi/id/board_version" DEBUG: reading from "/sys/class/dmi/id/board_name" baseboard vendor=System76 version=thelio-mira-b4.1 product=Thelio Mira ``` Changing `GHW_LOG_LEVEL` to `error` has the same effect of setting `GHW_DISABLE_WARNINGS`: ``` $ GHW_LOG_LEVEL=error ghwc baseboard baseboard vendor=System76 version=thelio-mira-b4.1 product=Thelio Mira ``` You can change the log level programmatically using the `WithLogLevel` modifier: ```go import ( "log/slog" "github.com/jaypipes/ghw" ) bb, err := ghw.Baseboard(ghw.WithLogLevel(slog.LevelDebug)) ``` To use the [logfmt][logfmt] standard log output format, set the `GHW_LOG_LOGFMT` envrions variable: ``` $ GHW_LOG_LOGFMT=1 ghwc baseboard time=2025-12-28T07:31:08.614-05:00 level=WARN msg="Unable to read board_serial: open /sys/class/dmi/id/board_serial: permission denied" baseboard vendor=System76 version=thelio-mira-b4.1 product=Thelio Mira ``` You can tell `ghw` to use `logfmt` standard output formatting using the `WithLogLogfmt` modifier: ```go import ( "log/slog" "github.com/jaypipes/ghw" ) bb, err := ghw.Baseboard(ghw.WithLogLogfmt()) ``` [logfmt]: https://www.cloudbees.com/blog/logfmt-a-log-format-thats-easy-to-read-and-write You can now programmatically override the logger that `ghw` uses with the `WithLogger` modifier. You pass in an instance of `slog.Logger`, like this example that shows how to use a simple logger with colored log output: ```go package main import ( "context" "encoding/json" "io" "log" "log/slog" "github.com/fatih/color" "github.com/jaypipes/ghw" ) type PrettyHandlerOptions struct { SlogOpts slog.HandlerOptions } type PrettyHandler struct { slog.Handler l *log.Logger } func (h *PrettyHandler) Handle(ctx context.Context, r slog.Record) error { level := r.Level.String() + ":" switch r.Level { case slog.LevelDebug: level = color.MagentaString(level) case slog.LevelInfo: level = color.BlueString(level) case slog.LevelWarn: level = color.YellowString(level) case slog.LevelError: level = color.RedString(level) } fields := make(map[string]interface{}, r.NumAttrs()) r.Attrs(func(a slog.Attr) bool { fields[a.Key] = a.Value.Any() return true }) b, err := json.MarshalIndent(fields, "", " ") if err != nil { return err } timeStr := r.Time.Format("[15:05:05.000]") msg := color.CyanString(r.Message) h.l.Println(timeStr, level, msg, color.WhiteString(string(b))) return nil } func NewPrettyHandler( out io.Writer, opts PrettyHandlerOptions, ) *PrettyHandler { h := &PrettyHandler{ Handler: slog.NewJSONHandler(out, &opts.SlogOpts), l: log.New(out, "", 0), } return h } func main() { opts := PrettyHandlerOptions{ SlogOpts: slog.HandlerOptions{ Level: slog.LevelDebug, }, } handler := NewPrettyHandler(os.Stdout, opts) logger := slog.New(handler) bb, err := ghw.Baseboard(ghw.WithLogger(logger)) if err != nil { logger.Error(err.String()) } fmt.Println(bb) } ``` Signed-off-by: Jay Pipes <[email protected]>
Replaces the SetTraceFunction() functionality in `pkg/snapshot` with the `pkg/context.Debug()` function which uses the standardized `log/slog` facility. Running `ghwc snapshot --debug` now: ``` > go run cmd/ghwc/main.go snapshot --debug DEBUG: processing block device "/sys/block/nvme0n1" DEBUG: link target for block device "/sys/block/nvme0n1" is "../devices/pci0000:00/0000:00:1a.0/0000:02:00.0/nvme/nvme0/nvme0n1" DEBUG: creating device directory /tmp/ghw-snapshot840812080/sys/devices/pci0000:00/0000:00:1a.0/0000:02:00.0/nvme/nvme0/nvme0n1 DEBUG: linking device directory /tmp/ghw-snapshot840812080/sys/block/nvme0n1 to ../devices/pci0000:00/0000:00:1a.0/0000:02:00.0/nvme/nvme0/nvme0n1 DEBUG: creating device directory "/tmp/ghw-snapshot840812080/sys/devices/pci0000:00/0000:00:1a.0/0000:02:00.0/nvme/nvme0/nvme0n1" from "/sys/devices/pci0000:00/0000:00:1a.0/0000:02:00.0/nvme/nvme0/nvme0n1" DEBUG: creating /tmp/ghw-snapshot840812080/sys/devices/pci0000:00/0000:00:1a.0/0000:02:00.0/nvme/nvme0/nvme0n1/alignment_offset DEBUG: creating /tmp/ghw-snapshot840812080/sys/devices/pci0000:00/0000:00:1a.0/0000:02:00.0/nvme/nvme0/nvme0n1/capability DEBUG: creating /tmp/ghw-snapshot840812080/sys/devices/pci0000:00/0000:00:1a.0/0000:02:00.0/nvme/nvme0/nvme0n1/csi ... ``` Signed-off-by: Jay Pipes <[email protected]>
bbfb9c0 to
c419aaa
Compare
|
Thanks for the followup! will review shortly! |
ffromani
left a comment
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.
Thanks @jaypipes ! this is another massive improvement.
I left comments inside for possible improvements, overall I'm largely in favor of this change.
The only somehow grey-ish area is the fact we are still changing the signature of public functions, belonging to sub-packages.
That made me think: perhaps we can refine our stance and ensure that only the top-level exported symbols (alias.go) are stable, and this could allow to save 90% of the pain from our users, and still allow us to evolve the codebase - and maybe tighten again the types again?
Perhaps we can explore this option?
| // DEPRECATED: Please use Option | ||
| type WithOption = option.Option | ||
|
|
||
| // DEPRECATED: Please use WithXXX functions. |
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.
This is a bit puzzling because 3 lines above we instruct users to NOT use a WithXXX function
| level = color.RedString(level) | ||
| } | ||
|
|
||
| fields := make(map[string]interface{}, r.NumAttrs()) |
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.
nit: map[string]any (please do NOT resubmit just for this)
| ctx := context.TODO() | ||
| ctx = ghwcontext.WithChroot(baseDir)(ctx) | ||
| paths := linuxpath.New(ctx) |
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.
in a followup PR we may want to try to add a test helper like kube did during the transition to contextual logger. I can play with this concept later on.
| // See the COPYING file in the root project directory for full text. | ||
| // | ||
|
|
||
| package context |
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'm thinking if we can avoid the name clash with the stdlib. I don't have good suggestions here.
| envKeyDisableTools = "GHW_DISABLE_TOOLS" | ||
| ) | ||
|
|
||
| type ContextKey string |
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.
do we want or need to export this? If exported, should it be ContextKey or just Key?
e.g.
ghwcontext.ContextKey
ghwcontext.Key
| ) | ||
|
|
||
| // ContextModifier sets some value on the context | ||
| type ContextModifier func(context.Context) context.Context |
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.
this needs to be exported indeed, but maybe just Modifier ?
|
|
||
| // FromArgs returns a context.Context populated with any old-style options or | ||
| // new-style arguments. | ||
| func FromArgs(args ...any) context.Context { |
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.
(here and everywhere else). I'm just a little off by the fact we need to use variadic args and the any type. But it's a necessary evil (and the best/only option to keep backward compat). The code is massively better with this change, so I think this is the sweet spot. In the future we will tighten the function signatures again I reckon.
| // See the COPYING file in the root project directory for full text. | ||
| // | ||
|
|
||
| package context |
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.
minorish: does this belong in the context package? do we want or need to add a dedicated log package?
I don't have strong opinions either way but I think is worth raising the point.
|
|
||
| // Warn outputs an WARN-level log message to the logger configured in the | ||
| // supplied context. | ||
| func Warn(ctx context.Context, format string, args ...any) { |
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.
on hindsight, looking at this (and other) function callsites, I'm leaning towards adding a log package somehow. Or whatever better name we can find. WDYT?
Continues the work from #417 in two important ways. First, this finalizes the move towards passing only a
context.Contextargument to the innerInfoXXX.load()methods instead of the oldpkg/option.Optionsstruct. Using acontext.Contextis more modern Go idiomatic and aligned with modern Go packages likelog/slog. EachInfoXXX.load()method ensures acontext.Contextis created to store various options and context that is passed between modules inghw. The function signature for the main module constructors likepkg/cpu.CPU()orpkg/block.Block()have all been changed from this:to
which is a backwards compatible API change. We then examine each of the
argsarguments and set various keys on thecontext.Context. Thecontext.Contextis examined by other modules likepkg/linuxpathorpkg/linuxdmiinstead of the priorpkg/option.Optionsstruct.The second major part of this patch is the addition of log/output formatting to use
log/slogandlog/slog.Handleroverrides to control the output of warning and debug log lines.The default log output in
ghwonly writes WARN-level messages tostderrin a simpleWARN: <msg>log record format:You can control a number of log output options programmatically or by using environs variables.
To change the log level
ghwuses, set theGHW_LOG_LEVELenvirons variable:Changing
GHW_LOG_LEVELtoerrorhas the same effect of settingGHW_DISABLE_WARNINGS:You can change the log level programmatically using the
WithLogLevelmodifier:To use the logfmt standard log output format, set the
GHW_LOG_LOGFMTenvrions variable:You can tell
ghwto uselogfmtstandard output formatting using theWithLogLogfmtmodifier:You can now programmatically override the logger that
ghwuses with theWithLoggermodifier. You pass in an instance ofslog.Logger, like this example that shows how to use a simple logger with colored log output: