replace github.com/siddontang/go-log with log/slog

[serprex]

as discussed in #929

There's room for further improvement. Ideally slog.Info functions would never be called, as these force usage of the global slog default. Instead all instances should somehow receive a logger. Or, if that isn't feasible, a global variable for the package should allow overriding the package's logger independently from the application (tho should still end up being slog.Default() in this case)

This is a breaking change, & since current interface includes setting handler in go-log to control logging, there's no way to make this not breaking

[lance6716]

Oh I just found it's not trivial to convert github.com/siddontang/go-log to log/slog 😂 I think it's better to merge it when we have a converting helper function or provide clear guidance.

[lance6716]

Hi @dveeden I want to merge this PR, and in readme or release note we remind the developers about this change. If the developers want the old logger he/she can still use https://github.com/serprex/slog-siddontang to fit the interface. What do you think?

[dveeden]

Hi @dveeden I want to merge this PR, and in readme or release note we remind the developers about this change. If the developers want the old logger he/she can still use https://github.com/serprex/slog-siddontang to fit the interface. What do you think?

This sounds good to me. I'll double check this PR today and then you can merge it later today or tomorrow.

[dveeden]

@lance6716 looks like there is a merge conflict? And this is in draft status?

[dveeden]

I don't really like the log+sprintf that's used in many places, however I think this is fine as a transition and can even help to reduce the changes in the default user visible output.

[serprex]

@lance6716 looks like there is a merge conflict? And this is in draft status?

PR was started as proof of concept, so was waiting on feedback on whether the design was acceptable or not. Will clean it up & remove draft status

[dveeden]

To build it needs something like this:

