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

InProcessResolver does not use provided Logger #522

Open
appleton opened this issue Jun 6, 2024 · 5 comments
Open

InProcessResolver does not use provided Logger #522

appleton opened this issue Jun 6, 2024 · 5 comments
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@appleton
Copy link

appleton commented Jun 6, 2024

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:

func NewInProcessService(cfg Configuration) *InProcess {
log := logger.NewLogger(zap.NewRaw(), false)
iSync, uri := makeSyncProvider(cfg, log)
// service specific metadata
var svcMetadata map[string]interface{}
if cfg.Selector != "" {
svcMetadata = make(map[string]interface{}, 1)
svcMetadata["scope"] = cfg.Selector
}
flagStore := store.NewFlags()
flagStore.FlagSources = append(flagStore.FlagSources, uri)
return &InProcess{
evaluator: evaluator.NewJSON(log, flagStore),
events: make(chan of.Event, 5),
logger: log,
listenerShutdown: make(chan interface{}),
serviceMetadata: svcMetadata,
sync: iSync,
}
}

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 beeme1mr added enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed labels Jun 6, 2024
@beeme1mr
Copy link
Member

beeme1mr commented Jun 6, 2024

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

@aepfli
Copy link
Member

aepfli commented Jun 7, 2024

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
Copy link
Member

beeme1mr commented Jun 7, 2024

Could the flagd provider be updated to use logr?

@aepfli
Copy link
Member

aepfli commented Jun 7, 2024

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 open-feature/go-sdk#260 which discusses this change for go-sdk - I think the big goal here is to find one solution ;)

@Kavindu-Dodan
Copy link
Contributor

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants