feat: add optional logger wherever possible

[ccoVeille]

This commit introduces an optional logger parameter to various structs.
This enhancement allows users to provide custom logging implementations.

Fixes #3558

• Feature Request: ability to provide a logger as an Options setting #3558

Note

This is a follow-up of #3560 that was merged in a branch (not master) and #3618 that was updated by @ndyakov to add changes on it.

The MR is now ready to be merged.

• feat: add optional logger wherever possible #3560
• feat: optional logger #3618

[jit-ci]

❌ The following Jit checks failed to run:

• secret-detection-trufflehog
• static-code-analysis-semgrep-pro

#jit_bypass_commit in this PR to bypass, Jit Admin privileges required.

More info in the Jit platform.

[ccoVeille]

I apparently failed to keep your GPG signature @ndyakov, it sounds logic as I used history rewritting to fix an issue in one of the commit I did earlier. Feel free to pull, sign your commits, and push again. My fork is now all set for maintainers edits

[ccoVeille]

typo spotted

Suggested change

	// This will NOT handle all logging at the moment, singe there is still a global logger in use.
	// This will NOT handle all logging at the moment, since there is still a global logger in use.

[ccoVeille]

There is something I don't get here with this interface

Either LegacyLogger and LoggerWrapper also support

Error(ctx context.Context, msg string, v ...any)
Warn(ctx context.Context, msg string, v ...any)
Info(ctx context.Context, msg string, v ...any)
Debug(ctx context.Context, msg string, v ...any)

Why do you restrict the interface to simili printf logger ?

[ndyakov]

those were the only one used, i don't mind adding the non format methods. feel free to add them.

[ccoVeille]

In fact, I feel like we should consider inverting the logic here.

The custom logger available in the various config should only mention

Error(ctx context.Context, msg string, v ...any)
Warn(ctx context.Context, msg string, v ...any)
Info(ctx context.Context, msg string, v ...any)
Debug(ctx context.Context, msg string, v ...any)
Enabled(ctx context.Context, level LogLevelT) bool

So no Whateverf involved, then the code use the wrapper to transform things.

I feel like asking people to implement Errorf, Infof and co is an error.

[ndyakov]

The point of those logger methods is to print a formatted log message, so *f makes sense to me and it is consistent. Users don't have to implement those methods, they do however have to wrap their logger with the provided wrapper if their logger follows the *Context interface like slog.

[ccoVeille]

I wasn't suggesting to rename things such as:

Infof -> Info

My problem was the fact people will have to add a struct that would satisfy the legacy Printf methods

While we could add this in a way the logger only have to satistify the method that would be used by a structured logger

So the InfoContext one and so on.

The code could adapt itself to use the LoggerWrapper if needed.

Said otherwise people could provide directly a slog.Handler with the v9 as the logger of each config.

The logger wrapper is something we need right now because the code needs the Infof

But I would prefer the magic to be hidden from user. People seeing the interface would think "oh I need to implement Errorf, Warnf and co it's boring and not convenient"

I will add a commit to explain what I have in mind. But later, either this weekend or next week. I will also add tests for all that.

[ndyakov]

Overall looks good to me, can we just make sure this one is addressed (preferably add a simple test):
#3618 (comment)

[ccoVeille]

Overall looks good to me, can we just make sure this one is addressed (preferably add a simple test):
#3618 (comment)

Here the difference I applied to an old commit.

func (cl *LoggerWrapper) Errorf(ctx context.Context, format string, v ...any) { 
         if cl == nil || cl.logger == nil { 
                 legacyLoggerWithLevel.Errorf(ctx, format, v...) 
                 return 
         } 
-         cl.logger.ErrorContext(ctx, format, v...)
+         cl.logger.ErrorContext(cl.printfToStructured(ctx, format, v...))
 }

I can add a test, but adding a test about a logger is never fun.

Maybe it can be done in a separate PR. I'm suggesting that, but if you are OK to wait for me to add the tests a few more days, maybe a week. I can do it.

[ndyakov]

@ccoVeille No problem, I will prepare a beta without the logger, you can write tests and resolve conflicts in the next week and then we will include it.