Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for log/slog #656

Open
Integralist opened this issue Jan 24, 2024 · 11 comments
Open

Support for log/slog #656

Integralist opened this issue Jan 24, 2024 · 11 comments

Comments

@Integralist
Copy link

👋🏻

I was looking to migrate from log to log/slog and noticed that https://pkg.go.dev/github.com/gomodule/[email protected]/redis#NewLoggingConn is constrained to the former.

What is the plan as far as supporting the new structured logging package?

I presume you'd want to modify the various signatures to using an interface rather than a concrete type.

Thanks.

@stevenh
Copy link
Collaborator

stevenh commented Jan 24, 2024

My gut reaction is that would require a new connection type.

In the longer term a v2 package could look to use functional options to significantly reduce API surface area and simplify the creation of connection, with and without timeout, loggers etc.

I'd be interested to hear your thoughts on what the goals of structure logging for the package might be?

@ydnar
Copy link

ydnar commented Jan 24, 2024

@stevenh do the internals depend on a concrete log.Logger or an interface?

Would you consider a backwards-compatible PR that adds the ability to create a connection that accepts something conforming to an interface, e.g. redis.Logger or something?

@stevenh
Copy link
Collaborator

stevenh commented Jan 24, 2024

Quick look indicates it only uses https://pkg.go.dev/log#Logger.Output, but the implementation in its current form wouldn't be particularly useful structure logging from what I can see as it uses buffer to create a single string output.

We couldn't break the existing API so would need new constructor to change the way it's passed in.

Overall I'm not sure changing the existing one would achieve what you're looking for hence my question about what the goal you're trying to achieve is?

@Integralist
Copy link
Author

Yeah, I agree, a new constructor would be the better way to go here.

I presume the addition of a new constructor would still require threading changes through the call stack for redigo to support either a log.Logger and slog.Logger.

As far as the question...

what the goals of structure logging for the package might be?

Our application currently uses log.Logger and we're looking to upgrade to the new slog package so we can benefit from more structured logging, so having our log output be consistent across redigo would be nice.

@stevenh
Copy link
Collaborator

stevenh commented Jan 24, 2024

what the goals of structure logging for the package might be?

Our application currently uses log.Logger and we're looking to upgrade to the new slog package so we can benefit from more structured logging, so having our log output be consistent across redigo would be nice.

But I'm guessing this would also include the use of structure, so a direct port the current logger wouldn't provide the full benefit even if it would enable you to use the same logger, I'm thinking performance and actual structure so things could be filtered base on action etc?

@cee-dub
Copy link

cee-dub commented Jan 24, 2024

I think all we need is log/slog.NewLogLogger() and to pass that to the existing constructor.

@stevenh
Copy link
Collaborator

stevenh commented Jan 25, 2024

If that's the case no changes needed?

I think I was thinking more scope, to add structure and improve performance.

@Integralist
Copy link
Author

So yeah at the moment I'm actually just getting by with slog.NewLogLogger()

redis.NewLoggingConn(c, slog.NewLogLogger(std.Handler(), slog.LevelInfo), "redis")

This is fine to unblock me, but I would imagine actual support for slog in redigo would require some changes which will depend on how it's implemented.

So we know from prior discussion that a new constructor that accepts a *slog.Logger type would be preferred over a new major release that changes the API to accept an interface.

But then the underlying loggingConn needs to have the logger stored off. So do you change the logger field to use an interface type and you build an abstraction over the top of it so that each logger call site will call down to its relevant instance (e.g. *log.Logger vs *slog.Logger).

Or do you have a new field added, like slogger *slog.Logger, and again, each call site where a log record is made the code has to check if logger or slogger is nil and then whichever isn't nil will get called.

And on top of that, regardless of the approach (new field vs interface type), redigo needs to structure the data (if dealing with *slog.Logger).

e.g. go from...

func (c *loggingConn) Close() error {
    // ...
    c.logger.Output(2, buf.String())
}

...to...

func (c *loggingConn) Close() error {
    // ...
    c.logger.LogAttrs(context.Background(), slog.LevelInfo, "closing connection",
        slog.String("event", "conn_close"),
    )
}

Probably not a great example ☝🏻 as I've realised just now how simple the log call actually is 🙂

I imagine the print() method probably would gain a bit more 'structure'.

But yeah, that was generally what I was thinking in my head.

@stevenh
Copy link
Collaborator

stevenh commented Jan 26, 2024

Without looking in detail I'd be tempted to create a new logging connection that has proper slog support. If that could then be used to provide a compatible constructor for the original even better.

@stevenh
Copy link
Collaborator

stevenh commented Jan 27, 2024

Had poke around in this, and while it would simple to create a new logging connection type with proper slog support, I already have 95% written, I do wonder if this is driven by the current structure, which it makes is hard to enhance current or add new features easily.

How do folks currently use this feature, code snippets would be most appreciated.

I'm wondering if it's time for a v2 that supports a progressive design such as functional options?

@ydnar
Copy link

ydnar commented Jan 27, 2024

Nice! That escalated fast!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants