From 2725aa205f47929be2f1678b451f9ec06dc93ba6 Mon Sep 17 00:00:00 2001 From: KaviiSuri Date: Mon, 23 Sep 2024 22:20:07 +0530 Subject: [PATCH 1/3] fix: logger changes --- internal/logger/zerolog.go | 105 ++++++++++++++++++++++++------------- 1 file changed, 69 insertions(+), 36 deletions(-) diff --git a/internal/logger/zerolog.go b/internal/logger/zerolog.go index faf669ab5..323185908 100644 --- a/internal/logger/zerolog.go +++ b/internal/logger/zerolog.go @@ -3,6 +3,7 @@ package logger import ( "context" "log/slog" + "time" "github.com/rs/zerolog" ) @@ -24,47 +25,14 @@ func newZerologHandler(logger *zerolog.Logger) *ZerologHandler { //nolint:gocritic // The slog.Record struct triggers hugeParam but we don't control the interface (it's a standard library one) func (h *ZerologHandler) Handle(ctx context.Context, record slog.Record) error { event := h.logger.WithLevel(mapLevel(record.Level)) - record.Attrs(func(attr slog.Attr) bool { - switch attr.Value.Kind() { - case slog.KindString: - event = event.Str(attr.Key, attr.Value.String()) - case slog.KindInt64: - event = event.Int64(attr.Key, attr.Value.Int64()) - case slog.KindFloat64: - event = event.Float64(attr.Key, attr.Value.Float64()) - case slog.KindBool: - event = event.Bool(attr.Key, attr.Value.Bool()) - default: - switch v := attr.Value.Any().(type) { - // error is a special case since zerlog has a dedicated method for it but it's not part of the slog.Kind - case error: - event = event.Err(v) - default: - event = event.Interface(attr.Key, attr.Value.Any()) - } - } + addAttrToZerolog(attr, event) return true }) - event.Msg(record.Message) return nil } -// mapLevel maps slog levels to zerolog levels -func mapLevel(level slog.Level) zerolog.Level { - switch { - case level == slog.LevelDebug: - return zerolog.DebugLevel - case level == slog.LevelWarn: - return zerolog.WarnLevel - case level == slog.LevelError: - return zerolog.ErrorLevel - default: - return zerolog.InfoLevel - } -} - // Enabled implements the slog.Handler interface func (h *ZerologHandler) Enabled(ctx context.Context, level slog.Level) bool { return h.logger.GetLevel() <= mapLevel(level) @@ -72,10 +40,11 @@ func (h *ZerologHandler) Enabled(ctx context.Context, level slog.Level) bool { // WithAttrs adds attributes to the log event func (h *ZerologHandler) WithAttrs(attrs []slog.Attr) slog.Handler { - logger := *h.logger + context := h.logger.With() for _, attr := range attrs { - logger = logger.With().Interface(attr.Key, attr.Value).Logger() + context = addAttrToZerolog(attr, context) } + logger := context.Logger() return newZerologHandler(&logger) } @@ -84,3 +53,67 @@ func (h *ZerologHandler) WithGroup(name string) slog.Handler { logger := h.logger.With().Str("group", name).Logger() return newZerologHandler(&logger) } + +// addAttrToZerolog is a generic function to add an slog.Attr to either a zerolog.Event or zerolog.Context +func addAttrToZerolog[T interface { + Str(string, string) T + Int64(string, int64) T + Uint64(string, uint64) T + Float64(string, float64) T + Bool(string, bool) T + Err(error) T + Time(string, time.Time) T + Dur(string, time.Duration) T + Interface(string, any) T + AnErr(key string, err error) T +}](attr slog.Attr, target T) T { + switch attr.Value.Kind() { + case slog.KindBool: + return target.Bool(attr.Key, attr.Value.Bool()) + case slog.KindDuration: + return target.Dur(attr.Key, attr.Value.Duration()) + case slog.KindFloat64: + return target.Float64(attr.Key, attr.Value.Float64()) + case slog.KindInt64: + return target.Int64(attr.Key, attr.Value.Int64()) + case slog.KindString: + return target.Str(attr.Key, attr.Value.String()) + case slog.KindTime: + return target.Time(attr.Key, attr.Value.Time()) + case slog.KindUint64: + return target.Uint64(attr.Key, attr.Value.Uint64()) + case slog.KindGroup: + // For group, we need to recurse into the group's attributes + group := attr.Value.Group() + for _, groupAttr := range group { + target = addAttrToZerolog(groupAttr, target) + } + return target + case slog.KindLogValuer: + // LogValuer is a special case that needs to be resolved + resolved := attr.Value.Resolve() + return addAttrToZerolog(slog.Attr{Key: attr.Key, Value: resolved}, target) + default: + switch v := attr.Value.Any().(type) { + // error is a special case since zerlog has a dedicated method for it but it's not part of the slog.Kind + case error: + return target.AnErr(attr.Key, v) + default: + return target.Interface(attr.Key, attr.Value) + } + } +} + +// mapLevel maps slog levels to zerolog levels +func mapLevel(level slog.Level) zerolog.Level { + switch { + case level == slog.LevelDebug: + return zerolog.DebugLevel + case level == slog.LevelWarn: + return zerolog.WarnLevel + case level == slog.LevelError: + return zerolog.ErrorLevel + default: + return zerolog.InfoLevel + } +} From 92e7b1bea8446a3754889975f047ce5965c0c7ec Mon Sep 17 00:00:00 2001 From: KaviiSuri Date: Tue, 24 Sep 2024 00:08:37 +0530 Subject: [PATCH 2/3] fix: macos forking behaviour was broken --- internal/eval/eval_darwin_arm64.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/internal/eval/eval_darwin_arm64.go b/internal/eval/eval_darwin_arm64.go index 0d13a5d5c..3a9421ac1 100644 --- a/internal/eval/eval_darwin_arm64.go +++ b/internal/eval/eval_darwin_arm64.go @@ -28,13 +28,16 @@ func EvalBGREWRITEAOF(args []string, store *dstore.Store) []byte { if config.EnableMultiThreading { return nil } - pid, _, err := syscall.RawSyscall(syscall.SYS_FORK, 0, 0, 0) + originalPID := syscall.Getpid() // Get the original PID (Process ID) - This is needed to check if we are in child process or not. The document approach is to check the return value of fork (it's 0 for child and non-zero for parent) but this is more reliable. For more details check - https://github.com/DiceDB/dice/issues/683 + _, _, err := syscall.RawSyscall(syscall.SYS_FORK, 0, 0, 0) if err != 0 { return diceerrors.NewErrWithMessage("Fork failed") } - if pid == 0 { + isChild := syscall.Getppid() == originalPID + + if isChild { // We are inside child process now, so we'll start flushing to disk. if err := dstore.DumpAllAOF(store); err != nil { return diceerrors.NewErrWithMessage("AOF failed") From ed5a0f1c9c12ba9ffca6ddf6e6371fd926bc82a0 Mon Sep 17 00:00:00 2001 From: KaviiSuri Date: Tue, 24 Sep 2024 01:52:37 +0530 Subject: [PATCH 3/3] fix: lint issues --- internal/eval/eval_darwin_arm64.go | 6 +++++- internal/logger/zerolog.go | 6 +++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/internal/eval/eval_darwin_arm64.go b/internal/eval/eval_darwin_arm64.go index 3a9421ac1..94cd73b5d 100644 --- a/internal/eval/eval_darwin_arm64.go +++ b/internal/eval/eval_darwin_arm64.go @@ -28,7 +28,11 @@ func EvalBGREWRITEAOF(args []string, store *dstore.Store) []byte { if config.EnableMultiThreading { return nil } - originalPID := syscall.Getpid() // Get the original PID (Process ID) - This is needed to check if we are in child process or not. The document approach is to check the return value of fork (it's 0 for child and non-zero for parent) but this is more reliable. For more details check - https://github.com/DiceDB/dice/issues/683 + + // Get the original PID (Process ID) - This is needed to check if we are in child process or not. + // The document approach is to check the return value of fork (it's 0 for child and non-zero for parent) but this is more reliable. + // For more details check - https://github.com/DiceDB/dice/issues/683 + originalPID := syscall.Getpid() _, _, err := syscall.RawSyscall(syscall.SYS_FORK, 0, 0, 0) if err != 0 { diff --git a/internal/logger/zerolog.go b/internal/logger/zerolog.go index 323185908..39d21de4b 100644 --- a/internal/logger/zerolog.go +++ b/internal/logger/zerolog.go @@ -40,11 +40,11 @@ func (h *ZerologHandler) Enabled(ctx context.Context, level slog.Level) bool { // WithAttrs adds attributes to the log event func (h *ZerologHandler) WithAttrs(attrs []slog.Attr) slog.Handler { - context := h.logger.With() + ctx := h.logger.With() for _, attr := range attrs { - context = addAttrToZerolog(attr, context) + ctx = addAttrToZerolog(attr, ctx) } - logger := context.Logger() + logger := ctx.Logger() return newZerologHandler(&logger) }