Add child logger & remove method names from log messages #412
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ian-shim
force-pushed
the
logging-refactor
branch
from
15f3ee3 to
f54001a
Compare
pschork
approved these changes
Mar 29, 2024
common/geth/client.go
Outdated
| break | ||
| } else { | ||
| return receipt, nil | ||
| } | ||
| } else { | ||
| c.Logger.Debug("EnsureTransactionEvaled: failed to get chain tip while waiting for transaction to mine", "err", err) | ||
| c.Logger.Debug("failed to get chain tip while waiting for transaction to mine", "err", err) |
There was a problem hiding this comment.
nit: failed to query block height
disperser/batcher/txn_manager.go
Outdated
| @@ -261,7 +261,7 @@ func (t *txnManager) monitorTransaction(ctx context.Context, req *TxnRequest) (* | |||
| } | |||
| txID, err := t.wallet.SendTransaction(ctx, newTx) | |||
| if err != nil { | |||
| t.logger.Error("failed to send txn", "component", "TxnManager", "method", "monitorTransaction", "tag", req.Tag, "txn", req.Tx.Hash().Hex(), "attempt", retryFromFailure, "maxRetry", maxSpeedUpRetry, "err", err) | |||
| t.logger.Warn("failed to send txn", "component", "TxnManager", "method", "monitorTransaction", "tag", req.Tag, "txn", req.Tx.Hash().Hex(), "attempt", retryFromFailure, "maxRetry", maxSpeedUpRetry, "err", err) | |||
There was a problem hiding this comment.
Maybe log Warn("failed to send txn - retrying" when retrying and Error("failed to send txn - retries exhaused" when we give up.
common/geth/client.go
Outdated
| @@ -265,13 +266,13 @@ func (c *EthClient) waitMined(ctx context.Context, txs []*types.Transaction) (*t | |||
| chainTip, err := c.BlockNumber(ctx) | |||
| if err == nil { | |||
| if receipt.BlockNumber.Uint64()+uint64(c.numConfirmations) > chainTip { | |||
| c.Logger.Debug("EnsureTransactionEvaled: transaction has been mined but don't have enough confirmations at current chain tip", "txnBlockNumber", receipt.BlockNumber.Uint64(), "numConfirmations", c.numConfirmations, "chainTip", chainTip) | |||
| c.Logger.Debug("transaction has been mined but don't have enough confirmations at current chain tip", "txnBlockNumber", receipt.BlockNumber.Uint64(), "numConfirmations", c.numConfirmations, "chainTip", chainTip) | |||
There was a problem hiding this comment.
nit: don't -> doesn't and tip -> head
dmanc
approved these changes
Mar 29, 2024
retriever/metrics.go
Outdated
| @@ -44,7 +44,7 @@ func NewMetrics(httpPort string, logger logging.Logger) *Metrics { | |||
| }, | |||
| ), | |||
| httpPort: httpPort, | |||
| logger: logger, | |||
| logger: logger.With("component", "retriever.Metrics"), | |||
There was a problem hiding this comment.
nit: Should we standarize on camel case? So this should be "RetrieverMetrics"?
There was a problem hiding this comment.
I standardized it to the struct name and prefixed by the <package_name>. when the struct name is vague.
I guess it's cleaner to just camel case it
operators/churner/server.go
Outdated
| @@ -39,7 +39,7 @@ func NewServer( | |||
| churner: churner, | |||
| latestExpiry: int64(0), | |||
| lastRequestTimeByOperatorID: make(map[core.OperatorID]time.Time), | |||
| logger: logger, | |||
| logger: logger.With("component", "churner.Server"), | |||
There was a problem hiding this comment.
Same comment for everything that has a "." in the name
jianoaix
reviewed
Mar 29, 2024
| @@ -41,10 +41,11 @@ var _ common.EthClient = (*EthClient)(nil) | |||
| // NewClient creates a new Ethereum client. | |||
| // If PrivateKeyString in the config is empty, the client will not be able to send transactions, and it will use the senderAddress to create transactions. | |||
| // If PrivateKeyString in the config is not empty, the client will be able to send transactions, and the senderAddress is ignored. | |||
| func NewClient(config EthClientConfig, senderAddress gethcommon.Address, rpcIndex int, logger logging.Logger) (*EthClient, error) { | |||
| func NewClient(config EthClientConfig, senderAddress gethcommon.Address, rpcIndex int, _logger logging.Logger) (*EthClient, error) { | |||
There was a problem hiding this comment.
why adding a new naming convention with underscore prefix? May keep is simple and consistent with others
ian-shim
force-pushed
the
logging-refactor
branch
from
f54001a to
cfe54c8
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why are these changes needed?
Refactoring loggers across codebase.
componentattribute for better filteringsourceattributeChecks