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

Logging With Zap Logger #160

Open
reesmanp opened this issue Jun 26, 2024 · 8 comments
Open

Logging With Zap Logger #160

reesmanp opened this issue Jun 26, 2024 · 8 comments
Labels
enhancement New feature or request server-sdk Issues affecting the Go server SDK

Comments

@reesmanp
Copy link

Zap is a popular logging library for golang which I am currently using. However, the logging interface launch darkly uses is at odds with zap and I cannot utilize the same logger I am using for the rest of the application with launch darkly. I think it would be ideal to support zap here.

@reesmanp reesmanp added enhancement New feature or request server-sdk Issues affecting the Go server SDK labels Jun 26, 2024
@cwaldren-ld
Copy link
Contributor

Hi @reesmanp , can you explain a bit more about what difficulty you are encountering - specifically?

Is there some reason Zap cannot be used with the BaseLoggers interface?

@reesmanp
Copy link
Author

@cwaldren-ld thanks for responding. There are two main issues. One is easy enough to add a wrapper around and implement myself, that is the f functions (Debugf, Errorf, etc). The real issue is that the non-f functions have the requirement of:

Debug(values ...interface{})

where as zap has:

Debug(msg string, fields ...Field)

@AFMiziara
Copy link

We would really appreciate this too. I don't understand how Println(values ...interface{}) works exactly to be able to implement a wrapper in Zap. Should I just concatenate all these values in one string? The current BaseLogger interface is very poor.

@cwaldren-ld
Copy link
Contributor

cwaldren-ld commented Aug 6, 2024

Hi @AFMiziara and @reesmanp , I think you might be running into friction because the BaseLogger is operating at a lower level of abstraction compared to Zap's functionality.

Our documentation says:

// Logs a message on a single line. This is equivalent to log.Logger.Println.

It is expected to behave like Go's standard log.Println works. This is meant to log the final string that is sent to output, so it's not aware of a log level.

For example, Println might be called by the SDK's logger like: Println("info: something has happened").

You can forward this to Zap. Example:

func (m *myBaseLogger) Println(args ...interface{}) {
    m.zapLogger.Sugar().Info(args...)
}

Or, you could use structured logging and have a field for "launchdarkly message" or similar.

One issue with the example I gave is that all logs would then be logged at Zap's Info level (or whatever you chose), whereas the actual string passed by the SDK will contain a level prefix (info:, warn:, etc.).

If that's unacceptable, you could use SetBaseLoggerForLevel. Pseudocode just for the idea:

// somewhere:
loggers.SetBaseLoggerForLevel(ldlog.Info, newZapInfoLogger(ldlog.Info))

// zapInfoLogger holds a reference to a zap logger, and an ldlog.Level so it can know what to strip 
// out of the message
func (m *zapInfoLogger) Println(args ...interface{}) {
    m.zap.Info(strings.TrimPrefix(fmt.Sprintf(args...), m.level.String()+":")
}
// you could instead convert the ldlog.Level into a zapcore.Level and use one of Zap's functions
// that accepts a level.

@cwaldren-ld cwaldren-ld added the waiting for feedback Indicates LaunchDarkly is waiting for customer feedback before issue is closed due to staleness. label Aug 7, 2024
@reesmanp
Copy link
Author

@cwaldren-ld ok, I think this works. Definitely not an ideal case but workable. Is this something you could add to the documentation?

Copy link
Contributor

This issue is marked as stale because it has been open for 30 days without activity. Remove the stale label or comment, or this will be closed in 7 days.

@github-actions github-actions bot added the Stale label Sep 19, 2024
@cwaldren-ld cwaldren-ld removed waiting for feedback Indicates LaunchDarkly is waiting for customer feedback before issue is closed due to staleness. Stale labels Sep 19, 2024
@jsok
Copy link

jsok commented Sep 23, 2024

Would adopting slog be a better alternative?

Current workaround I use is:

type strippedPrefixHandler struct {
	h      slog.Handler
	prefix string
}

func newStrippedPrefixHandler(h slog.Handler, prefix string) *strippedPrefixHandler {
	return &strippedPrefixHandler{h: h, prefix: prefix}
}

func (h *strippedPrefixHandler) Enabled(ctx context.Context, l slog.Level) bool {
	return h.h.Enabled(ctx, l)
}

func (h *strippedPrefixHandler) Handle(ctx context.Context, rec slog.Record) error {
	rec = rec.Clone()
	rec.Message = strings.TrimPrefix(rec.Message, h.prefix)
	return h.h.Handle(ctx, rec)
}

func (h *strippedPrefixHandler) WithAttrs(attrs []slog.Attr) slog.Handler {
	return h.h.WithAttrs(attrs)
}

func (h *strippedPrefixHandler) WithGroup(name string) slog.Handler {
	return h.h.WithGroup(name)
}

var _ slog.Handler = (*strippedPrefixHandler)(nil)

Finally, the client can be configured like:

var l *slog.Logger
loggers := ldlog.NewDefaultLoggers()

loggers.SetBaseLoggerForLevel(ldlog.Info, slog.NewLogLogger(newStrippedPrefixHandler(l.Handler(), "INFO: "), slog.LevelInfo))
loggers.SetBaseLoggerForLevel(ldlog.Warn, slog.NewLogLogger(newStrippedPrefixHandler(l.Handler(), "WARN: "), slog.LevelWarn))
loggers.SetBaseLoggerForLevel(ldlog.Debug, slog.NewLogLogger(newStrippedPrefixHandler(l.Handler(), "DEBUG: "), slog.LevelDebug))
loggers.SetBaseLoggerForLevel(ldlog.Error, slog.NewLogLogger(newStrippedPrefixHandler(l.Handler(), "ERROR: "), slog.LevelError))

config := ldclient.Config{Logging: ldcomponents.Logging().Loggers(loggers)}

@cwaldren-ld
Copy link
Contributor

cwaldren-ld commented Sep 23, 2024

Thank you for sharing that, @jsok.

I think slog would be good to adopt. Currently the minimum supported Go version of the SDK is Go 1.18, whereas slog was introduced in Go 1.21.

Our version policy is to support the last two releases of Go, so we could make this change (we generally try not to raise the minimum version unless it's required by useful feature - which we might consider slog to be.)

Filed internally as SDK-706.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request server-sdk Issues affecting the Go server SDK
Projects
None yet
Development

No branches or pull requests

4 participants