[DRAFT] Replace zerolog with slog #284
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Abhi Kapoor <[email protected]>
Mergecat's ReviewClick to read mergecats review!😼 Mergecat review of pkg/config/config_test.go@@ -1,10 +1,10 @@
package config
import (
+ "log/slog"
"testing"
"time"
- "github.com/rs/zerolog"
"github.com/spf13/viper"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
@@ -22,7 +22,7 @@ func TestNew(t *testing.T) {
cfg, err := NewWithViper(v)
require.NoError(t, err)
- assert.Equal(t, zerolog.InfoLevel, cfg.LogLevel)
+ assert.Equal(t, slog.LevelInfo, cfg.LogLevel)
assert.Equal(t, true, cfg.ArgoCDInsecure)
assert.Equal(t, true, cfg.ArgoCDPlainText)
assert.Equal(t, pkg.StateWarning, cfg.WorstConfTestState, "worst states can be overridden")Feedback & Suggestions:
😼 Mergecat review of cmd/container.go@@ -3,9 +3,9 @@ package cmd
import (
"context"
"fmt"
+ "log/slog"
"github.com/pkg/errors"
- "github.com/rs/zerolog/log"
"github.com/zapier/kubechecks/pkg/app_watcher"
"github.com/zapier/kubechecks/pkg/appdir"
"github.com/zapier/kubechecks/pkg/argo_client"
@@ -94,7 +94,7 @@ func newContainer(ctx context.Context, cfg config.ServerConfig, watchApps bool)
go ctr.ApplicationSetWatcher.Run(ctx)
}
} else {
- log.Info().Msgf("not monitoring applications, MonitorAllApplications: %+v", cfg.MonitorAllApplications)
+ slog.Info(fmt.Sprintf("not monitoring applications, MonitorAllApplications: %+v", cfg.MonitorAllApplications))
}
return ctr, nilFeedback & Suggestions:
😼 Mergecat review of pkg/git/repo_test.go@@ -2,6 +2,7 @@ package git
import (
"context"
+ "fmt"
"os"
"os/exec"
"strings"
@@ -61,8 +62,10 @@ git add ghi.txt
git commit -m "commit three"
`)
cmd.Env = append(cmd.Env, "TEMPDIR="+originRepo)
+ fmt.Println("Env: ", cmd.Env)
cmd.Stderr = os.Stderr
cmd.Stdout = os.Stdout
+ fmt.Println("Script: ", cmd)
err = cmd.Run()
require.NoError(t, err)
Feedback & Suggestions:
😼 Mergecat review of cmd/locations.go@@ -2,14 +2,14 @@ package cmd
import (
"context"
+ "log/slog"
"net/url"
"path/filepath"
"regexp"
"strings"
"time"
"github.com/pkg/errors"
- "github.com/rs/zerolog/log"
"github.com/zapier/kubechecks/pkg"
"github.com/zapier/kubechecks/pkg/container"
@@ -65,11 +65,12 @@ func maybeCloneGitUrl(ctx context.Context, repoManager cloner, repoRefreshDurati
}
if err := repo.Update(ctx); err != nil {
- log.Warn().
- Err(err).
- Str("path", repo.Directory).
- Str("url", repo.CloneURL).
- Msg("failed to update repo")
+ slog.Warn(
+ "failed to update repo",
+ "error", err,
+ "path", repo.Directory,
+ "url", repo.CloneURL,
+ )
}
}
}()Feedback & Suggestions:
😼 Mergecat review of pkg/git/repo.go@@ -5,6 +5,7 @@ import (
"context"
"fmt"
"io/fs"
+ "log/slog"
"net/url"
"os"
"os/exec"
@@ -53,11 +54,11 @@ func (r *Repo) Clone(ctx context.Context) error {
return errors.Wrap(err, "failed to make temp dir")
}
- log.Info().
- Str("temp-dir", r.Directory).
- Str("clone-url", r.CloneURL).
- Str("branch", r.BranchName).
- Msg("cloning git repo")
+ slog.Info("cloing git repo",
+ "temp-dir", r.Directory,
+ "clone-url", r.CloneURL,
+ "branch", r.BranchName,
+ )
// Attempt to locally clone the repo based on the provided information stored within
_, span := tracer.Start(ctx, "CloneRepo")
@@ -71,7 +72,7 @@ func (r *Repo) Clone(ctx context.Context) error {
cmd := r.execCommand("git", args...)
out, err := cmd.CombinedOutput()
if err != nil {
- log.Error().Err(err).Msgf("unable to clone repository, %s", out)
+ slog.Error(fmt.Sprintf("unable to clone repository, %s", out), "error", err)
return err
}
Feedback & Suggestions:
By addressing these points, the code will be more consistent, readable, and maintainable. 😼 Mergecat review of cmd/process.go@@ -1,7 +1,8 @@
package cmd
import (
- "github.com/rs/zerolog/log"
+ "log/slog"
+
"github.com/spf13/cobra"
"github.com/zapier/kubechecks/pkg/config"
@@ -17,28 +18,28 @@ var processCmd = &cobra.Command{
cfg, err := config.New()
if (err != nil) {
- log.Fatal().Err(err).Msg("failed to generate config")
+ LogFatal(ctx, "failed to generate config", "error", err)
}
ctr, err := newContainer(ctx, cfg, false)
if (err != nil) {
- log.Fatal().Err(err).Msg("failed to create container")
+ LogFatal(ctx, "failed to create container", "error", err)
}
- log.Info().Msg("initializing git settings")
+ slog.Info("initializing git settings")
if (err = initializeGit(ctr); err != nil) {
- log.Fatal().Err(err).Msg("failed to initialize git settings")
+ LogFatal(ctx, "failed to initialize git settings", "error", err)
}
repo, err := ctr.VcsClient.LoadHook(ctx, args[0])
if (err != nil) {
- log.Fatal().Err(err).Msg("failed to load hook")
+ LogFatal(ctx, "failed to load hook", "error", err)
return
}
processors, err := getProcessors(ctr)
if (err != nil) {
- log.Fatal().Err(err).Msg("failed to create processors")
+ LogFatal(ctx, "failed to create processors", "error", err)
}
server.ProcessCheckEvent(ctx, repo, ctr, processors)Feedback & Suggestions:
😼 Mergecat review of pkg/checks/diff/diff.go@@ -5,6 +5,8 @@ import (
"encoding/json"
"fmt"
"io"
+ "log/slog"
+ "os"
"strings"
"time"
@@ -21,9 +23,8 @@ import (
"github.com/argoproj/gitops-engine/pkg/sync/ignore"
"github.com/argoproj/gitops-engine/pkg/utils/kube"
"github.com/ghodss/yaml"
- "github.com/go-logr/zerologr"
+ "github.com/go-logr/logr"
"github.com/pmezard/go-difflib/difflib"
- "github.com/rs/zerolog/log"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime/schema"
@@ -56,14 +57,17 @@ func Check(ctx context.Context, request checks.Request) (msg.Result, error) {
app := request.App
- log.Debug().Str("name", app.Name).Msg("generating diff for application...")
+ slog.Debug(
+ "generating diff for application...",
+ "name", app.Name,
+ )
items := make([]objKeyLiveTarget, 0)
var unstructureds []*unstructured.Unstructured
for _, mfst := range request.JsonManifests {
obj, err := argoappv1.UnmarshalToUnstructured(mfst)
if err != nil {
- log.Warn().Err(err).Msg("failed to unmarshal to unstructured")
+ slog.Warn("Failed to unmarshal to unstructured", "error", err)
continue
}
@@ -99,7 +103,7 @@ func Check(ctx context.Context, request checks.Request) (msg.Result, error) {
var added, modified, removed int
for _, item := range items {
resourceId := fmt.Sprintf("%s/%s %s/%s", item.key.Group, item.key.Kind, item.key.Namespace, item.key.Name)
- log.Trace().Str("resource", resourceId).Msg("diffing object")
+ slog.Log(ctx, slog.Level(-8), "diffing object", "resource", resourceId)
if item.target != nil && hook.IsHook(item.target) || item.live != nil && hook.IsHook(item.live) {
continue
@@ -171,7 +175,7 @@ func addResourceDiffToMessage(ctx context.Context, diffBuffer *strings.Builder,
live = item.live
if err := json.Unmarshal(diffRes.PredictedLive, target); err != nil {
telemetry.SetError(span, err, "JSON Unmarshall")
- log.Warn().Err(err).Msg("failed to unmarshall json")
+ slog.Warn("failed to unmarshall json", "error", err)
}
} else {
live = item.live
@@ -201,8 +205,10 @@ func generateDiff(ctx context.Context, request checks.Request, argoSettings *set
ignoreNormalizerOpts := normalizers.IgnoreNormalizerOpts{
JQExecutionTimeout: 1 * time.Second,
}
+ slogLogger := slog.New(slog.NewJSONHandler(os.Stdout, &slog.HandlerOptions{}))
+ slogCtx := logr.NewContextWithSlogLogger(ctx, slogLogger)
diffConfig, err := argodiff.NewDiffConfigBuilder().
- WithLogger(zerologr.New(&log.Logger)).
+ WithLogger(logr.FromContextOrDiscard(slogCtx)).
WithDiffSettings(request.App.Spec.IgnoreDifferences, overrides, ignoreAggregatedRoles, ignoreNormalizerOpts).
WithTracking(argoSettings.AppLabelKey, argoSettings.TrackingMethod).
WithNoCache().
@@ -260,17 +266,17 @@ var nilApp = argoappv1.Application{}
func isApp(item objKeyLiveTarget, manifests []byte) (argoappv1.Application, bool) {
if strings.ToLower(item.key.Group) != "argoproj.io" {
- log.Debug().Str("group", item.key.Group).Msg("group is not correct")
+ slog.Debug("group is not correct", "group", item.key.Group)
return nilApp, false
}
if strings.ToLower(item.key.Kind) != "application" {
- log.Debug().Str("kind", item.key.Kind).Msg("kind is not correct")
+ slog.Debug("kind is not correct", "kind", item.key.Kind)
return nilApp, false
}
var app argoappv1.Application
if err := json.Unmarshal(manifests, &app); err != nil {
- log.Warn().Err(err).Msg("failed to deserialize application")
+ slog.Warn("failed to unmarshal application", "error", err)
return nilApp, false
}
Feedback & Suggestions:
By addressing these points, the code will be more robust, maintainable, and secure. 🛡️🚀 😼 Mergecat review of cmd/controller_cmd.go@@ -2,13 +2,13 @@ package cmd
import (
"context"
+ "log/slog"
"os"
"os/signal"
"syscall"
"time"
_ "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
- "github.com/rs/zerolog/log"
"github.com/spf13/cobra"
"github.com/spf13/viper"
@@ -30,52 +30,53 @@ var ControllerCmd = &cobra.Command{
Run: func(cmd *cobra.Command, args []string) {
ctx := context.Background()
- log.Info().
- Str("git-tag", pkg.GitTag).
- Str("git-commit", pkg.GitCommit).
- Msg("Starting KubeChecks")
+ slog.Log(ctx, slog.LevelInfo, "Starting KubeChecks",
+ "git-tag", pkg.GitTag,
+ "git-commit", pkg.GitCommit,
+ )
- log.Info().Msg("parsing configuration")
+ slog.Info("parse configuration")
cfg, err := config.New()
if err != nil {
- log.Fatal().Err(err).Msg("failed to parse configuration")
+ LogFatal(ctx, "failed to parse configuration", "error", err)
}
ctr, err := newContainer(ctx, cfg, true)
if err != nil {
- log.Fatal().Err(err).Msg("failed to create container")
+ LogFatal(ctx, "failed to create container", "error", err)
}
- log.Info().Msg("initializing git settings")
+ slog.Info("initializing git settings")
if err = initializeGit(ctr); err != nil {
- log.Fatal().Err(err).Msg("failed to initialize git settings")
+ LogFatal(ctx, "failed to initialize git settings", "error", err)
}
if err = processLocations(ctx, ctr, cfg.PoliciesLocation); err != nil {
- log.Fatal().Err(err).Msg("failed to process policy locations")
+ LogFatal(ctx, "failed to process policy locations", "error", err)
}
if err = processLocations(ctx, ctr, cfg.SchemasLocations); err != nil {
- log.Fatal().Err(err).Msg("failed to process schema locations")
+ LogFatal(ctx, "failed to process schema locations", "error", err)
}
processors, err := getProcessors(ctr)
if err != nil {
- log.Fatal().Err(err).Msg("failed to create processors")
+ LogFatal(ctx, "failed to create processors", "error", err)
}
t, err := initTelemetry(ctx, cfg)
if err != nil {
- log.Panic().Err(err).Msg("Failed to initialize telemetry")
+ slog.Error("failed to initialize telemetry", "error", err)
+ panic(err)
}
defer t.Shutdown()
- log.Info().Msgf("starting web server")
+ slog.Info("starting app watcher")
startWebserver(ctx, ctr, processors)
- log.Info().Msgf("listening for requests")
+ slog.Info("listening for requests")
waitForShutdown()
- log.Info().Msg("shutting down gracefully")
+ slog.Info("shutting down gracefully")
waitForPendingRequest()
},
}
@@ -102,7 +103,7 @@ func initializeGit(ctr container.Container) error {
func waitForPendingRequest() {
for events.GetInFlight() > 0 {
- log.Info().Int("count", events.GetInFlight()).Msg("waiting for in-flight requests to complete")
+ slog.Info("waiting for in-flight requests to complete", "count", events.GetInFlight())
time.Sleep(time.Second * 3)
}
}
@@ -116,7 +117,7 @@ func waitForShutdown() {
go func() {
sig := <-sigs
- log.Debug().Str("signal", sig.String()).Msg("received signal")
+ slog.Debug("received signal", "signal", sig.String())
done <- true
}()
Feedback & Suggestions:
By addressing these points, you can ensure that the transition to 😼 Mergecat review of pkg/config/config.go@@ -1,13 +1,13 @@
package config
import (
+ "fmt"
+ "log/slog"
"reflect"
"time"
"github.com/mitchellh/mapstructure"
"github.com/pkg/errors"
- "github.com/rs/zerolog"
- "github.com/rs/zerolog/log"
"github.com/spf13/viper"
"github.com/zapier/kubechecks/pkg"
@@ -62,7 +62,7 @@ type ServerConfig struct {
// misc
FallbackK8sVersion string `mapstructure:"fallback-k8s-version"`
LabelFilter string `mapstructure:"label-filter"`
- LogLevel zerolog.Level `mapstructure:"log-level"`
+ LogLevel slog.Level `mapstructure:"log-level"`
MonitorAllApplications bool `mapstructure:"monitor-all-applications"`
OpenAIAPIToken string `mapstructure:"openai-api-token"`
RepoRefreshInterval time.Duration `mapstructure:"repo-refresh-interval"`
@@ -81,9 +81,15 @@ func NewWithViper(v *viper.Viper) (ServerConfig, error) {
var cfg ServerConfig
if err := v.Unmarshal(&cfg, func(config *mapstructure.DecoderConfig) {
config.DecodeHook = func(in reflect.Type, out reflect.Type, value interface{}) (interface{}, error) {
- if in.String() == "string" && out.String() == "zerolog.Level" {
input := value.(string)
- return zerolog.ParseLevel(input)
+ var level slog.Level
+ var err = level.UnmarshalText([]byte(input))
+ if err == nil {
+ return level, nil
+ } else {
+ return nil, err
+ }
}
if in.String() == "string" && out.String() == "pkg.CommitState" {
@@ -102,11 +108,11 @@ func NewWithViper(v *viper.Viper) (ServerConfig, error) {
return cfg, errors.Wrap(err, "failed to read configuration")
}
- log.Info().Msg("Server Configuration: ")
- log.Info().Msgf("Webhook URL Base: %s", cfg.WebhookUrlBase)
- log.Info().Msgf("Webhook URL Prefix: %s", cfg.UrlPrefix)
- log.Info().Msgf("VCS Type: %s", cfg.VcsType)
- log.Info().Msgf("ArgoCD Namespace: %s", cfg.ArgoCDNamespace)
+ slog.Info("Server Configuration: ")
+ slog.Info(fmt.Sprintf("Webhook URL Base: %s", cfg.WebhookUrlBase))
+ slog.Info(fmt.Sprintf("Webhook URL Prefix: %s", cfg.UrlPrefix))
+ slog.Info(fmt.Sprintf("VCS Type: %s", cfg.VcsType))
+ slog.Info(fmt.Sprintf("ArgoCD Namespace: %s", cfg.ArgoCDNamespace))
return cfg, nil
}Feedback & Suggestions:
😼 Mergecat review of cmd/root.go package cmd
import (
+ "context"
+ "log/slog"
"os"
"strings"
- "github.com/rs/zerolog"
- "github.com/rs/zerolog/log"
"github.com/sirupsen/logrus"
"github.com/spf13/cobra"
"github.com/spf13/viper"
@@ -29,6 +29,20 @@ const envPrefix = "kubechecks"
var envKeyReplacer = strings.NewReplacer("-", "_")
+const (
+ LevelTrace = slog.Level(-8)
+ LevelFatal = slog.Level(12)
+)
+
+var LevelNames = map[slog.Level]string{
+ slog.LevelError: "ERROR",
+ slog.LevelWarn: "WARN",
+ slog.LevelInfo: "INFO",
+ slog.LevelDebug: "DEBUG",
+ LevelTrace: "TRACE",
+ LevelFatal: "FATAL",
+}
+
func init() {
// allows environment variables to use _ instead of -
viper.SetEnvKeyReplacer(envKeyReplacer) // sync-provider becomes SYNC_PROVIDER
@@ -39,13 +53,15 @@ func init() {
stringFlag(flags, "log-level", "Set the log output level.",
newStringOpts().
withChoices(
- zerolog.LevelErrorValue,
- zerolog.LevelWarnValue,
- zerolog.LevelInfoValue,
- zerolog.LevelDebugValue,
- zerolog.LevelTraceValue,
+ func() []string {
+ values := make([]string, 0, len(LevelNames))
+ for (_, value := range LevelNames {
+ values = append(values, value)
+ }
+ return values
+ }()...,
).
- withDefault("info").
+ withDefault("INFO").
withShortHand("l"),
)
boolFlag(flags, "persist-log-level", "Persists the set log level down to other module loggers.")
@@ -115,30 +131,66 @@ func init() {
}
func setupLogOutput() {
- output := zerolog.ConsoleWriter{Out: os.Stdout}
- log.Logger = log.Output(output)
-
- // Default level is info, unless debug flag is present
- levelFlag := viper.GetString("log-level")
- level, _ := zerolog.ParseLevel(levelFlag)
+ ctx := context.Background()
+ logLevel := &slog.LevelVar{} // INFO
+ opts := &slog.HandlerOptions{
+ Level: logLevel,
+ // Map custom log levels to their string representation
+ ReplaceAttr: func(groups []string, a slog.Attr) slog.Attr {
+ if a.Key == slog.LevelKey {
+ level := a.Value.Any().(slog.Level)
+ levelLabel, exists := LevelNames[level]
+ if !exists {
+ levelLabel = level.String()
+ }
+
+ a.Value = slog.StringValue(levelLabel)
+ }
+
+ return a
+ },
+ }
+ logger := slog.New(slog.NewJSONHandler(os.Stdout, opts))
+
+ // Retrieve log level from viper
+ levelFlag := strings.ToUpper(viper.GetString("log-level"))
+ level, err := ParseLevel(levelFlag)
+ if err != nil {
+ logLevel.Set(level)
+ } else {
+ logLevel.Set(slog.LevelInfo)
+ }
- zerolog.SetGlobalLevel(level)
- log.Debug().Msg("Debug level logging enabled.")
- log.Trace().Msg("Trace level logging enabled.")
- log.Info().Msg("Initialized logger.")
+ slog.SetDefault(logger)
+ logger.Debug("Debug level logging enabled.")
+ logger.Log(ctx, LevelTrace, "Trace level logging enabled.")
+ logger.Info("Initialized logger.")
// set logrus log level to overwrite the logs exporting from argo-cd package
logrusLevel := logrus.ErrorLevel
if viper.GetBool("persist_log_level") {
- if log.Debug().Enabled() {
logrusLevel = logrus.DebugLevel
}
- if log.Trace().Enabled() {
+ if logger.Enabled(ctx, slog.LevelDebug) {
+ logrusLevel = logrus.DebugLevel
+ }
+ if logger.Enabled(ctx, LevelTrace) {
logrusLevel = logrus.TraceLevel
}
}
logrus.StandardLogger().Level = logrusLevel
- log.Info().Str("log_level", logrus.StandardLogger().Level.String()).Msg("setting logrus log level")
+ logger.Info(
+ "log_level", logrus.StandardLogger().Level.String(),
+ "setting logrus log level",
+ )
+}
+
+func ParseLevel(s string) (slog.Level, error) {
+ var level slog.Level
+ var err = level.UnmarshalText([]byte(s))
+ return level, err
+}
+func LogFatal(ctx context.Context, msg string, keysAndValues ...interface{}) {
+ slog.Log(ctx, LevelFatal, msg, keysAndValues...)
+ os.Exit(1)
}Feedback & Suggestions:
By addressing these suggestions, the code will be more robust, maintainable, and secure. 🛠️ Dependency ReviewClick to read mergecats review!No suggestions found |
|
After looking at the difference between slog and zerolog, we'd be losing a bunch of functionality to remove the dependency: hooks, integrations, Trace and Fatal, etc. Unless you feel strongly about this one, I don't think we gain enough by swapping to slog. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.