Skip to content

Enhanced Logging Interface #80

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.

Sign up for GitHub

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

Merged
hnicolaysen merged 7 commits into from
Apr 29, 2024
Merged

Enhanced Logging Interface #80

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
aaeb082
Implementing a recent logger and changing the severity level from Inf…
MontmirailValentin Jan 5, 2024
5e8be8a
Cannot use slog, we are still in Go 1.13
MontmirailValentin Jan 5, 2024
6d14636
Make the New() function private. It is only related to logger so chan…
MontmirailValentin Jan 5, 2024
8eeb8b6
Adding a breaking point as accepted by frzifus
MontmirailValentin Jan 21, 2024
59965a7
Update to Go 1.21 since slog is only available starting from 1.21
MontmirailValentin Jan 21, 2024
401c496
Update all packages to their latest versions. We still need a baselin…
MontmirailValentin Jan 21, 2024
b70fb8d
Fixing merge conflict after the upgrade to Go 1.21
MontmirailValentin Apr 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ascii_over_tcp_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ func (mb *asciiTCPTransporter) Send(aduRequest []byte) (aduResponse []byte, err
}

// Send the request
mb.logf("modbus: send %q\n", aduRequest)
mb.Debug("modbus: send %q\n", aduRequest)
if _, err = mb.conn.Write(aduRequest); err != nil {
return
}
Expand All @@ -78,6 +78,6 @@ func (mb *asciiTCPTransporter) Send(aduRequest []byte) (aduResponse []byte, err
}
}
aduResponse = data[:length]
mb.logf("modbus: recv %q\n", aduResponse)
mb.Debug("modbus: recv %q\n", aduResponse)
return
}
4 changes: 2 additions & 2 deletions asciiclient.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ func (mb *asciiSerialTransporter) Send(aduRequest []byte) (aduResponse []byte, e
mb.startCloseTimer()

// Send the request
mb.logf("modbus: send % x\n", aduRequest)
mb.Debug("modbus: send % x\n", aduRequest)
if _, err = mb.port.Write(aduRequest); err != nil {
return
}
Expand All @@ -204,7 +204,7 @@ func (mb *asciiSerialTransporter) Send(aduRequest []byte) (aduResponse []byte, e
}
}
aduResponse = data[:length]
mb.logf("modbus: recv % x\n", aduResponse)
mb.Debug("modbus: recv % x\n", aduResponse)
return
}

