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.

This is a naive implementation of #3558

It is provided to iterate on the changes, there are many things that are missing:

• tests
• WithLogger should be added at multiple places

Also, the current discussion in #3558 let me think the current logging interface should be reconsidered, so everything will have to be refactored.

[jit-ci]

Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset.

In case there are security findings, they will be communicated to you as a comment inside the PR.

Hope you’ll enjoy using Jit.

Questions? Comments? Want to learn more? Get in touch with us.

[ndyakov]

@ccoVeille thank you for opening this PR. I am willing to work with you on improving the logging and do agree that the current interface lacks some useful features. If you have a current need to just be able to set logger per client, we can improve this initially and then continue on the whole logging interface.

[ccoVeille]

I found a workaround by injecting a dedicated logger in the context and restoring from the context in the current Printf interface.

So I don't have a need right now. We can work together on the target solution you planned initially for the v10 with a fallback for the v9

[ndyakov]

so @ccoVeille would you consider this pr ready for review ?

[ccoVeille]

Yes, you can review. I'm curious about your feedback.

I dislike the idea it adds a configuration option in each struct that satisfies the current internal.Logging interface.

I feel like we shouldn't add option with interface we may change for supporting the log level and other things you listed.

So here I'm open to suggestions.

[ccoVeille]

so @ccoVeille would you consider this pr ready for review ?

Ping @ndyakov

Maybe I should have pinged you

[ndyakov]

@ccoVeille just moved it to Ready for Review and will work on reviewing it this week, thank you.

[ndyakov]

@ccoVeille thank you! Left some comments. What came to mind is that we can try to compose an Logger in those structs, but let's keep this for the future. For now, let's address the pointer to an interface and the ordering of the loggers when we decide which one to use. I am fine with any of the two approaches mentioned (e.g. by using the custom as default and fallback to the internal OR by having an explicit IF/ELSE statement)

[ccoVeille]

@ndyakov I applied your suggestions to the first commit

Then I have added another commit where I apply something that seems to me to be better.

Let me know what you think.

[ccoVeille]

I found a panic in the code, I made a fix for this branch, but you may want me to fix it separately

The bug is here, because err can be nil

go-redis/maintnotifications/handoff_worker.go

Lines 484 to 490 in c176672

	err := conn.Close() // Close the connection if no pool provided
	if err != nil {
	internal.Logger.Printf(ctx, "redis: failed to close connection: %v", err)
	}
	if internal.LogLevel.WarnOrAbove() {
	internal.Logger.Printf(ctx, logs.NoPoolProvidedCannotRemove(conn.GetID(), err))
	}

But here the call to reason.Error() will panic

go-redis/internal/maintnotifications/logs/log_messages.go

Lines 298 to 303 in c176672

	func NoPoolProvidedCannotRemove(connID uint64, reason error) string {
	message := fmt.Sprintf("conn[%d] %s due to: %v", connID, NoPoolProvidedMessageCannotRemoveMessage, reason)
	return appendJSONIfDebug(message, map[string]interface{}{
	"connID": connID,
	"reason": reason.Error(),
	})

Here is the code that spotted it because of my refactoring to always call the logger, and so remove the test on WarnOrAbove
https://github.com/redis/go-redis/actions/runs/19151645751/job/54743116032#step:3:1025

[ndyakov]

Nice approach! Thank you for fixing the panic as well! There is just one comment and I think we can merge this (please sync with master, there is another PR that may interfere with this one, but I will figure out the merging order so we can release next week.

[ndyakov]

added two more comments.

[ccoVeille]

I plan to squash the first 2 commits

[ndyakov]

This is starting to look better on each iterations, congrats @ccoVeille.

I have one note / suggestion that we can either implement now and do it in a separate PR:

• Right now we have one official way of setting the log level SetLogLevel but at the end this is only needed when there is no custom logger (granted, a user can use the global log level in their own custom logger implementation, but this is not important).

What I would like to suggest is to have a "constructor" function to create a new logger for a given level and then suggest (in the readme and in the documentation) that the users should create custom loggers and pass them in the options if they would like to change the log level. Of course, we won't remove the SetLogLevel function to be backwards compatible, and it will continue to work, but we can mark it as Deprecated and this can help us transition easily to custom loggers in the future.

[ccoVeille]

This is starting to look better on each iterations, congrats @ccoVeille.

You are welcome.

I have one note / suggestion that we can either implement now and do it in a separate PR:

• Right now we have one official way of setting the log level SetLogLevel but at the end this is only needed when there is no custom logger (granted, a user can use the global log level in their own custom logger implementation, but this is not important).

What I would like to suggest is to have a "constructor" function to create a new logger for a given level and then suggest (in the readme and in the documentation) that the users should create custom loggers and pass them in the options if they would like to change the log level. Of course, we won't remove the SetLogLevel function to be backwards compatible, and it will continue to work, but we can mark it as Deprecated and this can help us transition easily to custom loggers in the future.

We can of course do it in another PR.

But I feel like the current MR should target having code that accepts a logger which signature would match the one of slog.Logger

I feel like your lib should move to structured logger. Not in this PR. Take a look at the hack you have with the metadata you add via json.Encode.

I dislike the idea of merging the PR like this.

I read again slog and log. I have ideas. I will try to find a solution.

[ndyakov]

@ccoVeille I agree, but don't see a clear way to do it without breaking changes (if we include this PR). Let me think about it a bit. I won't have much time next week, but will get back to it as soon as possible.

[ccoVeille]

@ccoVeille I agree, but don't see a clear way to do it without breaking changes (if we include this PR). Let me think about it a bit. I won't have much time next week, but will get back to it as soon as possible.

I refactored everything.

Please take a took. I added structured logging support.

There might be some iterations to validate everything works as expected. I also need to add tests.
But now, it matches what I would expect from the package.