Enhanced Logging Interface

[ValentinMontmirail]

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:

• Debugging logs are now using Debug()
• Informational logs are now using Info()
• Error logs are now using Error()

Impact:

• Improved Readability: Log statements are now more descriptive and contextually appropriate, making it easier for developers to understand the code flow and debug issues.

• Enhanced Logging Control: With level-specific logging methods, it's now possible to control the verbosity of logs more effectively, which is crucial for production environments.

• Alignment with Logging Standards: This change aligns our logging practices with common industry standards, enhancing the maintainability and scalability of our codebase.

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:

• Enhancement of Logging System in Modbus Package #79

[ValentinMontmirail]

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

[dammarco]

Thank you very much for the initiative here @ValentinMontmirail !
From my side, I don't see any issues as we are not using the logs in our system 🤔
I like the improvement of having a few log levels available to be used.

I have requested the review from our logging expert - maybe I am not seeing everything :D

[dammarco]

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

[frzifus]

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(...)

[frzifus]

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(...)

[frzifus]

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) {...}

[ValentinMontmirail]

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]

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

[dammarco]

we are checking that internally ;)

[hnicolaysen]

@ValentinMontmirail we updated to repository to Go 1.21

[frzifus]

ping @ValentinMontmirail

[ValentinMontmirail]

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 :).

[hnicolaysen]

Thank you for the initiative to improve the logging here 🙌
Good to see some log levels being used now and finally the switch to slog!

[andig]

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

[andig]

See #86 (comment). 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]

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

[dammarco]

@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]

@tretmar seems it is agreed that this PR should be reverted? Then I'd suggest to move forward together with @srebhan with #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]

Prepared #88 for full cleanup.

[hnicolaysen]

According to the discussion above, we just kicked off a discussion thread about a release strategy for this repo. Please have a look: #89.