feat(server): cmd flag to disable colored logs #18478
Conversation
|
It seems that the existing content is still valid, so I'll repeat it verbatim in my response: WalkthroughThe changes involve the introduction of a new feature for controlling log colorization across various files in a codebase. A flag has been added to toggle colored logging, which is then utilized in the logger creation function to set the appropriate logging options. This update is consistent across client and server components, enhancing user control over log output aesthetics. Changes
Poem
TipsChat with CodeRabbit Bot (
|
github-prbot
requested review from
a team,
facundomedica and
likhita-809
and removed request for
a team
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- client/flags/flags.go (1 hunks)
- server/cmd/execute.go (1 hunks)
- server/util.go (2 hunks)
Files skipped from review due to trivial changes (2)
- client/flags/flags.go
- server/cmd/execute.go
Additional comments: 1
server/util.go (1)
- 197-200: The addition of the log level option is correctly implemented and the removal of the duplicate
log.TraceOptionfrom the previous version is a good cleanup. The code now correctly sets the log level based on theFlagLogLevelvalue from the context.
|
I'm adding a backport to v0.47. We had that functionality in our v0.46 fork, and would like to have that option in v0.47, otherwise our logging service will be less friendly. |
robert-zaremba
added
the
backport/v0.47.x
|
We should keep it on by default imho. Only v0.46 didn't had it on. |
julienrbrt
added
the
backport/v0.50.x
server/cmd/execute.go
Outdated
| @@ -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") | |||
There was a problem hiding this comment.
| 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
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :-)
There was a problem hiding this comment.
There is no other comment, and both of you prefer to keep the "current" defaults => I will do the update.
There was a problem hiding this comment.
I've renamed the flag to FlagLogNotColored. Boolean flags should be false by default.
|
We can't guarantee flags will stay the same in server v2. I'd propose we have an app.toml variable for which logger is used, json, default(colored), noncolored. This way the config will feed into things already in use. Secondly apps can pass their own custom logger if needed |
Isn't viper already doing that? If not, I would say that this should be done be a separte PR. The scope of this PR is to follow what is currently in the upstream. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- CHANGELOG.md (1 hunks)
- client/flags/flags.go (1 hunks)
- server/cmd/execute.go (1 hunks)
- server/util.go (2 hunks)
Files not reviewed due to errors (1)
- server/util.go (Error: Server error. Please try again later.)
Files skipped from review due to trivial changes (3)
- CHANGELOG.md
- client/flags/flags.go
- server/cmd/execute.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- CHANGELOG.md (1 hunks)
- server/util.go (2 hunks)
Files not reviewed due to errors (1)
- server/util.go (Error: Server error. Please try again later.)
Files skipped from review due to trivial changes (1)
- CHANGELOG.md
There was a problem hiding this comment.
Thanks for this change @robert-zaremba! I just made a comment about keeping the original behavior instead of unexpectedly turning it off for your use case: majority of the users were used to colored logs.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- client/flags/flags.go (1 hunks)
- server/cmd/execute.go (1 hunks)
- server/util.go (2 hunks)
Files skipped from review due to trivial changes (2)
- client/flags/flags.go
- server/cmd/execute.go
Additional comments: 1
server/util.go (1)
- 198-201: The logger level is correctly set based on the parsed log level string. This is a good use of the
zerologpackage's level parsing capabilities.
Wow TIL. That is true (https://github.com/rs/zerolog/blob/bb14b8b9de1133342fce9aff0158b354ec72f08c/console.go#L323-L333), this PR isn't needed then indeed. |
Good to know about the zerolog env variable. However, it's not documented in SDK, and I bet 99% are not aware about it. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- server/util.go (2 hunks)
Additional comments: 2
server/util.go (2)
173-179: The changes here correctly implement the new feature to control log colorization based on the
log_coloredflag. The use of!ctx.Viper.GetBool(flags.FlagLogNoColor)is appropriate to toggle the color option. However, ensure that theFlagLogNoColorconstant is correctly defined and corresponds to thelog_coloredflag mentioned in the summary. Also, verify that the default value of this flag is indeedfalseas intended.198-201: The removal of the redundant
log.TraceOptionand consolidation with the existing options is a good simplification. Ensure that theFlagTraceconstant is correctly defined and used throughout the codebase.
There was a problem hiding this comment.
LGTM, thank you for the inversion @robert-zaremba! However, after @tac0turtle's discovery of the environment variable toggle, should we perhaps document or proceed otherwise with this PR? I personally vote to proceed with this PR as it won't be an implementation detail but even more keeps the code highly configurable just by our start time flags.
|
+1 to have this explicit. |
(cherry picked from commit 8c7e694) # Conflicts: # CHANGELOG.md
|
@odeke-em documenting zerolog dependency is not part of this PR, and CLI thankfully is self explanatory. |
cosmos#18508) Co-authored-by: Julien Robert <julien@rbrt.fr>
Description
Ref: #13699
Colored logs should be optional. Enforcing colored logs will make the log capture ugly and non dev friendly.
Common use case is to stream logs to a logging service (eg Google logs). With colored logs, this will contain all color codes, making the log browsing very unfriendly.
This PR adds a new global flag:
log_colored. By default it's false (so this PR changes the default behavior), meaning that logs are not colored by default. When set, just by adding--log_coloredthe behavior will be as "now". This setting is more system friendly.Note, I was thinking to use more traditional name scheme with
-for connecting words (solog-coloredinstead oflog_colored), but other log related flags are using_(eg:log_level).Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdmake lintandmake testReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking changeSummary by CodeRabbit
New Features
Refactor
Documentation