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

feat: make logger easily replaceable #15358

Merged
merged 11 commits into from
Mar 13, 2023
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

### Improvements

* (server) [#15358](https://github.com/cosmos/cosmos-sdk/pull/15358) Add `server.CreateServerContextFromConfig` as alternative to `server.InterceptConfigsPreRunHandler` which does not set the server context and the default SDK logger.
* [#15011](https://github.com/cosmos/cosmos-sdk/pull/15011) Introduce `cosmossdk.io/log` package to provide a consistent logging interface through the SDK. CometBFT logger is now replaced by `cosmossdk.io/log.Logger`.
* (x/auth) [#14758](https://github.com/cosmos/cosmos-sdk/pull/14758) Allow transaction event queries to directly passed to Tendermint, which will allow for full query operator support, e.g. `>`.
* (server) [#15041](https://github.com/cosmos/cosmos-sdk/pull/15041) Remove unnecessary sleeps from gRPC and API server initiation. The servers will start and accept requests as soon as they're ready.
Expand Down
26 changes: 25 additions & 1 deletion docs/docs/core/07-cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -168,4 +168,28 @@ https://github.com/cosmos/cosmos-sdk/blob/v0.47.0-rc1/simapp/simd/cmd/root.go#L6

The `SetCmdClientContextHandler` call reads persistent flags via `ReadPersistentCommandFlags` which creates a `client.Context` and sets that on the root command's `Context`.

The `InterceptConfigsPreRunHandler` call creates a viper literal, default `server.Context`, and a logger and sets that on the root command's `Context`. The `server.Context` will be modified and saved to disk via the internal `interceptConfigs` call, which either reads or creates a Tendermint configuration based on the home path provided. In addition, `interceptConfigs` also reads and loads the application configuration, `app.toml`, and binds that to the `server.Context` viper literal. This is vital so the application can get access to not only the CLI flags, but also to the application configuration values provided by this file.
The `InterceptConfigsPreRunHandler` call creates a viper literal, default `server.Context`, and a logger and sets that on the root command's `Context`. The `server.Context` will be modified and saved to disk. The internal `interceptConfigs` call reads or creates a Tendermint configuration based on the home path provided. In addition, `interceptConfigs` also reads and loads the application configuration, `app.toml`, and binds that to the `server.Context` viper literal. This is vital so the application can get access to not only the CLI flags, but also to the application configuration values provided by this file.

:::tip
When willing to configure which logger is used, do not to use `InterceptConfigsPreRunHandler`, which sets the default SDK logger, but instead use `CreateServerContextFromConfig` and set the server context and the logger manually:

```diff
-return server.InterceptConfigsPreRunHandler(cmd, customAppTemplate, customAppConfig, customCMTConfig)

+serverCtx, err := server.CreateServerContextFromConfig(cmd, customAppTemplate, customAppConfig, customCMTConfig)
+if err != nil {
+ return err
+}

+// overwrite default server logger
+logger, err := server.CreateSDKLogger(serverCtx, cmd.OutOrStdout())
+if err != nil {
+ return err
+}
+serverCtx.Logger = logger.With(log.ModuleKey, "server")

+// set server context
+return server.SetCmdServerContext(cmd, serverCtx)
```

:::
2 changes: 1 addition & 1 deletion docs/rfc/README.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
---
sidebar_position: 0
sidebar_position: 1
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated: #15354 (comment)

---

# Requests for Comments
Expand Down
67 changes: 40 additions & 27 deletions server/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (
"os/signal"
"path"
"path/filepath"
"strconv"
"strings"
"syscall"
"time"
Expand Down Expand Up @@ -51,20 +50,11 @@ type Context struct {
Logger log.Logger
}

// ErrorCode contains the exit code for server exit.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was not used.

type ErrorCode struct {
Code int
}

func (e ErrorCode) Error() string {
return strconv.Itoa(e.Code)
}

func NewDefaultContext() *Context {
return NewContext(
viper.New(),
cmtcfg.DefaultConfig(),
log.NewLogger(os.Stdout), // TODO(mr): update NewDefaultContext to accept log destination.
log.NewLogger(os.Stdout),
)
}

Expand Down Expand Up @@ -106,7 +96,26 @@ func bindFlags(basename string, cmd *cobra.Command, v *viper.Viper) (err error)
return err
}

// InterceptConfigsPreRunHandler performs a pre-run function for the root daemon
// InterceptConfigsPreRunHandler is identical to CreateServerContextFromConfig
// except it also sets the server context on the command and the server logger.
func InterceptConfigsPreRunHandler(cmd *cobra.Command, customAppConfigTemplate string, customAppConfig interface{}, cmtConfig *cmtcfg.Config) error {
serverCtx, err := CreateServerContextFromConfig(cmd, customAppConfigTemplate, customAppConfig, cmtConfig)
if err != nil {
return err
}

// overwrite default server logger
logger, err := CreateSDKLogger(serverCtx, cmd.OutOrStdout())
if err != nil {
return err
}
serverCtx.Logger = logger.With(log.ModuleKey, "server")

// set server context
return SetCmdServerContext(cmd, serverCtx)
}

// CreateServerContextFromConfig performs a pre-run function for the root daemon
// application command. It will create a Viper literal and a default server
// Context. The server CometBFT configuration will either be read and parsed
// or created and saved to disk, where the server Context is updated to reflect
Expand All @@ -116,25 +125,25 @@ func bindFlags(basename string, cmd *cobra.Command, v *viper.Viper) (err error)
// is used to read and parse the application configuration. Command handlers can
// fetch the server Context to get the CometBFT configuration or to get access
// to Viper.
func InterceptConfigsPreRunHandler(cmd *cobra.Command, customAppConfigTemplate string, customAppConfig interface{}, cmtConfig *cmtcfg.Config) error {
func CreateServerContextFromConfig(cmd *cobra.Command, customAppConfigTemplate string, customAppConfig interface{}, cmtConfig *cmtcfg.Config) (*Context, error) {
Copy link
Member Author

@julienrbrt julienrbrt Mar 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think @tac0turtle this conveys enough what the function does? (which is more than creating a server context, as it calls interceptConfigs).

I feel like maybe InterceptConfigsPreRunHandlerNoLoggerNoSaveContext, while hideous, will convey more what this does. I am open for a better name too 😄.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I even think, this could be named InterceptConfigsPreRunHandler and have the other one be renamed to something more clear. However doing that is API breaking, so not ideal.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to InterceptConfigsAndCreateContext.

serverCtx := NewDefaultContext()

// Get the executable name and configure the viper instance so that environmental
// variables are checked based off that name. The underscore character is used
// as a separator.
executableName, err := os.Executable()
if err != nil {
return err
return nil, err
}

basename := path.Base(executableName)

// configure the viper instance
if err := serverCtx.Viper.BindPFlags(cmd.Flags()); err != nil {
return err
return nil, err
}
if err := serverCtx.Viper.BindPFlags(cmd.PersistentFlags()); err != nil {
return err
return nil, err
}

serverCtx.Viper.SetEnvPrefix(basename)
Expand All @@ -144,48 +153,52 @@ func InterceptConfigsPreRunHandler(cmd *cobra.Command, customAppConfigTemplate s
// intercept configuration files, using both Viper instances separately
config, err := interceptConfigs(serverCtx.Viper, customAppConfigTemplate, customAppConfig, cmtConfig)
if err != nil {
return err
return nil, err
}

// return value is a CometBFT configuration object
serverCtx.Config = config
if err = bindFlags(basename, cmd, serverCtx.Viper); err != nil {
return err
return nil, err
}

return serverCtx, nil
}

// CreateSDKLogger creates a the default SDK logger.
// It reads the log level and format from the server context.
func CreateSDKLogger(ctx *Context, out io.Writer) (log.Logger, error) {
var logger log.Logger
if serverCtx.Viper.GetString(flags.FlagLogFormat) == cmtcfg.LogFormatJSON {
zl := zerolog.New(cmd.OutOrStdout()).With().Timestamp().Logger()
if ctx.Viper.GetString(flags.FlagLogFormat) == cmtcfg.LogFormatJSON {
zl := zerolog.New(out).With().Timestamp().Logger()
logger = log.NewCustomLogger(zl)
} else {
logger = log.NewLogger(cmd.OutOrStdout())
logger = log.NewLogger(out)
}

// set filter level or keys for the logger if any
logLvlStr := serverCtx.Viper.GetString(flags.FlagLogLevel)
logLvlStr := ctx.Viper.GetString(flags.FlagLogLevel)
logLvl, err := zerolog.ParseLevel(logLvlStr)
if err != nil {
// If the log level is not a valid zerolog level, then we try to parse it as a key filter.
filterFunc, err := log.ParseLogLevel(logLvlStr, zerolog.InfoLevel.String())
if err != nil {
return fmt.Errorf("failed to parse log level (%s): %w", logLvlStr, err)
return nil, fmt.Errorf("failed to parse log level (%s): %w", logLvlStr, err)
}

logger = log.FilterKeys(logger, filterFunc)
} else {
zl := logger.Impl().(*zerolog.Logger)
// Check if the CometBFT flag for trace logging is set if it is then setup a tracing logger in this app as well.
// Note it overrides log level passed in `log_levels`.
if serverCtx.Viper.GetBool(cmtcli.TraceFlag) {
if ctx.Viper.GetBool(cmtcli.TraceFlag) {
logger = log.NewCustomLogger(zl.Level(zerolog.TraceLevel))
} else {
logger = log.NewCustomLogger(zl.Level(logLvl))
}
}

serverCtx.Logger = logger.With("module", "server")

return SetCmdServerContext(cmd, serverCtx)
return logger, nil
}

// GetServerContextFromCmd returns a Context from a command or an empty Context
Expand Down
10 changes: 7 additions & 3 deletions server/util_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,7 @@ var errCanceledInPreRun = errors.New("canceled in prerun")
// Used in each test to run the function under test via Cobra
// but to always halt the command
func preRunETestImpl(cmd *cobra.Command, args []string) error {
err := server.InterceptConfigsPreRunHandler(cmd, "", nil, cmtcfg.DefaultConfig())
if err != nil {
if err := server.InterceptConfigsPreRunHandler(cmd, "", nil, cmtcfg.DefaultConfig()); err != nil {
return err
}

Expand Down Expand Up @@ -435,7 +434,12 @@ func TestEmptyMinGasPrices(t *testing.T) {
// Run StartCmd.
cmd = server.StartCmd(nil, tempDir)
cmd.PreRunE = func(cmd *cobra.Command, _ []string) error {
return server.InterceptConfigsPreRunHandler(cmd, "", nil, cmtcfg.DefaultConfig())
ctx, err := server.CreateServerContextFromConfig(cmd, "", nil, cmtcfg.DefaultConfig())
if err != nil {
return err
}

return server.SetCmdServerContext(cmd, ctx)
}
err = cmd.ExecuteContext(ctx)
require.Errorf(t, err, sdkerrors.ErrAppConfig.Error())
Expand Down
11 changes: 1 addition & 10 deletions simapp/simd/main.go
Original file line number Diff line number Diff line change
@@ -1,23 +1,14 @@
package main

import (
"os"

"cosmossdk.io/simapp"
"cosmossdk.io/simapp/simd/cmd"
"github.com/cosmos/cosmos-sdk/server"
svrcmd "github.com/cosmos/cosmos-sdk/server/cmd"
)

func main() {
rootCmd := cmd.NewRootCmd()
if err := svrcmd.Execute(rootCmd, "", simapp.DefaultNodeHome); err != nil {
switch e := err.(type) {
case server.ErrorCode:
os.Exit(e.Code)

default:
os.Exit(1)
}
panic(err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this keep the exit code of 1? If not, please revert.

}
}