Logging should be done by the user of the SDK

[0xd3e]

Version: v3.0.0-next.2

There are several places where this SDK calls a logger to log certain events. I came across this in the authorization package.

In my opinion logging should be done by the user of the SDK, because logging is subjective and people handle logging differently. You assume a logger from the golang.org/x/exp/slog package, but log/slog landed in the standard library now. Some people might use different logging libraries.

I use the log/slog package from the standard library and now I have to glue the old package (golang.org/x/exp/slog) and the new package together somehow to not mess up my logging. This underlines my opinion that logging should be done by the user.

[0xd3e]

For anyone who comes across this and is not interested in the logs from the SDK, you can pass your own logger to the Authorizer which discards any log messages.

import (
    ...
    zitadellog "golang.org/x/exp/slog"
    ...
) 

var authz *authorization.Authorizer[*oauth.IntrospectionContext]
{
    authz, err = authorization.New[*oauth.IntrospectionContext](
        ctx,
        zitadel.New(conf.Zitadel.Domain),
        oauth.DefaultAuthorization(conf.Zitadel.KeyPath),
        authorization.WithLogger[*oauth.IntrospectionContext](
            zitadellog.New(
                zitadellog.NewTextHandler(
                    io.Discard, // Don't log anything from Zitadel
                    &zitadellog.HandlerOptions{},
                ),
            ),
        ),
    )
}

[redradrat]

@hifabienne Maybe that's a question of understanding here: If the only way to set a logger, is on allocating a new Authorizer, then that would mean by extension, we would have to do that for each request (as likely that would bring a new sublogger, or structured log data related to the request context).

Is the intention of the Authorizer to be spun up for every request? Or is there logic to apply Options to a New Authorizer after init?

Could also be my mental model is off here.

[adlerhurst]

If the only way to set a logger, is on allocating a new Authorizer, then that would mean by extension, we would have to do that for each request (as likely that would bring a new sublogger, or structured log data related to the request context).

Is the intention of the Authorizer to be spun up for every request? Or is there logic to apply Options to a New Authorizer after init?

@livio-a @muhlemmer I guess you know best about integrating the oidc lib.

[adlerhurst]

thanks for the inputs @0xd3e

We for sure need to upgrade to log/slog. I guess easiest would be to allow to configure a logger globally and then use the logger in the library. What do you think?

[0xd3e]

Is there even a need to log things? In my opinion logging is part of error handling and errors can just be returned to the caller. Then the caller can decide what to do with the error (log, retry, whatever).

But maybe there is a valid case for logging inside the library that I haven't seen.

[muhlemmer]

In my opinion logging is part of error handling and errors can just be returned to the caller. Then the caller can decide what to do with the error (log, retry, whatever).

But maybe there is a valid case for logging inside the library that I haven't seen.

There are many different opinions on this in the Go community. We opted that we log in our libraries and it has made our lives easier. Especially there are parts of the zitadel/oidc library that do not return to a caller, as they implement http handlers directly. The user of the library initiates the OP or server and from that point on the handlers return errors to the http client, not the package user. Hence, the library needs to log. The same goes for some of the client packages, like rp which also implements http handlers.

As log/slog allows deep customization through handlers, it also allows disabling it completely in a library and there will be almost no performance cost. So I don't see any objections using the logger in a library, even for functions that do return an error.

At the moment, if no logger is provided to the oidc package, it uses slog.Default(). We could change this to a disabled logger by default. However, a part of me also considers the defaulting to default is easier to understand. And if users want the default to be a disabled logger, they can achieve that by slog.SetDefault(), and then use custom loggers in their code where anything else is required. @0xd3e For a further discussion on this, please open an issue in the zitadel/oidc library and we can take it from there.

You assume a logger from the golang.org/x/exp/slog package

This was because of the go release policy we have with supporting the last 2 releases of Go. When we implemented slog, go 1.20 was just released so we needed to use exp/slog in order to build for go 1.19. Our oidc lib has since moved to log/slog and this library still needs to be updated. Our SDKs sometimes seem to lag because different people work on different parts of our ecosystem. @adlerhurst we can easily resolve that by version-bumping OIDC and related packages to the latest versions. (and drop go <1.20 support).

• @adlerhurst: For this repo we need to version-bump to the latest OIDC and replace exp/slog by log/slog, then we can close this issue specifically.

[0xd3e]

Thanks for the explanation. I understand your decision. Looking forward for the log/slog support.