cgroup logging with slog #269
Conversation
| } | ||
|
|
||
| // For cgroups, it should clone the default logger and append an additional attribute (i.e. name, id, etc.) | ||
| func LoadCgroupLogger(args ...any) *slog.Logger { |
There was a problem hiding this comment.
does this wrapper around .With do anything interesting? Why not just directly call .With when needed?
|
|
||
| // Default logger for the entire system (Any call from log will immediately be written with a JSON handler) | ||
| // TODO: Only format it as JSON for cgroups but any other systems calling log.Printf should just get a regular log output | ||
| var defaultSystemLogger = slog.New(slog.NewJSONHandler(os.Stdout, nil)) |
There was a problem hiding this comment.
The config should have an option about whether to use JSON our Text formatting of output.
There was a problem hiding this comment.
I like how #203 was handling the config. Can we do something similar?
There was a problem hiding this comment.
We need to handle different logging levels for different subsystems. How will that work out? It seems like you could create different handlers (since you specify the level when you create the handler), but will we have multiple locks and jumbled output then?
| "os" | ||
| ) | ||
|
|
||
| // Default logger for the entire system (Any call from log will immediately be written with a JSON handler) |
There was a problem hiding this comment.
You have some more details comments about logging and locking in the PR itself. I think we should do most of the explanation in the actual comments. Please include links/quotes to documentation where appropriate.
| var defaultSystemLogger = slog.New(slog.NewJSONHandler(os.Stdout, nil)) | ||
|
|
||
| // Set the defaultSystemLogger as default logger when common is initialized | ||
| func init() { |
There was a problem hiding this comment.
Probably need to do this later, after the config is loaded.
| @@ -33,11 +33,12 @@ func NewCgroupPool(name string) (*CgroupPool, error) { | |||
| recycled: make(chan *CgroupImpl, CGROUP_RESERVE), | |||
| quit: make(chan chan bool), | |||
| nextID: 0, | |||
| log: common.LoadCgroupLogger("cg-pool", path.Base(path.Dir(common.Conf.Worker_dir))+"-"+name), | |||
There was a problem hiding this comment.
We'll usually only have one cgroup pool, so I don't think it's worth including this path in every log line.
| @@ -59,30 +60,24 @@ func (pool *CgroupPool) NewCgroup() Cgroup { | |||
| cg := &CgroupImpl{ | |||
| name: fmt.Sprintf("cg-%d", pool.nextID), | |||
| pool: pool, | |||
| log: pool.log.With("cg", fmt.Sprintf("cg-%d", pool.nextID)), | |||
There was a problem hiding this comment.
Let's avoid copying code for the cg name in case we choose a different naming scheme later. Better to store the name in a separate variable, then use it in both places.
|
Thanks for taking this on! This is attempting to do the same as a prior PR that has been abandoned: #203. For that one, we were not sure we had a good strategy to avoid jumbled output, and there was not further progress. The locking gets tricky when both log and slog are used, and when multiple levels in slog are used (need to make sure loggers with different handlers wrapping the same std output actually coordinate). |
Note: On the log side, the default logger that’s named “std” and functions like Printf is utilizing the output method of std. On the slog side, the default logger is an object called “defaultLogger” which is just a slog logger whose handler is std.output. When we create a new slog logger with a JSONHandler and pass it through slog.SetDefault, “defaultLogger” now points to that new slog logger. In addition, it changes the “out” field of “std” and it now wraps our new JSONHandler.
When log.Printf is called, it first uses its own lock before "writing" the output. When "writing" the output, it uses the JSONHandler's "Write" method that uses its own lock before writing the actual output in JSON format. Since this is a nested lock with two different locks, it should 1) not jumble the output and 2) not cause a deadlock.
Modified cgroups to use slog with a JSON handler and have level-specific attributes for the logs. For example, at the pool level it should log "cg-pool: *pool_name", and at the cg-level it should log "cg-pool: *pool-name, cg: *cg-name".
Downside, every call to log.Printf will be a JSON format, so currently working on a way so that calls to log.Printf would just show regular log but cgroup logs will show-up as JSON format.