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

Support custom log levels in slog.Handler Implementation #116

Closed
lvlcn-t opened this issue Apr 4, 2024 · 2 comments · Fixed by #117
Closed

Support custom log levels in slog.Handler Implementation #116

lvlcn-t opened this issue Apr 4, 2024 · 2 comments · Fixed by #117
Assignees
Labels
bug Something isn't working

Comments

@lvlcn-t
Copy link
Contributor

lvlcn-t commented Apr 4, 2024

Issue

When implementing an issue for my logging library, I encountered an issue with your slog.Handler implementation. Since v0.4.0 introduced custom levels in combination with using your Level type, I noticed that the slog handler implementation does not support custom log levels. I'd like to see this feature extended to the slog handler implementation as well.

Problem Description

Currently your implementation of the slog.Handler interface, the conversion from slog.Level to log.Level is handled through a static map, fromSlogLevel. This map explicitly associates slog's default log levels to your own levels. However, this direct mapping presents a limitation when attempting to extend logging levels beyond the defaults provided by you and slog, as any new custom levels are not included in this map, causing the map lookup to fail. This issue does not result in a panic, but it prevents the correct handling of records with custom log levels.

This limitation hinders the development of logging libraries that are built on top of slog and also use charmbracelet/log, as they are forced to implement workarounds like this, to ensure that extended log levels are recognized and handled appropriately:

type Level = slog.Level

const LevelFatal Level = slog.Level(16)

type logger struct { *slog.Logger }

// Fatal logs at [LevelFatal] and then calls os.Exit(1).
func (l *logger) Fatal(msg string, args ...any) {
	// This is necessary since the log.Logger slog implementation only recognizes the default slog log levels.
	// So this means we need to use their own Log method so that the log level is correctly set.
	if h, ok := l.Handler().(*log.Logger); ok {
		h.Log(log.Level(LevelFatal), msg, args...)
	} else {
		l.logAttrs(context.Background(), LevelFatal, msg, args...)
	}
	os.Exit(1)
}

This workaround is not ideal, as it adds complexity to codebases and requires to manually handle the conversion of custom log levels. It would be more beneficial if you could directly support custom log levels without the need for such workarounds.

Solution Proposal

To address this issue and enhance the flexibility of log level extension, I propose modifying the way of handling slog levels. Instead of relying on a static map for level conversion, you could dynamically cast the provided slog.Level (of base type int) to your own Level type (base type int32). This approach would allow for a more generic handling of log levels, accommodating both the default levels and any extended levels introduced by developers. Here is a conceptual outline of how the level conversion may be adapted:

// Example adjustment in the Handle method or similar
level := Level(int32(record.Level)) // Directly cast from slog.Level (int) to log.Level (int32)

This solution would simplify the process of integrating extended log levels, eliminating the need for workarounds and fostering greater flexibility and creativity in logging practices.

Additional Context

https://github.com/charmbracelet/log/blob/main/level_121.go#L9
https://github.com/charmbracelet/log/blob/main/logger_121.go#L36

@aymanbagabas aymanbagabas added the bug Something isn't working label Apr 5, 2024
@aymanbagabas
Copy link
Member

Hy @lvlcn-t, thanks for reporting this issue. I think the proposed proposal is totally reasonable and should be part of Log. With that being said, PRs are always welcome at Charm, feel free to send a patch if you feel like it 🙂

@lvlcn-t
Copy link
Contributor Author

lvlcn-t commented Apr 5, 2024

@aymanbagabas Sure, I'd be happy to contribute, so feel free to assign me this issue 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants