cron
cron copied to clipboard
Add external logger support
Hello,
This PR adds support for external loggers via io.Writer
. Sample code of it working is below:
package main
import (
"fmt"
"log"
"time"
"github.com/robfig/cron/v3"
"go.uber.org/zap"
)
func main() {
zapLogger, _ := zap.NewDevelopment()
stdOutLogger := zap.NewStdLog(zapLogger)
zW := stdOutLogger.Writer()
el := cron.NewExternalLogger(zW)
c := cron.New(
cron.WithChain(cron.SkipIfStillRunning(el)),
cron.WithSeconds(),
)
if _, err := c.AddFunc("* * * * * *", testJob); err != nil {
log.Fatal(err)
}
c.Start()
time.Sleep(1 * time.Minute)
}
func testJob() {
time.Sleep(3 * time.Second)
fmt.Println("job")
}
Should output something like:
2023-10-26T14:46:27.000-0500 INFO cron/logger.go:76 cron: /home/.../cron/logger.go:76: skip
2023-10-26T14:46:28.000-0500 INFO cron/logger.go:76 cron: /home/.../cron/logger.go:76: skip
2023-10-26T14:46:29.000-0500 INFO cron/logger.go:76 cron: /home/.../cron/logger.go:76: skip
job
2023-10-26T14:46:31.000-0500 INFO cron/logger.go:76 cron: /home/.../cron/logger.go:76: skip
2023-10-26T14:46:32.000-0500 INFO cron/logger.go:76 cron: /home/.../cron/logger.go:76: skip
job
2023-10-26T14:46:33.000-0500 INFO cron/logger.go:76 cron: /home/.../cron/logger.go:76: skip
Hello,
this seems to me an adapter for the zap
logger into the required cron.Logger
interface. Even though the new method you're proposing takes a generic io.Writer
as a parameter, I see this as very use-case-specific. An adapter is defined depending on the specific adaptee structure and IMHO it should not be provided by the package itself. I don't see the need for additional external logger support since (external?) logging is already supported, it's more about aligning APIs.
The example uses zap
, but any other logger that supports io.Writer
should also work. While it is true that cron
supports its own logger, it doesnt support the native standard library logging interface (which this adds support for). The problem with the current implementation is that a project may incorporate in robfig/cron
and have their own logging library (logrus
/zap
), but because robfig/cron
doesnt support unifying the logs the output is drastically different than what would be expected if the end user is logging centrally. For example, the level is different, the timestamps could be different, the output could be in a different format (json); all these are things that outside loggers handle.
Logging libraries provide a way to get an io.Writer
out of them so that third party modules (robfig/cron
) write to the io.Writer
are unified around whatever logger is being used. For example, a list of go loggers that support this:
- logrus: https://pkg.go.dev/github.com/sirupsen/Logrus#readme-logger-as-an-io-writer
- zap: https://pkg.go.dev/go.uber.org/zap#NewStdLog
- slog: https://pkg.go.dev/golang.org/x/exp/slog#NewJSONHandler, https://pkg.go.dev/golang.org/x/exp/slog#NewTextHandler
- zerolog: https://pkg.go.dev/github.com/rs/zerolog#New
The problem here is that the robfig/cron
logger is opinionated instead of extensible. This PR makes it extensible so that the end user can use any logger they want.
I also want to point out that other projects do this to allow for unification of logs, for example go-fiber: https://docs.gofiber.io/api/middleware/logger/
It's interesting that you say it's more about aligning APIs; but you don't realize that supporting io.Writer
does just that.
Thank you for your quick reply!
Do you agree that a logger created with
zapLogger, _ := zap.NewDevelopment()
stdOutLogger := zap.NewStdLog(zapLogger)
zW := stdOutLogger.Writer()
logger := cron.VerbosePrintfLogger(log.New(zW, "cron: ", log.Llongfile))
would behave almost the same way as the external logger you are adding? "Almost" because of the extra call to formatTimes()
.
You can definitely do it that way to the same affect. If that is preferred method in robfin/cron
I would suggest updating docs to reflect that. Especially since that doesnt follow the same pattern as various other projects do (passing an io.Writer
instead of wrapping an internal logger interface).
I'm not a maintainer and hence I cannot make any decision. Just wanted to share my thoughts.
However, I consider very interesting what you pointed out and maybe this could lead to some discussion for a potential logging API refactor proposal.
Refactor is probably overkill for something like this, especially with projects that use this library. That's why this PR adds additional functionality instead of removing/changing the old.