diff options
author | Patrick Steinhardt <psteinhardt@gitlab.com> | 2023-08-16 11:15:07 +0300 |
---|---|---|
committer | Patrick Steinhardt <psteinhardt@gitlab.com> | 2023-09-05 08:40:40 +0300 |
commit | fea96d943f4948e7fe1777904668a6acc02ed375 (patch) | |
tree | 31e433df85c3462c9396df9f581a45c0de5347fb | |
parent | 9ca8b86102188f14e9eeb600127d0ecd15eaea8c (diff) |
cli/praefect: Fix logging setup for client-side subcommands
Many of the Praefect subcommands do not set up the logger, but
immediately request them by calling `log.Default()`. This has multiple
consequences:
- The log level is fixed at "info" level, which is potentially quite
noisy.
- The gRPC logger isn't replaced properly, which has the consequence
that the logger is overly verbose. Furthermore, we do not pay
attention to the "GRPC_GO_LOG_SEVERITY_LEVEL" environment variable
as we should be doing.
- The logger will print its output to standard output. This breaks
the ability to consume the output of some commands whose output is
intended to be machine-parsable.
Fix these issues by calling `log.Configure()` instead of `log.Default()`
such that the logging infrastructure is getting properly initialized.
Note that we don't use the configuration to set up the log format and
level. This is intentional, because changing the server's configuration
should not influence how clients perform logs.
In any case, the loggers are configured to output to standard error
instead of standard output to fix (3). Furthermore, the verbosity of the
logger is decreased to "error" level to address (1) and (2).
We could either add client-specific logging configuration to Praefect's
configuration or alternatively add switches to the commands to configure
logging ad-hoc. For the time being though we just care about fixing the
above issues, so this is left for a later point in time if we see that
there is indeed demand for such flexibility.
Changelog: fixed
5 files changed, 24 insertions, 5 deletions
diff --git a/internal/cli/praefect/subcmd_dataloss.go b/internal/cli/praefect/subcmd_dataloss.go index 6b904d5bb..a97e8e2d6 100644 --- a/internal/cli/praefect/subcmd_dataloss.go +++ b/internal/cli/praefect/subcmd_dataloss.go @@ -3,6 +3,7 @@ package praefect import ( "fmt" "io" + "os" "sort" "strings" @@ -62,7 +63,10 @@ func datalossAction(ctx *cli.Context) error { return err } - logger := log.Default() + logger, err := log.Configure(os.Stderr, "text", "error") + if err != nil { + return fmt.Errorf("configuring logger: %w", err) + } includePartiallyAvailable := ctx.Bool("partially-unavailable") diff --git a/internal/cli/praefect/subcmd_list_untracked_repositories.go b/internal/cli/praefect/subcmd_list_untracked_repositories.go index 6685f660d..0551061c1 100644 --- a/internal/cli/praefect/subcmd_list_untracked_repositories.go +++ b/internal/cli/praefect/subcmd_list_untracked_repositories.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "io" + "os" "time" "github.com/urfave/cli/v2" @@ -68,7 +69,10 @@ func listUntrackedRepositoriesAction(appCtx *cli.Context) error { return err } - logger := log.Default() + logger, err := log.Configure(os.Stderr, "text", "error") + if err != nil { + return fmt.Errorf("configuring logger: %w", err) + } onlyIncludeOlderThan := appCtx.Duration("older-than") delimiter := appCtx.String("delimiter") diff --git a/internal/cli/praefect/subcmd_remove_repository.go b/internal/cli/praefect/subcmd_remove_repository.go index e095a60b4..1efdd701b 100644 --- a/internal/cli/praefect/subcmd_remove_repository.go +++ b/internal/cli/praefect/subcmd_remove_repository.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "io" + "os" "sync" "time" @@ -78,7 +79,10 @@ func removeRepositoryAction(appCtx *cli.Context) error { return err } - logger := log.Default() + logger, err := log.Configure(os.Stderr, "text", "error") + if err != nil { + return fmt.Errorf("configuring logger: %w", err) + } ctx := appCtx.Context openDBCtx, cancel := context.WithTimeout(ctx, 30*time.Second) diff --git a/internal/cli/praefect/subcmd_track_repositories.go b/internal/cli/praefect/subcmd_track_repositories.go index 863e8f375..4c942cc35 100644 --- a/internal/cli/praefect/subcmd_track_repositories.go +++ b/internal/cli/praefect/subcmd_track_repositories.go @@ -63,7 +63,10 @@ func trackRepositoriesAction(appCtx *cli.Context) error { return err } - logger := log.Default() + logger, err := log.Configure(os.Stderr, "text", "error") + if err != nil { + return fmt.Errorf("configuring logger: %w", err) + } db, clean, err := openDB(conf.DB, appCtx.App.ErrWriter) if err != nil { diff --git a/internal/cli/praefect/subcmd_track_repository.go b/internal/cli/praefect/subcmd_track_repository.go index 54a0a47f0..576913f88 100644 --- a/internal/cli/praefect/subcmd_track_repository.go +++ b/internal/cli/praefect/subcmd_track_repository.go @@ -7,6 +7,7 @@ import ( "fmt" "io" "math/rand" + "os" "time" "github.com/sirupsen/logrus" @@ -91,7 +92,10 @@ func trackRepositoryAction(appCtx *cli.Context) error { return err } - logger := log.Default() + logger, err := log.Configure(os.Stderr, "text", "error") + if err != nil { + return fmt.Errorf("configuring logger: %w", err) + } virtualStorage := appCtx.String(paramVirtualStorage) relativePath := appCtx.String(paramRelativePath) |