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