-
Notifications
You must be signed in to change notification settings - Fork 166
improve logging, separate display/debug logs #423
Conversation
dfae442
to
c7d9b34
Compare
d24fbcf
to
6366717
Compare
ae754c3
to
425f94d
Compare
solution: create alternate loggingT instance, using glog.D (vs. glog.V) update tests colorize output respectively ensure debug (glog.V) logs are still rendered in stderr with traces for severity=.Error implement convenience colorizing methods
425f94d
to
d603b54
Compare
solution: use display logs as default for stderr, sending glog.V logs to file set global verbosity for glog.V -> logger.Debug (Level 5), noting that --verbosity flag can still override use --neckbeard flag to enable sending debug logs to stderr INSTEAD of display logs expose VerbosityTraceFloor as debug API <-- note: this to be REMOVED
solution: able to provide rolling status logs at glog.D(logger.Warn) verbosity, and event logs for blockchain, headerchain, mining, and downloading
… display solution: makes quite a few log lines better aware of verbosity and severity, so errors use .Error, more important logs have verbosities Warn, etc. add glog.D log lines for configuration notices, cmd progress reports, and other important HUD events
d603b54
to
0b16bcc
Compare
making chainIsMorden fail when --testnet used solution: refactor
solution: fix tests (adjust bats tests to new syntax/+logging cases - move datadir schema migration logic to cli.Before because it requires that the default log directory (in datadir) have not been established when checking for non/existing datadirs.
and to be able to handle possibly unexpected edge cases better
this makes the default stderr logging show chain events like insert, download, and mine in addition to rolling status logs by default
solution: remove flag and RPC method This flag and method were used to be able to configure the trace level for standard logging. With display/debug logs, there is little to no use for adjusting the trace level for debug logs
solution: turn off testing.Log for visual checking of rewards
solution: toggle display logging for tests to 0 at init fn
solution: implement congruent test fns for display tests and write a few display tests
solution: implement pattern to run tests which call glog.Fatal (ie os.Exit(1)) in separate process
solution: use filepath.Join for expected out
solution: add display log
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found some small issues. I'll re-review this tomorrow with fresh mind.
cmd/geth/main.go
Outdated
} | ||
} | ||
// Set default debug verbosity level. | ||
glog.SetV(5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sets magic verbosity level (which by accident equals the default value) - please introduce constant.
cmd/geth/main.go
Outdated
glog.SetToStderr(true) | ||
// Turn verbosity down for migration check. If migration happens, it will print to Warn. | ||
// Otherwise logs are just debuggers. | ||
glog.SetV(3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would extract this entire block between glog.SetV
s to a separate function.
cmd/geth/main.go
Outdated
|
||
// log.Println("Writing logs to ", logDir) | ||
// Turn on only file logging, disabling logging(T).toStderr and logging(T).alsoToStdErr | ||
glog.SetToStderr(false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And move the entire logging configuration to separate file (probably after merging #426)
cmd/geth/main.go
Outdated
// Set log dir. | ||
// GOTCHA: There may be NO glog.V logs called before this is set. | ||
// Otherwise everything will get all fucked and there will be no logs. | ||
glog.SetLogDir(logDir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before glog.SetLogDir
is called, logs are saved to system-default temporary directory. If logging is started before this call, the new logDir
will be used after file rotation (by default after 1800MB of data per file).
core/blockchain.go
Outdated
@@ -530,7 +537,7 @@ func (self *BlockChain) LoadLastState(dryrun bool) error { | |||
defer self.mu.Lock() | |||
|
|||
if recoveredHeight == 0 { | |||
glog.V(logger.Warn).Infoln("WARNING: No recoverable data found, resetting to genesis.") | |||
glog.V(logger.Error).Errorln("WARNING: No recoverable data found, resetting to genesis.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why WARNING:
from the message is not removed in all places?
core/blockchain.go
Outdated
glog.V(logger.Warn).Infof("Last block: #%d [%x…] TD=%v", self.currentBlock.Number(), self.currentBlock.Hash().Bytes()[:4], blockTd) | ||
glog.V(logger.Warn).Infof("Fast block: #%d [%x…] TD=%v", self.currentFastBlock.Number(), self.currentFastBlock.Hash().Bytes()[:4], fastTd) | ||
glog.D(logger.Warn).Infof("Local head header: #%s [%s…] TD=%s", | ||
logger.ColorGreen(strconv.Itoa(int(self.hc.CurrentHeader().Number.Uint64()))), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of Itoa
and type cast, you should use FormatUint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: still TODO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logging (or rather display) infrastructure is quite complicated after latest changes. But it gives great flexibility - dash
is a perfect example.
I need some time running dashboard in real world to provide more comments about it's look and feel (first thought: I would love to see chain name somewhere).
I approve the PR. As dashboard is still experimental, I think time and feedback is needed to eventually improve the display.
cmd/geth/log_context.go
Outdated
if ctx.GlobalIsSet(DisplayFlag.Name) { | ||
i := ctx.GlobalInt(DisplayFlag.Name) | ||
if i > 5 { | ||
return fmt.Errorf("--%s level must be 0 <= i <= 3, got: %d", DisplayFlag.Name, i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0 <= i <= 5
, not <= 3
cmd/geth/log_context.go
Outdated
|
||
if logger.MlogEnabled() { | ||
glog.V(logger.Warn).Warnf("Machine log config: mlog=%s mlog-dir=%s", logger.GetMLogFormat().String(), logger.GetMLogDir()) | ||
glog.D(logger.Warn).Infof("Machine log config: mlog=%s mlog-dir=%s", logger.ColorGreen(logger.GetMLogFormat().String()), logger.ColorGreen(logger.GetMLogDir())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This Infof
seems inconsistent with other Warnf
s.
cmd/geth/log_dispatch.go
Outdated
// displayEventHandlers set. This can be considered a temporary solve for handling "registering" or | ||
// "delegating" log interface systems. | ||
func mustGetDisplaySystemFromName(s string) displayEventHandlers { | ||
displaySystem := basicDisplaySystem |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's more "idiomatic" to write this without variable, returning from each case, and fataling by default. But it's just a cosmetic change, and I'm not sure which solution is more clear ;)
cmd/geth/log_dispatch.go
Outdated
} | ||
} | ||
if len(eqs) < 2 { | ||
glog.Errorf("%v: %v. Must be comma-separated pairs of module=interval.", ErrInvalidFlag, eqs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why Error
->os.Exit
instead of Fatalf
?
cmd/geth/log_display_basic.go
Outdated
} | ||
|
||
// Examples of spec'd output. | ||
var xlocalOfMaxD = "#92481951 of #93124363" // #2481951 of #3124363 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like const
, not var
. Consider using block, instead of individual consts
/vars
.
"github.com/ethereumproject/go-ethereum/logger" | ||
"github.com/ethereumproject/go-ethereum/core" | ||
"fmt" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment with example "display" is a nice feature of default display format. For "prettier" logging, comments may contain colorless transcript of a display.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
cmd/geth/log_display_dash.go
Outdated
return sl | ||
} | ||
|
||
// greenDisplaySystem is "spec'd" in PR #423 and is a little fancier/more detailed and colorful than basic. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy-pasted comment, please update or remove.
cmd/geth/flags.go
Outdated
} | ||
DisplayFormatFlag = cli.StringFlag{ | ||
Name: "display-fmt", | ||
Usage: "Display format", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add information about possible values (default
, dash
and green
).
Looks great!
or to (left):
it seems that second option is more logical and more flexible for extensions |
- adds commented examples for display fmts - moves a few vars to block consts - use glog.Fatal instead of glog.Err + os.Exit - fix typos and deadness in comments - make a few logs more consistent
solution: refactors log_display_basic, adds handler for mined block event - also debugs print basic trying to calc diff for whole chain
use global 'current' bookmark vars instead of local copies check that we're not going to try and calc progress rate for whole chain
solution: reorg mode decision logic
FYI -- waiting on rustup brokennes rust-lang/rustup#1316 |
@whilei tested with last changes, looks really good now. The only thing I've noticed is that it put duplicate lines sometimes:
It looks strange, especially the fact that block hash is same, but number of transactions is different. |
Also, when it imports few blocks at once, let's add number of additional blocks, like:
and make sure that txs/mgas for multiple blocks is sum over these blocks |
solution: use event block data instead of db currentblock for import log
solution: pass event to print fn instead of block, improve logic around bookmark current head printed and to-print block
solution: add display line for interrupt signal
@splix Just pushed a few changes addressing your notes above.
Updated: |
solution: make it 4-digit padded for consistency across all possible cases
problem
Existing logging isn't as friendly-looking as it could be. The logs are purely event/protocol derived and include a lot of often-noisy information.
solution
Separate "debug"-y logs from "displayable" logs; ie end-user-facing vs. developer-facing. Base display logs not only on events (via
mux
subscriptions), but also on time-interval-based status logging.The proposed solution:
glog.V
protocol/event logs to files in<chaindir>/log
with default verbosityDebug
. These files are then rotated automatically based on size and age.glog.D
for logs which should only be rendered to stderr; these display logs are also verbosity-aware, and can be adjusted. Current default is--display=2
on a practical scale of0(totally silent)-3(include event subscriptions)
, where2
prints mostly only interval-based status logs (ala--log-status
), like:while
--display=3
includes subscription-based logs likeseverity=Error
logs fromglog.V
to also be written to stderr, along with their file trace--neckbeard
flag for developers who'd rather get all normalglog.V
debug-y logs to stderr and don't like pretty colorsdiscussion
❓ Do we like the colors? Any suggestions, feedback, or wishes for the proposed formatting?
❓ Should we move default
--display
verbosity to3
to also include subscription-based logs?todo
🖌 There's a lot of commits and they're pretty messy (not always
problem/solution
format, etc...). Will squash before merge, but left as-is for sake of transparency and granularity for now.examples
ℹ️ Note in the screenshots the small terminal window below where
tail
is following the relevant debug log files (~/Library/EthereumClassic/mainnet/log/geth.INFO
).Current default:

--display=3
--display=2
--neckbeard
iconography
Event logs have associated icons:
SYNC logs have associated icons:
Rel issues logrotation: #421, noisy log lines: #410, general improve status log: #127
Rel milestone 4.2: https://github.com/ethereumproject/go-ethereum/milestone/12