Update log levels #609
Update log levels #609
Conversation
| @@ -572,7 +572,7 @@ func (b *SimulatedBackend) EstimateGas(ctx context.Context, call interfaces.Call | |||
| if transfer == nil { | |||
| transfer = new(big.Int) | |||
| } | |||
| log.Warn("Gas estimation capped by limited funds", "original", hi, "balance", balance, | |||
| log.Info("Gas estimation capped by limited funds", "original", hi, "balance", balance, | |||
There was a problem hiding this comment.
This can occur if a user requests too much gas, so we should not consider it an unexpected warn log
| @@ -1508,7 +1508,7 @@ func (bc *BlockChain) reorg(oldBlock, newBlock *types.Block) error { | |||
| logFn(msg, "number", commonBlock.Number(), "hash", commonBlock.Hash(), | |||
| "drop", len(oldChain), "dropfrom", oldChain[0].Hash(), "add", len(newChain), "addfrom", newChain[0].Hash()) | |||
| } else { | |||
| log.Warn("Unlikely preference change (rewind to ancestor) occurred", "oldnum", oldHead.Number(), "oldhash", oldHead.Hash(), "newnum", newHead.Number(), "newhash", newHead.Hash()) | |||
| log.Debug("Preference change (rewind to ancestor) occurred", "oldnum", oldHead.Number(), "oldhash", oldHead.Hash(), "newnum", newHead.Number(), "newhash", newHead.Hash()) | |||
There was a problem hiding this comment.
Follows update made in Coreth to reduce this to Debug since this can occur on restart when a node rewinds its preference to the last accepted block.
| log.Error("error occurred with JSON unmarshalling ethCallRequest.RequestArgs", "err", err) | ||
| return nil, nil | ||
| } | ||
|
|
||
| result, err := ethapi.DoCall(ctx, c.backend, transactionArgs, lastAcceptedBlockNumberOrHash, nil, c.backend.RPCEVMTimeout(), c.backend.RPCGasCap()) | ||
| if err != nil { | ||
| log.Debug("error occurred with EthCall", "err", err, "transactionArgs", ethCallRequest.RequestArgs, "blockNumberOrHash", lastAcceptedBlockNumberOrHash) | ||
| log.Error("error occurred with EthCall", "err", err, "transactionArgs", ethCallRequest.RequestArgs, "blockNumberOrHash", lastAcceptedBlockNumberOrHash) | ||
| return nil, nil | ||
| } | ||
|
|
||
| executionResult, err := json.Marshal(&result) | ||
| if err != nil { | ||
| log.Debug("error occurred with JSON marshalling result", "err", err) | ||
| log.Error("error occurred with JSON marshalling result", "err", err) |
There was a problem hiding this comment.
These requests should only come from a VM running on the same node, so failing to serve the request should be considered a warning at this time.
Note: CrossChainHandler is not used by any VM at this time
There was a problem hiding this comment.
Should these be a Warn? not an Error?
There was a problem hiding this comment.
This is assuming that requests coming from the same node. This is a feature that's not in use atm, so it depends on if VMs that wanted to use this should only send requests that should be successful 🤔
That was my logic when updating to Error at least.
There was a problem hiding this comment.
As of right now, if this log showed up I'd want to know about it rather than only wanting to know about it if it showed up in a large quantity or a surge, so I'm going to leave this as is for now.
We may want to change this in the future if the access pattern for it changes.
| @@ -257,7 +257,7 @@ func (client *stateSyncerClient) syncBlocks(ctx context.Context, fromHash common | |||
| } | |||
| blocks, err := client.client.GetBlocks(ctx, nextHash, nextHeight, parentsPerRequest) | |||
| if err != nil { | |||
| log.Warn("could not get blocks from peer", "err", err, "nextHash", nextHash, "remaining", i+1) | |||
| log.Error("could not get blocks from peer", "err", err, "nextHash", nextHash, "remaining", i+1) | |||
There was a problem hiding this comment.
This means that it has failed to sync, so this should be increased from Warn to Error
There was a problem hiding this comment.
Wouldn't it be possible for the peer to send us a malformed blocks and trigger this?
There was a problem hiding this comment.
The call to client.client.GetBlocks includes retries so if we exhaust retries we can log Error since we will return err and the sync has failed.
Responded to Stephen's comment. Going to leave as |
Why this should be merged
This PR updates the log levels in Subnet-EVM to ensure that they follow best practices.
How this works
Updates log levels
How this was tested
n/a
How is this documented
Debug - information useful for debugging that should not be surfaced in normal operation
Info - basic information about the program that a normal user would want to be aware of
Warn - not necessarily harmful, but unexpected
Error - should never happen and should cause a page to whoever is running the node and the maintainers of the repo to debug
Fatal - fatal error that should never happen has occurred
Note: the descriptions of Error and Fatal assume that the operator is using the application correctly. User error may lead to an error or fatal log that does not indicate a bug in the code.
There's a lot written up about log levels, but no canonical place for standards. Here is a decent write up that summarizes reasonable standards for logging in a little more detail: https://stackify.com/logging-levels-101/.