refactor: WIP: #1324 Slog refactor #1802
Conversation
Signed-off-by: Dave Josephsen <dave.josephsen@gmail.com>
Signed-off-by: Adam H <35571609+aharab@users.noreply.github.com>
✅ Deploy Preview for polite-licorice-3db33c canceled.
|
dosubot
bot
added
the
size:XXL
Summary of ChangesHello @djosephsen, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request initiates a significant refactoring of the project's logging infrastructure by introducing a Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
djosephsen
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a significant refactoring of the logging system, introducing a Logger interface to abstract away the underlying implementation (Zap or slog). This is a great step towards making the codebase more flexible. I've identified several issues in the new logger implementations, particularly around attribute handling and configuration, which need to be addressed to ensure the new system is robust and functions as intended. There are also a few minor issues प्रोटीन the rest of the codebase related to these changes.
|
|
||
| func TestGetState_Valid_ContainsFlag(t *testing.T) { | ||
| evaluator := flagdEvaluator.NewJSON(logger.NewLogger(nil, false), store.NewFlags()) | ||
| evaluator := flagdEvaluator.NewJSON(logger.New("slog",false,"json") |
There was a problem hiding this comment.
There is a syntax error on this line; a closing parenthesis ) is missing.
evaluator := flagdEvaluator.NewJSON(logger.New("slog",false,"json"))| func makeFields(args ...any) []zap.Field { | ||
| // if there are an odd number of | ||
| if len(args)%2 != 0 { | ||
| lastArg := args[len(args)-1] | ||
| args[len(args)-1] = "BADKEY!" | ||
| args = append(args, lastArg) | ||
| } | ||
| out := make([]zap.Field, len(args)) | ||
| for i := 0; i < len(args); i += 2 { | ||
| var k string | ||
| switch args[i].(type) { | ||
| case string: | ||
| k = k | ||
| default: | ||
| k = "!BADKEY" | ||
| } | ||
| val := args[i+1] | ||
| out = append(out, zap.Any(k, val)) | ||
| } | ||
| return out | ||
| } |
There was a problem hiding this comment.
The makeFields function has a few issues that will cause incorrect behavior:
- Incorrect key assignment: On line 61,
k = kis used. This should bek = v(wherevis from the type switchv := args[i].(type)). This will result in empty keys for string-keyed fields. - Incorrect slice initialization: On line 56,
out := make([]zap.Field, len(args))pre-allocates a slice withlen(args)zero-value elements. Then,appendis used in the loop, which adds new elements to the end. This will result in a slice with leadingnilfields. It should be initialized asout := make([]zap.Field, 0, len(args)/2). - Does not handle
zap.Fieldarguments: The function assumes key-value pairs ofstring, any. However, many calls in the codebase passzap.Fieldstructs directly (e.g.,logger.With(zap.String(...))). The current implementation will incorrectly handle these, resulting in!BADKEYfields. You should add logic to handlezap.Fieldarguments.
func makeFields(args ...any) []zap.Field {
if len(args) == 0 {
return nil
}
if _, ok := args[0].(zap.Field); ok {
fields := make([]zap.Field, 0, len(args))
for _, arg := range args {
if f, ok := arg.(zap.Field); ok {
fields = append(fields, f)
}
}
return fields
}
// if there are an odd number of
if len(args)%2 != 0 {
lastArg := args[len(args)-1]
args[len(args)-1] = "BADKEY!"
args = append(args, lastArg)
}
out := make([]zap.Field, 0, len(args)/2)
for i := 0; i < len(args); i += 2 {
var k string
switch v := args[i].(type) {
case string:
k = v
default:
k = "!BADKEY"
}
val := args[i+1]
out = append(out, zap.Any(k, val))
}
return out
}| func (l *slogger) DebugWithID(reqID string, msg string, args ...any) { | ||
| if !l.reqIDLogging { | ||
| return | ||
| } | ||
| attrs := append(makeAttrs(args...), l.getAttrs(reqID)...) | ||
| l.Logger.LogAttrs(context.TODO(), slog.LevelDebug, msg, attrs...) | ||
| } |
There was a problem hiding this comment.
The DebugWithID method is missing attributes from With() calls (stored in l.attrs) and does not add the reqID as a log attribute. All contextual attributes should be included in the log output. This applies to all ...WithID methods in this file.
func (l *slogger) DebugWithID(reqID string, msg string, args ...any) {
if !l.reqIDLogging {
return
}
attrs := append(makeAttrs(args...), l.getAttrs(reqID)...)
attrs = append(attrs, l.attrs...)
attrs = append(attrs, slog.String(RequestIDFieldName, reqID))
l.Logger.LogAttrs(context.TODO(), slog.LevelDebug, msg, attrs...)
}| func (l *slogger) Debug(msg string, args ...any) { | ||
| l.Logger.LogAttrs(context.TODO(), slog.LevelDebug, msg, makeAttrs(args...)...) | ||
| } |
There was a problem hiding this comment.
The logging methods, such as Debug, do not include the attributes added via With() calls (stored in l.attrs). This means context is lost. You should append l.attrs to the attributes being logged. This applies to all logging methods in this file (Info, Warn, Error, etc.).
func (l *slogger) Debug(msg string, args ...any) {
attrs := append(makeAttrs(args...), l.attrs...)
l.Logger.LogAttrs(context.TODO(), slog.LevelDebug, msg, attrs...)
}| requestAttrs: &sync.Map{}, | ||
| Logger: sl, | ||
| attrs: []slog.Attr{}, | ||
| reqIDLogging: false, |
There was a problem hiding this comment.
The reqIDLogging field is hardcoded to false, which disables request-ID-based logging for slog. This should be configurable, likely based on the Debug flag passed to logger.New, similar to zapLogger.
reqIDLogging: true, // This should be made configurable| func (l *slogger) With(attrs ...any) Logger { | ||
| return &slogger{ | ||
| Logger: l.Logger, | ||
| requestAttrs: l.requestAttrs, | ||
| attrs: makeAttrs(attrs), | ||
| reqIDLogging: l.reqIDLogging, | ||
| } | ||
| } |
There was a problem hiding this comment.
The With method overwrites l.attrs instead of appending to them. This means that chained With calls will lose context, e.g., logger.With("a", 1).With("b", 2) will only have the field b=2. The fields should be appended to preserve context from previous With calls.
func (l *slogger) With(attrs ...any) Logger {
newAttrs := make([]slog.Attr, len(l.attrs))
copy(newAttrs, l.attrs)
newAttrs = append(newAttrs, makeAttrs(attrs...)...)
return &slogger{
Logger: l.Logger,
requestAttrs: l.requestAttrs,
attrs: newAttrs,
reqIDLogging: l.reqIDLogging,
}
}| func (l *zapLogger) With(fields ...any) Logger { | ||
| return &zapLogger{ | ||
| Logger: l.Logger, | ||
| requestFields: l.requestFields, | ||
| fields: makeFields(fields), | ||
| reqIDLogging: l.reqIDLogging, | ||
| } | ||
| } |
There was a problem hiding this comment.
The With method overwrites the existing fields rather than appending to them. This means that chained With calls will lose context, e.g., logger.With("a", 1).With("b", 2) will only have the field b=2. The fields should be appended to preserve context from previous With calls.
func (l *zapLogger) With(fields ...any) Logger {
newFields := make([]zap.Field, len(l.fields))
copy(newFields, l.fields)
newFields = append(newFields, makeFields(fields)...)
return &zapLogger{
Logger: l.Logger,
requestFields: l.requestFields,
fields: newFields,
reqIDLogging: l.reqIDLogging,
}
}| } | ||
| logger := logger.NewLogger(l, Debug) | ||
| rtLogger := logger.WithFields(zap.String("component", "start")) | ||
| logger := logger.New("slog", Debug, "json") |
There was a problem hiding this comment.
The log-format command-line flag is being ignored. The logger is created with a hardcoded "json" format. You should use viper.GetString(logFormatFlagName) to respect the user's configuration.
| logger := logger.New("slog", Debug, "json") | |
| logger := logger.New("slog", Debug, viper.GetString(logFormatFlagName)) |
2 participants
This PR
The intent of this pr is to action #1324 by moving the existing concrete logger type, which is implementation-dependent upon uber-go/zaplog, into an interface type which may be implemented by either go-native slog, or uber-go/zap (or any other logging subysystem the project may become enamored with in the future)
Included is the Interface, along with implementations for both the pre-existing zap-based logger, as well as an interface implementation for slog. The default constructor is set to use the slog subsystem. We have included a full suite of table-based tests for the slog implementation, to prove correctness and demonstrate the logger's use.
Related Issues
#1324
Notes
This is a work-in-progress PR intended to begin the conversation about how the eventual logging refactor should take place. (Adam and I plan to be available at the community meeting to discuss)
Follow-up Tasks
TODOs:
There are several places in the current flagd codebase where uber-go/zap is explicitly used by packages in a way that make the project dependent on zap, even outside the current concrete "logger" package, which was intended to abstract these away. These still need to be cleaned up, but that clean-up will depend on the direction the project decides to go for the final implementation of this interface. Examples:
Every method invocation to a logger that uses
WithFieldscurrently passes in an upstream type called azap.Field. We can clean this up in a number of ways. The current interface replacesWithFieldwith a method calledWithwhich takes variadicanyand can be extended to support zap.Field if we want to keep it.Various Tests in the telemetry builder and kubernetes packages create
zapcore.Observertypes which act like write buffers, enabling the tests to log into the buffer and then examine the buffer's content. The newly added slogger implementation simply takes an io.Writer instead, and can provide the same functionality, but we felt going through and fixing all of those tests was a tad beyond the scope of this WIP. When we're satisfied that this is the correct approach, we'll clean up those tests accordingly (90% of the test coverage for simple cases is already fixed in this PR).How to test
We've included unit tests for core/pkg/logger. These compile and run. The project also compiles and runs with the new default Logger interface with an slog implementation backing it.