search

simonvetter / modbus

Go modbus stack (client and server)
MIT License
280 stars 89 forks source link

Allow custom logger #20

Closed andig closed 1 year ago

andig commented 1 year ago

Nothing to add- would be happy to provide PR.

simonvetter commented 1 year ago

Hi andig,

the latest release (v1.6.0) allows passing a custom logger to both server and client objects, does that help?

If yes, I'll let you close this issue.

andig commented 1 year ago

Imho the logger should be an interface, not a concrete implementation?

simonvetter commented 1 year ago

So, I thought about it during the weekend and went back and forth over it.

I almost sided with you on it being an interface, but the golang standard lib has a fairly compact, no-frills object which everyone seems to be using, so I just went with that for the sake of clarity and simplicity.

You should be able to instantiate a bare-bones log.Logger object with the third party io.Writer of your choice though, see https://github.com/simonvetter/modbus/blob/master/logger_test.go#L13 for an example.

If that proves to be an issue over time, i'll reconsider and change the log.Logger type to an interface (which log.Logger satisfies, so as not to break the API) later on.

andig commented 1 year ago

Using the writer forces you to parse the log levels which is really expensive (and error-prone). The typical pattern is having an interface and doing so (even adding it now) wouldn't even impact compatibility.

simonvetter commented 1 year ago

Yep, agreed. Unfortunately Go's default logger doesn't provide any way to specify the criticity/level of a message.

That said, is your use case logging so much that logger performance is an issue? On my (medium to large) deployments, I'm only seeing the occasional CRC error or wrong transaction ID, not much more.

andig commented 1 year ago

That said, is your use case logging so much that logger performance is an issue?

No, it isn't. But I wouldn't use that as argument against "doing it right". I need a logger for the physical traffic and I prefer to raise PRs against libraries I'm using to introduce interfaces. Ymmv of course.

Happy to close if you want to leave as-is now.

simonvetter commented 1 year ago

Oh yeah, i see. Like I said, I may revisit this in the future, but I'd rather keep the design simple and adhere to Golang's standard libraries for now.

If full wire traffic logging is your use case then I agree logging performance could be an issue. If I had to implement something like this I'd most likely do it as a generic TCP proxy, or maybe as a wrapper around a TCPConn object, rather than adding it to the modbus library.

I'm going to go ahead and close this, feel free to reopen if needed.