KimMachineGun
Please consider logging by default
Hey, I just started using this package. I like it! But, I use it alongside automaxprocs, and I noticed automaxprocs always logs when it changes the GOMAXPROCS setting, whereas your package only logs when debug mode is enabled. I think since most people will use both packages, it would be nice if you also logged by default.
Hi @mbyio! Thank you for your proposal.
I'm planning to make some changes to logging in v0.3.0 (using a library like slog for structured logging).
It won't log everything by default, but I'm considering increasing the log level of some important logs appropriately to be recorded by default.
It can accept a Logger interface instead of a concrete type, so we can pass in our own logger instance.
I think I just want it to log what automaxprocs logs, which is whether or not it changed the memlimit, and, if it did, what it set the memlimit to.
Would something like a automemlimit.Modifications() (map[string]string, error) work?
where the map would contain a key-value pair mapping from change-key to change-value.
That way we could implement our own logging, or other means to display the changes
any call to automemlimit.Xyz would add or update a modification, and we can print them (or any error) after calling/importing it.
@KimMachineGun Can I use loggers like klog with this package?
@thapabishwa-plerionaut
I don't know much about the klog package. However, I'm currently working on migrating the logging package to log/slog (announced in Go 1.21). So, after that, all logs will be logged in a structured manner with different levels.
AFAIK log/slog is interoperable with logr, if that's the case then it should also work with klog.
@KimMachineGun Do you have any eta on the release?
@thapabishwa-plerionaut I am still determining the exact plan, but it is likely to be released either in late November or early December.
@KimMachineGun has slog compatibility been released with 0.4.0 version?
Looks like it hasn't been done just yet. I've opened PR #12 to address the slog interoperability. Could you please take a look and let me know about it @KimMachineGun ? Thanks
@thapabishwa Sorry for being late. The structured logging feature was in private testing, and I just pushed it to the v0.5.0-pre branch. (Note: this is not the final version of v0.5.0) Please take a look at the code and feel free to share your feedback.
+ I appreciate your PR, but at this time, I prefer not to add more external dependencies to this library. I think log/slog can handle most cases. If you have any thoughts on this, please do let me know.
Sure. Let me take a look at if the recent changes. I will let you know if works for me or not.
I can confirm that it works fine with klog. One caveat is that I had to upgrade to go 1.21.0
slogger := slog.New(slogr.NewSlogHandler(klog.Log.WithName("memlimit")))
// setup memlimit
memlimit.SetGoMemLimitWithOpts(memlimit.WithEnv(), memlimit.WithLogger(
slogger,
))
Finally, Any visibility on 0.5.0 release?
@thapabishwa-plerionaut Thank you for testing! I have just released the pre-release version, and I will release v0.5.0 after a few days of testing.
I understand your concerns about updating the Go version. However, I believe that Go 1.21 was released over three months ago, and thanks to their compatibility promise, most programs should work well in Go 1.21 (in fact, they may work even better).
+ It may be possible to provide a way to configure the logger for versions prior to 1.21, but doing so would result in additional maintenance points. In my opinion, the effort required to maintain multiple versions outweighs the benefits that it provides. 😢
@thapabishwa-plerionaut Sorry, the testing was done, but it had a tiny issue with its dependency. (https://github.com/containerd/cgroups/pull/321.) Now, the issue is resolved but not released yet. And I'm waiting for the release.
If it won't be released soon, I'll tell you about the updated release plan for v0.5.0.
Hey @KimMachineGun , any reason why 0.5.0.pre.4 droppped support for slog?
The issue with slog is it's not supported in all "supported" versions of Go. Since a lot of projects support what Go officially supports for compilers (Latest - 1), slog would force everyone to only the latest Go version.
When Go 1.22, slog can be added, since it's supported in Go >= 1.21.
Personally, I would prefer supporting logging via creating a compatible interface. This would allow whatever logging the downstream project wants to use, rather than locking to a specific logger.
For example:
- https://pkg.go.dev/go.uber.org/[email protected]/maxprocs#Logger
- https://pkg.go.dev/github.com/gosnmp/gosnmp#Logger
@thapabishwa-plerionaut First of all, I feel sorry about it.
As @SuperQ mentioned, I decided to delay the release of log/slog until the next Go release. Go 1.22 will be released in February, so there is about a month left to release.
But I need to figure out the interface-type logger. I think just the log/slog package would satisfy almost every case since there are already a lot of ecosystems built on top of log/slog.Handler for adapting third-party loggers. IMHO, only log/slog could be a logger that everyone agrees on like gofmt.
Gofmt's style is no one's favorite, yet gofmt is everyone's favorite.
Yup, I'm really looking forward to log/slog, but for ecosystem compatibility, some delay is prudent.
Should we reconsider changes introduced by PR #12?
gologr has interoperability/implementation with various popular loggers(non-exhaustive list), such as:
- slog
- github.com/google/glog
- k8s.io/klog (for Kubernetes)
- go.uber.org/zap:
- log (the Go standard library logger)
- github.com/sirupsen/logrus
- github.com/wojas/genericr
- logfmt (Heroku style logging)
- github.com/rs/zerolog
- github.com/go-kit/log
- bytes.Buffer (writing to a buffer)
Source: gologr implementations Source: slog interoperabilty
I think gologr effectively addresses the issues previously discussed here:
- end users should be able choose their preferred Logger instance
- end users should be able to do it without having to upgrade to Go >= 1.21.
LMK your thoughts, @KimMachineGun.
@thapabishwa-plerionaut Well... I am unlikely to reconsider gologr for now because of these reasons.
end users should be able choose their preferred Logger instance
I totally agree with you, but I don't think the library would be the best choice. As I mentioned earlier, log/slog would be enough.
end users should be able to do it without having to upgrade to Go >= 1.21.
This seems reasonable for now, but after the Go 1.22 release, every Go program should use at least Go 1.21, which will be the lowest version maintained by the Go team. So, if we release this feature after the Go 1.22 release, it won't be problematic. (The Go team maintains only the last two versions: Go 1.20/1.21 for now and Go 1.21/1.22 after the next release.)
cc. @SuperQ (FYI. could be related to your proposal https://github.com/KimMachineGun/automemlimit/issues/14)
@thapabishwa-plerionaut Well... I am unlikely to reconsider gologr for now because of these reasons.
end users should be able choose their preferred Logger instance
I totally agree with you, but I don't think the library would be the best choice. As I mentioned earlier,
log/slogwould be enough.end users should be able to do it without having to upgrade to Go >= 1.21.
This seems reasonable for now, but after the Go 1.22 release, every Go program should use at least Go 1.21, which will be the lowest version maintained by the Go team. So, if we release this feature after the Go 1.22 release, it won't be problematic. (The Go team maintains only the last two versions: Go 1.20/1.21 for now and Go 1.21/1.22 after the next release.)
cc. @SuperQ (FYI. could be related to your proposal #14)
Great. Looking forward to slog
My apologies for the late response, I've been quite busy lately.
I've just finished a preliminary version of this feature. As usual, we should have a few weeks for testing, possibly aiming for a late March release.
Your feedback on this pre-release version would be greatly appreciated, so feel free to leave any comments.
https://github.com/KimMachineGun/automemlimit/releases/tag/v0.6.0-pre.1
Finally, v0.6.0 has been released! Please open an issue if you have any suggestions or feedback about logging. https://github.com/KimMachineGun/automemlimit/releases/tag/v0.6.0