Expand Down
4 changes: 3 additions & 1 deletion client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@ import (

// logger is the interface to the required logging functions
type logger interface {
Printf(format string, v ...interface{})
Debug(format string, args ...interface{})
Info(format string, args ...interface{})
Error(format string, args ...interface{})
Copy link
Copy Markdown
Contributor

@frzifus frzifus Jan 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my point of view, it would be a good improvement if the available API calls would take the context into account.

Does it perhaps make sense to include the context already in the logger? The Slog impl. itself would fulfills it too.

func (l *Logger) InfoContext(ctx context.Context, msg string, args ...any) {...}
func (l *Logger) DebugContext(ctx context.Context, msg string, args ...any) {...}
func (l *Logger) ErrorContext(ctx context.Context, msg string, args ...any) {...}

}

// ClientHandler is the interface that groups the Packager and Transporter methods.
Expand Down
44 changes: 39 additions & 5 deletions cmd/modbus-cli/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,44 @@ import (
"github.com/grid-x/serial"
)

type logger struct {
}

func newLogger(out io.Writer, prefix string, flag int) logger {
log.SetOutput(out)
log.SetPrefix(prefix)
log.SetFlags(flag)
Copy link
Copy Markdown
Contributor

@dammarco dammarco Jan 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason why you are using log and not slog here?
I saw that you are using slog in the unit tests later.

Copy link
Copy Markdown
Contributor

@frzifus frzifus Jan 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition, is there a reason why this logger implements more methods than those specified in the interface?

Maybe it makes sense to simply create a slog instance, this would match the defined interface. In the modbus cli case, this would be a breaking change, which in my opinion is acceptable.

logger := slog.Default()
logger.Info(...)
logger.Debug(...)
logger.Error(...)

return logger{}
}

func (l logger) Printf(format string, v ...interface{}) {
log.Printf(format, v...)
}

func (l logger) Println(format string, v ...interface{}) {
log.Printf(format, v...)
}

func (l logger) Debug(format string, v ...interface{}) {
log.Printf("DEBUG: "+format, v...)
}

func (l logger) Info(format string, v ...interface{}) {
log.Printf("INFO: "+format, v...)
}

func (l logger) Error(format string, v ...interface{}) {
log.Printf("ERROR: "+format, v...)
}

func (l logger) Fatalf(format string, v ...interface{}) {
log.Fatalf(format, v...)
}

func (l logger) Fatal(v ...interface{}) {
log.Fatal(v...)
}

func main() {
var opt option
// general
Expand Down Expand Up @@ -64,7 +102,7 @@ func main() {
return
}

logger := log.New(os.Stdout, "", 0)
logger := newLogger(os.Stdout, "", 0)
if *register > math.MaxUint16 || *register < 0 {
logger.Fatalf("invalid register value: %d", *register)
}
Expand Down Expand Up @@ -408,10 +446,6 @@ func resultToString(r []byte, order binary.ByteOrder, forcedOrder string, varTyp
return "", fmt.Errorf("unsupported datatype: %s", varType)
}

type logger interface {
Printf(format string, v ...interface{})
}

type option struct {
address string
slaveID int
Expand Down
4 changes: 2 additions & 2 deletions rtu_over_tcp_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ func (mb *rtuTCPTransporter) Send(aduRequest []byte) (aduResponse []byte, err er
}

// Send the request
mb.logf("modbus: send % x\n", aduRequest)
mb.Debug("modbus: send % x\n", aduRequest)
if _, err = mb.conn.Write(aduRequest); err != nil {
return
}
Expand Down Expand Up @@ -95,6 +95,6 @@ func (mb *rtuTCPTransporter) Send(aduRequest []byte) (aduResponse []byte, err er
return
}
aduResponse = data[:n]
mb.logf("modbus: recv % x\n", aduResponse)
mb.Debug("modbus: recv % x\n", aduResponse)
return
}
4 changes: 2 additions & 2 deletions rtuclient.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,7 @@ func (mb *rtuSerialTransporter) Send(aduRequest []byte) (aduResponse []byte, err
mb.startCloseTimer()

// Send the request
mb.logf("modbus: send % x\n", aduRequest)
mb.Debug("modbus: send % x\n", aduRequest)
if _, err = mb.port.Write(aduRequest); err != nil {
return
}
Expand All @@ -271,7 +271,7 @@ func (mb *rtuSerialTransporter) Send(aduRequest []byte) (aduResponse []byte, err
time.Sleep(mb.calculateDelay(len(aduRequest) + bytesToRead))

data, err := readIncrementally(aduRequest[0], aduRequest[1], mb.port, time.Now().Add(mb.Config.Timeout))
mb.logf("modbus: recv % x\n", data[:])
mb.Debug("modbus: recv % x\n", data[:])
aduResponse = data
return
}
Expand Down
18 changes: 15 additions & 3 deletions serial.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,21 @@ func (mb *serialPort) close() (err error) {
return
}

func (mb *serialPort) logf(format string, v ...interface{}) {
func (mb *serialPort) Debug(format string, v ...interface{}) {
if mb.Logger != nil {
mb.Logger.Printf(format, v...)
mb.Logger.Debug(format, v...)
}
}

func (mb *serialPort) Info(format string, v ...interface{}) {
if mb.Logger != nil {
mb.Logger.Info(format, v...)
}
}

func (mb *serialPort) Error(format string, v ...interface{}) {
if mb.Logger != nil {
mb.Logger.Error(format, v...)
}
}

Expand All @@ -96,7 +108,7 @@ func (mb *serialPort) closeIdle() {
}

if idle := time.Since(mb.lastActivity); idle >= mb.IdleTimeout {
mb.logf("modbus: closing connection due to idle timeout: %v", idle)
mb.Debug("modbus: closing connection due to idle timeout: %v", idle)
mb.close()
}
}
24 changes: 18 additions & 6 deletions tcpclient.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ func (mb *tcpTransporter) Send(aduRequest []byte) (aduResponse []byte, err error
return
}
// Send data
mb.logf("modbus: send % x", aduRequest)
mb.Debug("modbus: send % x", aduRequest)
if _, err = mb.conn.Write(aduRequest); err != nil {
return
}
Expand All @@ -201,7 +201,7 @@ func (mb *tcpTransporter) Send(aduRequest []byte) (aduResponse []byte, err error
continue
}

mb.logf("modbus: close connection and retry, because of %v", err)
mb.Debug("modbus: close connection and retry, because of %v", err)

mb.close()
time.Sleep(mb.LinkRecoveryTimeout)
Expand All @@ -216,7 +216,7 @@ func (mb *tcpTransporter) readResponse(aduRequest []byte, data []byte, recoveryD
if err == nil {
err = verify(aduRequest, aduResponse)
if err == nil {
mb.logf("modbus: recv % x\n", aduResponse)
mb.Debug("modbus: recv % x\n", aduResponse)
return // everything is OK
}
}
Expand Down Expand Up @@ -382,9 +382,21 @@ func (mb *tcpTransporter) flush(b []byte) (err error) {
return
}

func (mb *tcpTransporter) logf(format string, v ...interface{}) {
func (mb *tcpTransporter) Debug(format string, v ...interface{}) {
if mb.Logger != nil {
mb.Logger.Printf(format, v...)
mb.Logger.Debug(format, v...)
}
}

func (mb *tcpTransporter) Info(format string, v ...interface{}) {
if mb.Logger != nil {
mb.Logger.Info(format, v...)
}
}

func (mb *tcpTransporter) Error(format string, v ...interface{}) {
if mb.Logger != nil {
mb.Logger.Error(format, v...)
}
}

Expand All @@ -407,7 +419,7 @@ func (mb *tcpTransporter) closeIdle() {
}

if idle := time.Since(mb.lastActivity); idle >= mb.IdleTimeout {
mb.logf("modbus: closing connection due to idle timeout: %v", idle)
mb.Debug("modbus: closing connection due to idle timeout: %v", idle)
mb.close()
}
}
4 changes: 2 additions & 2 deletions test/asciiclient_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
package test

import (
"log"
"log/slog"
"os"
"testing"

Expand All @@ -30,7 +30,7 @@ func TestASCIIClientAdvancedUsage(t *testing.T) {
handler.Parity = "E"
handler.StopBits = 1
handler.SlaveID = 12
handler.Logger = log.New(os.Stdout, "ascii: ", log.LstdFlags)
handler.Logger = slog.New(slog.NewJSONHandler(os.Stdout, nil))
err := handler.Connect()
if err != nil {
t.Fatal(err)
Expand Down
4 changes: 2 additions & 2 deletions test/rtu_over_tcp_client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
package test

import (
"log"
"log/slog"
"os"
"testing"
"time"
Expand All @@ -28,7 +28,7 @@ func TestRTUOverTCPClientAdvancedUsage(t *testing.T) {
handler := modbus.NewRTUOverTCPClientHandler(rtuOverTCPDevice)
handler.Timeout = 5 * time.Second
handler.SlaveID = 1
handler.Logger = log.New(os.Stdout, "rtu over tcp: ", log.LstdFlags)
handler.Logger = slog.New(slog.NewJSONHandler(os.Stdout, nil))
handler.Connect()
defer handler.Close()

Expand Down
4 changes: 2 additions & 2 deletions test/rtuclient_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
package test

import (
"log"
"log/slog"
"os"
"testing"

Expand All @@ -30,7 +30,7 @@ func TestRTUClientAdvancedUsage(t *testing.T) {
handler.Parity = "E"
handler.StopBits = 1
handler.SlaveID = 11
handler.Logger = log.New(os.Stdout, "rtu: ", log.LstdFlags)
handler.Logger = slog.New(slog.NewJSONHandler(os.Stdout, nil))
err := handler.Connect()
if err != nil {
t.Fatal(err)
Expand Down
4 changes: 2 additions & 2 deletions test/tcpclient_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
package test

import (
"log"
"log/slog"
"os"
"testing"
"time"
Expand All @@ -26,7 +26,7 @@ func TestTCPClientAdvancedUsage(t *testing.T) {
handler := modbus.NewTCPClientHandler(tcpDevice)
handler.Timeout = 5 * time.Second
handler.SlaveID = 1
handler.Logger = log.New(os.Stdout, "tcp: ", log.LstdFlags)
handler.Logger = slog.New(slog.NewJSONHandler(os.Stdout, nil))
handler.Connect()
defer handler.Close()

Expand Down