diff --git a/replication/binlogsyncer.go b/replication/binlogsyncer.go
index e95fbfd..cc50849 100644
--- a/replication/binlogsyncer.go
+++ b/replication/binlogsyncer.go
@@ -862,7 +862,7 @@ func (b *BinlogSyncer) handleEventAndACK(s *BinlogStreamer, e *BinlogEvent, need
        case *RotateEvent:
                b.nextPos.Name = string(event.NextLogName)
                b.nextPos.Pos = uint32(event.Position)
-               b.cfg.Logger.Info("rotate to next binlog", slog.String("file", b.nextPos.Name), slog.Int("position", b.nextPos))
+               b.cfg.Logger.Info("rotate to next binlog", slog.String("file", b.nextPos.Name), slog.Uint64("position", uint64(b.nextPos.Pos)))
 
        case *GTIDEvent:
                if b.prevGset == nil {

[dveeden]

We could do something like this as example:

diff --git a/cmd/go-mysqlbinlog/main.go b/cmd/go-mysqlbinlog/main.go
index 12f3ced..bb0bd12 100644
--- a/cmd/go-mysqlbinlog/main.go
+++ b/cmd/go-mysqlbinlog/main.go
@@ -7,6 +7,7 @@ import (
        "context"
        "flag"
        "fmt"
+       "log/slog"
        "os"
 
        "github.com/pingcap/errors"
@@ -31,6 +32,7 @@ var (
        backupPath = flag.String("backup_path", "", "backup path to store binlog files")
 
        rawMode = flag.Bool("raw", false, "Use raw mode")
+       format  = flag.String("format", "plain", "log format")
 )
 
 func main() {
@@ -50,6 +52,15 @@ func main() {
                MaxReconnectAttempts: 10,
        }
 
+       switch *format {
+       case "json":
+               cfg.Logger = slog.New(slog.NewJSONHandler(os.Stderr, nil))
+       case "plain":
+               // This is the default format
+       default:
+               panic("unsupported log format")
+       }
+
        err := mysql.ValidateFlavor(*flavor)
        if err != nil {
                fmt.Printf("Flavor error: %v\n", errors.ErrorStack(err))

dvaneeden@dve-carbon:~/dev/go-mysql$ ./bin/go-mysqlbinlog -format json > /dev/null
{"time":"2025-03-04T15:56:05.619590715+01:00","level":"INFO","msg":"begin to sync binlog from position","position":{"Name":"","Pos":4}}
{"time":"2025-03-04T15:56:05.625397936+01:00","level":"INFO","msg":"Connected to server","flavor":"mysql","version":"9.2.0"}
{"time":"2025-03-04T15:56:05.625582082+01:00","level":"INFO","msg":"rotate to next binlog","file":"binlog.000001","position":4}
^C

[dveeden]

Or even like this:

dvaneeden@dve-carbon:~/dev/go-mysql$ ./bin/go-mysqlbinlog -format json > /dev/null
{"time":"2025-03-04T16:04:40.707995695+01:00","level":"INFO","msg":"begin to sync binlog from position","position":{"Name":"","Pos":4}}
{"time":"2025-03-04T16:04:40.711097722+01:00","level":"INFO","msg":"Connected to server","flavor":"mysql","version":"9.2.0"}
{"time":"2025-03-04T16:04:40.711274055+01:00","level":"INFO","msg":"rotate to next binlog","file":"binlog.000001","position":4}
^C
dvaneeden@dve-carbon:~/dev/go-mysql$ ./bin/go-mysqlbinlog -format json -verbose > /dev/null
{"time":"2025-03-04T16:04:46.85523127+01:00","level":"INFO","source":{"function":"github.com/go-mysql-org/go-mysql/replication.(*BinlogSyncer).StartSync","file":"/home/dvaneeden/dev/go-mysql/replication/binlogsyncer.go","line":440},"msg":"begin to sync binlog from position","position":{"Name":"","Pos":4}}
{"time":"2025-03-04T16:04:46.860961252+01:00","level":"INFO","source":{"function":"github.com/go-mysql-org/go-mysql/replication.(*BinlogSyncer).prepare","file":"/home/dvaneeden/dev/go-mysql/replication/binlogsyncer.go","line":407},"msg":"Connected to server","flavor":"mysql","version":"9.2.0"}
{"time":"2025-03-04T16:04:46.861151704+01:00","level":"INFO","source":{"function":"github.com/go-mysql-org/go-mysql/replication.(*BinlogSyncer).handleEventAndACK","file":"/home/dvaneeden/dev/go-mysql/replication/binlogsyncer.go","line":865},"msg":"rotate to next binlog","file":"binlog.000001","position":4}
^C
dvaneeden@dve-carbon:~/dev/go-mysql$ ./bin/go-mysqlbinlog -verbose > /dev/null
time=2025-03-04T16:04:58.933+01:00 level=INFO source=/home/dvaneeden/dev/go-mysql/replication/binlogsyncer.go:440 msg="begin to sync binlog from position" position="(, 4)"
time=2025-03-04T16:04:58.938+01:00 level=INFO source=/home/dvaneeden/dev/go-mysql/replication/binlogsyncer.go:407 msg="Connected to server" flavor=mysql version=9.2.0
time=2025-03-04T16:04:58.939+01:00 level=INFO source=/home/dvaneeden/dev/go-mysql/replication/binlogsyncer.go:865 msg="rotate to next binlog" file=binlog.000001 position=4
^C

diff --git a/cmd/go-mysqlbinlog/main.go b/cmd/go-mysqlbinlog/main.go
index 12f3ced..512be58 100644
--- a/cmd/go-mysqlbinlog/main.go
+++ b/cmd/go-mysqlbinlog/main.go
@@ -7,6 +7,7 @@ import (
        "context"
        "flag"
        "fmt"
+       "log/slog"
        "os"
 
        "github.com/pingcap/errors"
@@ -31,6 +32,8 @@ var (
        backupPath = flag.String("backup_path", "", "backup path to store binlog files")
 
        rawMode = flag.Bool("raw", false, "Use raw mode")
+       format  = flag.String("format", "plain", "log format")
+       verbose = flag.Bool("verbose", false, "verbose logging")
 )
 
 func main() {
@@ -50,6 +53,19 @@ func main() {
                MaxReconnectAttempts: 10,
        }
 
+       logOpts := &slog.HandlerOptions{
+               AddSource: *verbose,
+       }
+
+       switch *format {
+       case "json":
+               cfg.Logger = slog.New(slog.NewJSONHandler(os.Stderr, logOpts))
+       case "plain":
+               cfg.Logger = slog.New(slog.NewTextHandler(os.Stderr, logOpts))
+       default:
+               panic("unsupported log format")
+       }
+
        err := mysql.ValidateFlavor(*flavor)
        if err != nil {
                fmt.Printf("Flavor error: %v\n", errors.ErrorStack(err))

Or we could add some info about this in the README instead?

[serprex]

I think readme is best place, for configuring logging people should either setup default slog logger (for consistent logging across app), or pass in logger (which tends to happen when code paths have enriched logger with extra fields)

edit: see this is about commands supplied by go-mysql, yeah that could work

[dveeden]

To me it looks like this is now ready to be merged. @lance6716 , do you agree?

[lance6716]

thanks @serprex , please check if my comments are acceptable.