search

grid-x / modbus

BSD 3-Clause "New" or "Revised" License
78 stars 26 forks source link

Enhanced Logging Interface #80

Closed ValentinMontmirail closed 4 months ago

ValentinMontmirail commented 8 months ago

Overview

This PR introduces an enhancement to the logging system within our codebase. We've evolved from a single-function logging interface to a more robust, level-specific logging interface. This change not only improves the clarity of our logs but also aligns with best practices in logging management.

Updated Logger Interface:

The existing logger interface, which previously contained only the Printf function, has been expanded to include three distinct functions: Debug, Info, and Error. Each function is tailored to a specific severity level of logging. The updated interface is as follows:

// logger is the interface to the required logging functions
type logger interface {
    Debug(format string, args ...interface{})
    Info(format string, args ...interface{})
    Error(format string, args ...interface{})
}

Refactoring Log Statements:

All instances of the Printf method throughout the code have been reviewed and replaced with the appropriate logging level method. This refactoring ensures that each log statement is categorized correctly as per its context and severity. The changes are as follows:

Impact:

Request for Review:

I kindly request a review of the changes, focusing on the appropriateness of the log level classifications and the overall structure of the new logging interface. Any feedback or suggestions for further improvement are highly welcome.

Bug Fixed:

ValentinMontmirail commented 8 months ago

Friendly ping @frzifus. I think having a severity-level for the logs will be highly appreciated by Industry !

ValentinMontmirail commented 7 months ago

I am now using slog but keep in mind that slog is only available after 1.21+ so we will need a updated baseline image for the CI/CD. Either that, or we will have to move to GitHub Actions.

frzifus commented 7 months ago

It might make sense to bump it to 1.21, since 1.19 is already deprecated. wdyt @tretmar ?

dammarco commented 7 months ago

we are checking that internally ;)

hnicolaysen commented 7 months ago

@ValentinMontmirail we updated to repository to Go 1.21

frzifus commented 4 months ago

ping @ValentinMontmirail

ValentinMontmirail commented 4 months ago

Hello @frzifus, sorry about that, I did not see the message on Feb. 14... Fixed the merge conflict after your upgrade to Go 1.21.0, we now have slog that can deal with Context when required :).

andig commented 4 months ago

Fwiw: it would have been great to squash the commits on merge.

andig commented 4 months ago

See https://github.com/grid-x/modbus/pull/86#issuecomment-2090381752. This is a breaking change for existing consumers! The added interface is much broader than necessary and makes life for implementers unnecessarily hard.

I'd suggest to revert- until versioning clarified and minimal logger interface agreed.

andig commented 4 months ago

Ping @frzifus @hsteidl @tretmar this PR breaks current users of this module if they supply their own logger. It should be reverted.

dammarco commented 4 months ago

@andig we already tried to revert it directly but due to another PR that got merged before, the revert feature of GitHub doesn't work here. It's not so easy to have a clean revert right now.

andig commented 4 months ago

@tretmar seems it is agreed that this PR should be reverted? Then I'd suggest to move forward together with @srebhan with https://github.com/grid-x/modbus/pull/86 and use that PR to trim the interface back to what it was before. We can demonstrate in main.go how to use an slog Logger for implement that interface.

andig commented 4 months ago

Prepared #88 for full cleanup.

hnicolaysen commented 4 months ago

According to the discussion above, we just kicked off a discussion thread about a release strategy for this repo. Please have a look: https://github.com/grid-x/modbus/discussions/89.