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.

Sign up for GitHub

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(server): cmd flag to disable colored logs #18478

Merged
merged 10 commits into from
Nov 19, 2023
Merged

feat(server): cmd flag to disable colored logs #18478

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
debb6ab
feat(server): cmd flag to disable colored logs
robert-zaremba Nov 15, 2023
c62d5a1
changelog
robert-zaremba Nov 15, 2023
5570800
rename flag
robert-zaremba Nov 15, 2023
b5e39d5
add comment about CometBFT Trace flag
robert-zaremba Nov 15, 2023
5fbe2f2
Merge remote-tracking branch 'origin/main' into robert/color-logs
robert-zaremba Nov 15, 2023
340614b
update comment
robert-zaremba Nov 17, 2023
17020cc
rename flag
robert-zaremba Nov 17, 2023
7809c4c
simplify name
robert-zaremba Nov 17, 2023
ff6d836
fix compile
robert-zaremba Nov 17, 2023
04e9d94
forgot to save
robert-zaremba Nov 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions client/flags/flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ const (
// Logging flags
FlagLogLevel = "log_level"
FlagLogFormat = "log_format"
FlagColorLogs = "log_colored"
robert-zaremba marked this conversation as resolved.
Show resolved Hide resolved
)

// List of supported output formats
Expand Down
1 change: 1 addition & 0 deletions server/cmd/execute.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ func Execute(rootCmd *cobra.Command, envPrefix, defaultHome string) error {
rootCmd.PersistentFlags().String(flags.FlagLogLevel, zerolog.InfoLevel.String(), "The logging level (trace|debug|info|warn|error|fatal|panic|disabled or '*:<level>,<key>:<level>')")
// NOTE: The default logger is only checking for the "json" value, any other value will default to plain text.
rootCmd.PersistentFlags().String(flags.FlagLogFormat, "plain", "The logging format (json|plain)")
rootCmd.PersistentFlags().Bool(flags.FlagColorLogs, false, "Enable colored logs")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
rootCmd.PersistentFlags().Bool(flags.FlagColorLogs, false, "Enable colored logs")
rootCmd.PersistentFlags().Bool(flags.FlagColorLogs, true, "Enable colored logs")

Or switch the flag name to disable color

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why?

Copy link
Member

@julienrbrt julienrbrt Nov 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Because it should stay enabled by default. It was on in v0.45, v0.47 and v0.50. v0.46 was an exception because the logger was reverted to comet logger.

Additionally the PR mentions a flag to disable the logger, so I don't expect a behavior change here (turning off the coloring by default).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Normally non colored logs are the default in software. I would consider default=colored setting as a "soft bug".
But if everyone wants to keep colored by default, then fair enough. Will wait for one more comment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

While I understand and agree that logs should be clean, we should focus on majority usage and expectations plus keeping expectations as they are. Please read about Hyrum's law https://www.hyrumslaw.com/
I am with @julienrbrt that we shouldn't just be turning off long standing behavior. Most users expect the colored lines to help them find out what's tripping or interesting events.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fair enough. So maybe last question - do we want to conduct a quick survey with other projects before we finalize the decision about the default.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think, inverting the change later on is a much easier process and it'll allow you to get your commit in as we collect data. Essentially I'd say, let's get it to the current defaults, meantime when running your node, turn off coloring, then put out a survey asking folks about the problem and give it time so you get sufficient data and once ready, just flipping an opposite will be a trivial commit :-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is no other comment, and both of you prefer to keep the "current" defaults => I will do the update.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've renamed the flag to FlagLogNotColored. Boolean flags should be false by default.


executor := cmtcli.PrepareBaseCmd(rootCmd, envPrefix, defaultHome)
return executor.ExecuteContext(ctx)
Expand Down
6 changes: 3 additions & 3 deletions server/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,9 @@ func CreateSDKLogger(ctx *Context, out io.Writer) (log.Logger, error) {
if ctx.Viper.GetString(flags.FlagLogFormat) == flags.OutputFormatJSON {
opts = append(opts, log.OutputJSONOption())
}
opts = append(opts,
log.ColorOption(ctx.Viper.GetBool(flags.FlagColorLogs)),
log.TraceOption(ctx.Viper.GetBool("trace"))) // cmtcli.TraceFlag
robert-zaremba marked this conversation as resolved.
Show resolved Hide resolved
robert-zaremba marked this conversation as resolved.
Show resolved Hide resolved

// check and set filter level or keys for the logger if any
logLvlStr := ctx.Viper.GetString(flags.FlagLogLevel)
Expand All @@ -194,9 +197,6 @@ func CreateSDKLogger(ctx *Context, out io.Writer) (log.Logger, error) {
opts = append(opts, log.LevelOption(logLvl))
}

// Check if the CometBFT flag for trace logging is set and enable stack traces if so.
opts = append(opts, log.TraceOption(ctx.Viper.GetBool("trace"))) // cmtcli.TraceFlag

return log.NewLogger(out, opts...), nil
}

Expand Down
Loading