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

slog: Handle arbitrary log level #603

Open
ccremer opened this issue Jan 2, 2024 · 1 comment
Open

slog: Handle arbitrary log level #603

ccremer opened this issue Jan 2, 2024 · 1 comment
Labels
proposal Proposal to add a new feature to pterm proposal-accepted Proposals which are accepted for implementation in the future

Comments

@ccremer
Copy link

ccremer commented Jan 2, 2024

Problem

slog.Level is an alias on an int. While slog has defined some constants like slog.Level{Debug, Info, Warn, Error}, the docs state that these are not the only supported levels and there are levels between the default constants.
However, pterm does a 1:1 mapping of the constants and not allow any others:

pterm/slog_handler.go

Lines 15 to 27 in 33f6aa1

func (s *SlogHandler) Enabled(ctx context.Context, level slog.Level) bool {
switch level {
case slog.LevelDebug:
return s.logger.CanPrint(LogLevelDebug)
case slog.LevelInfo:
return s.logger.CanPrint(LogLevelInfo)
case slog.LevelWarn:
return s.logger.CanPrint(LogLevelWarn)
case slog.LevelError:
return s.logger.CanPrint(LogLevelError)
}
return false
}

The code above effectively means that something like

backend := pterm.DefaultLogger.WithLevel(pterm.LogLevelTrace)
handler := pterm.NewSlogHandler(backend)
slogger := slog.New(handler)
slogger.Log(context.Background(), slog.Level(-5), "trace")

is not going to be printed, even though it might conceptually match pterm's LogLevelTrace.

Proposal

Support arbitrary slog levels

...Especially for increased verbosity like debugging/traces. The good news is, that both pterm and slog use a numeric type for level with increasing severity for higher values. slog starts at 0 for info, pterm starts with an offset at 3. In the easy case we can just add 3 to the slog level. E.g.

// Enabled returns true if the given level is enabled.
func (s *SlogHandler) Enabled(ctx context.Context, level slog.Level) bool {
	return s.logger.CanPrint(level + 3)
}

The "bad" news is that pterm.LogLevelDisabled equals 0 and a slog level higher than Info doesn't match pterm's log level as pterm doesn't have gaps between Info and Warn like slog has, so we can't just add 3 there.

For the former problem, passing slog.Level(-3) matches pterm.LogLevelDisabled (=0) and that would cause nothing to be printed, so we'll need to address that.

For the latter problem either the Enabled func needs to handle this case, or pterm's levels also introduces gaps (which may be a breaking change).

Example (haven't tested this)

// Enabled returns true if the given level is enabled.
func (s *SlogHandler) Enabled(ctx context.Context, level slog.Level) bool {
	switch {
	case level <= 0:
		return s.logger.CanPrint(level + 3)	
	case level < 4:
		return s.logger.CanPrint(LogLevelInfo)
	case level < 8:
		return s.logger.CanPrint(LogLevelWarn)
	}
	return s.logger.CanPrint(LogLevelError)
}

Also, for debugging or lower levels, we might prefer to print the level with the value, e.g. TRACE-2 or just LEVEL-2. See for example how slog does it:

https://github.com/golang/go/blob/6018ad99a4a951581b2d846a8ccd6f1d4e74fd11/src/log/slog/level.go#L59-L77

Also, styling would be adjusted for their base level, e.g. use debug style for slog levels -3, -2 and -1, while using Trace style for slog levels below -4.

My list of changes isn't complete, there may be other changes required to achieve this.

Disabled log level

Set the value of LogLevelDisabled to a arbitrary high value (something after LogLevelPrint I guess), or even math.MaxInt. By logic higher values produce less and less of logs, to the point where nothing is printed. Or some other form of disabling logging, e.g. remove this "level" and create a dedicated SetDisabled func or encourage users to use a no-op handler. Depending on implementation, this may be a breaking change.

Background

My app is using the go-logr framework with pterm as the implementation backend. I love pterm's colored output and thus created plogr to marry go-logr with pterm. This was back before slog was a thing. Now both go-logr and pterm support slog in-between so basically my stack looks like this: logr -> slog -> pterm (which makes plogr obsolete).

My app uses for example logs like this:

log.V(1).Info("Opening DB", "dir", c.getTargetPath())

// or
log.V(1).Info("Awaiting response")
...
log.V(2).Info("Read response", "body", string(b))
...
log.V(1).Info("Parsed response", "json", result)

A good argument for arbitrary log levels are cases like this: Business layer uses info/debug level (0 or 1 resp.). While it uses a REST client, the client's using level 2 for logging responses, and increased verbosity with level 3 or 4 would even print request headers for example. debug and trace would not be enough levels for this kind of verbosity increase.
(Just to be clear, I'm not saying debug and trace levels are bad and should be gone or anything, as this is too much of a debate of opinions. I'm just proposing an improved implementation for the slog handler as envisioned by the slog designers.)

@ccremer ccremer added the proposal Proposal to add a new feature to pterm label Jan 2, 2024
@MarvinJWendt MarvinJWendt added the proposal-accepted Proposals which are accepted for implementation in the future label Jan 3, 2024
@MarvinJWendt
Copy link
Member

Hi @ccremer thanks for the very detailed proposal! I totally agree with you, that our slog implementation does need some improvements. For example, the PTerm Logger does not support groups, which means that slog groups are completely ignored currently. I kinda rushed the slog implementation, as I always wanted a standardized way for structured logging in Go, but I didn't want to miss on PTerms colored output (I really like PTerms Logger).

For me, it would be fine to make this a breaking change, as we have a couple breaking changes queued up, which we also want to implement soon™️. After all, we had 73 releases without a breaking change now. If we can do it without breaking anything - even better.

I will take a closer look at this in the next days (hopefully)!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal Proposal to add a new feature to pterm proposal-accepted Proposals which are accepted for implementation in the future
Projects
None yet
Development

No branches or pull requests

2 participants