open-feature / go-sdk-contrib

Community maintained OpenFeature Providers and Hooks for Go
https://openfeature.dev
Apache License 2.0
42 stars 38 forks source link

InProcessResolver does not use provided Logger #522

Open appleton opened 3 months ago

appleton commented 3 months ago

Given configuration like this:

logger := logr.FromSlogHandler(slog.Default().Handler())
provider = flagd.NewProvider(
    flagd.WithLogger(logger),
    flagd.WithInProcessResolver(),
    flagd.WithOfflineFilePath(path),
)

I would expect the provided logger instance to be used but I actually see log output implying that it is not and when I dug into the provider initialization I see that the configured logger is ignored and that a new instance is created:

https://github.com/open-feature/go-sdk-contrib/blob/b90eb4de72975b4b60addefdab3f2cf20a82ff72/providers/flagd/pkg/service/in_process/service.go#L40-L62

This is tricky in my app because we don't use JSON as our log format so we end up with weird looking logs like this:

time=2024-06-06T10:02:58.161598000+01:00 level=INFO msg="Starting up server"
time=2024-06-06T10:02:58.258284000+01:00 level=INFO msg="Tracing is enabled" endpoint=localhost:4318 service=core-api
{"level":"info","ts":"2024-06-06T10:02:58+01:00","msg":"operating in in-process mode with offline flags sourced from /<redacted>/cmd/api/features.json"}
{"level":"info","ts":"2024-06-06T10:02:58+01:00","msg":"Starting filepath sync notifier"}
{"level":"info","ts":"2024-06-06T10:02:58+01:00","msg":"watching filepath: /Users/andy/poolside/forge/cmd/api/features.json"}
time=2024-06-06T10:02:58.276403000+01:00 level=INFO msg="Database is ready to serve"
beeme1mr commented 3 months ago

Hey @appleton, I agree that we should be using the logger supplied by the user. Would you be willing to make the change?

aepfli commented 3 months ago

I took a short look at this, and it seems to me this is not a trivial issue (at least from my noobish experience).

The problem is that go-sdk-contrib uses logr where as flagd uses zap. There is a library to wrap a zap logger in a logr.logger but not vice versa. We are passing a logr.logger to our service creation but need a zap logger.

If somebody can guide me here, on what best todo, i am more than happy to contribute.

beeme1mr commented 3 months ago

Could the flagd provider be updated to use logr?

aepfli commented 3 months ago

I had a chat with @Kavindu-Dodan. I will open issues for Flagd, where we evaluate and decide which logger we will use (slog or logr). Both do have their benefits, but we will decide within Flagd. there is also https://github.com/open-feature/go-sdk/issues/260 which discusses this change for go-sdk - I think the big goal here is to find one solution ;)

Kavindu-Dodan commented 3 months ago

@aepfli yeah, the go community is divided among frameworks. But I think given slog is standardized by Go lang itself, it will be the goto structured logging framework going forward.

Anyway, this change should be initiated at flagd core