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

[Admin] add log level customization command #1362

Merged
merged 7 commits into from Oct 9, 2021
Merged

Conversation

synzhu
Copy link
Contributor

@synzhu synzhu commented Sep 27, 2021

Add log level admin command, to allow dynamically changing the log level without restarting a node.

Tested and it works on localnet.

To change the log level to debug, for example:

curl -d '{"commandName": "set-log-level", "data": {"level": "debug"}}' -H 'Content-Type: application/json' localhost:5873/admin/run_command

where localhost:5873 is the address of the admin server.

Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

This is essentially just changing log levels. Could we instead use the admin server to perform this change?

@durkmurder
Copy link
Member

Yeeeah, I would actually just crank log level to debug and that's it.

@synzhu
Copy link
Contributor Author

synzhu commented Oct 4, 2021

This is essentially just changing log levels. Could we instead use the admin server to perform this change?

@huitseeker I considered this too. On one hand, it looks like this should be possible: https://github.com/rs/zerolog#setting-global-log-level

On the other hand, not sure if it's actually possible to do this at runtime: rs/zerolog#252

I'll look into this and investigate further.

@codecov-commenter
Copy link

codecov-commenter commented Oct 6, 2021

Codecov Report

Merging #1362 (8c6b7ae) into master (cdd3ee6) will decrease coverage by 0.01%.
The diff coverage is 10.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1362      +/-   ##
==========================================
- Coverage   55.29%   55.27%   -0.02%     
==========================================
  Files         516      516              
  Lines       32181    32185       +4     
==========================================
- Hits        17793    17790       -3     
- Misses      12001    12007       +6     
- Partials     2387     2388       +1     
Flag Coverage Δ
unittests 55.27% <10.25%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cmd/node_builder.go 0.00% <0.00%> (ø)
cmd/scaffold.go 0.91% <0.00%> (-0.01%) ⬇️
admin/command_runner.go 77.77% <100.00%> (-1.54%) ⬇️
engine/common/synchronization/engine.go 68.78% <100.00%> (ø)
...ngine/common/synchronization/finalized_snapshot.go 68.75% <0.00%> (-4.17%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cdd3ee6...8c6b7ae. Read the comment docs.

@synzhu synzhu requested a review from huitseeker October 6, 2021 10:25
@synzhu synzhu changed the title [Consensus] add logging for sync [Admin] add log level customization command Oct 6, 2021
Copy link
Member

@zhangchiqing zhangchiqing left a comment

Choose a reason for hiding this comment

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

Looks good. Had some minor suggestion

Comment on lines +374 to +375
log = log.Level(zerolog.DebugLevel)
zerolog.SetGlobalLevel(lvl)
Copy link
Member

Choose a reason for hiding this comment

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

why using debug level as global level?

Better to use InfoLevel as default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The way this works, is that SetGlobalLevel sets the minimum level that will be logged, regardless of what the root logger has set.

So, I set the root logger to Debug level, and use SetGlobalLevel to set the actual level we want to use. This is the only way to make the log level dynamically configurable. I cannot do things the other way around, because once the root logger's level is set, you cannot change it.

Hope this makes sense.

@@ -242,14 +247,15 @@ func (r *CommandRunner) runAdminServer(ctx context.Context) error {
func (r *CommandRunner) runCommand(ctx context.Context, command string, data map[string]interface{}) error {
r.logger.Info().Str("command", command).Msg("received new command")
Copy link
Member

Choose a reason for hiding this comment

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

better to log the input and output of each command call.

  1. if it receives a command, it should log received new command
  2. if it has run the command, it should log finish running the command
  3. if it fails to run the command, it should log failed to run the command with the error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this will be done in a separate PR :)

Copy link
Contributor

@vishalchangrani vishalchangrani left a comment

Choose a reason for hiding this comment

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

lgtm! thanks for putting this together.

@synzhu
Copy link
Contributor Author

synzhu commented Oct 9, 2021

bors merge

@bors
Copy link
Contributor

bors bot commented Oct 9, 2021

@bors bors bot merged commit be64190 into master Oct 9, 2021
@bors bors bot deleted the smnzhu/sync-logging branch October 9, 2021 03:10
@synzhu synzhu mentioned this pull request Oct 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants