add log for txpool tx track #1046
Conversation
georgehao
requested review from
jonastheis,
Thegaram,
omerfirmak,
0xmountaintop and
colinlyguo
Co-authored-by: colin <[email protected]>
Co-authored-by: colin <[email protected]>
…t/add_log_for_long_sustain_tx
WalkthroughThe changes in this pull request focus on enhancing logging within the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…tech/go-ethereum into feat/add_log_for_long_sustain_tx
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (8)
core/tx_pool.go (8)
434-436: Consider using Debug log level for transaction evictionsWhile logging evicted transactions is useful for debugging, using the Info log level for frequent operations like transaction evictions might lead to excessive logging. Consider changing this to
log.Debugto reduce log verbosity during normal operations.- log.Info("evict queue tx for timeout", "tx", tx.Hash().String()) + log.Debug("evict queue tx for timeout", "tx", tx.Hash().String())
869-870: Consider using Debug log level for new pooled transactionsSimilar to the previous comment, logging new pooled transactions at the Info level might lead to excessive logging during normal operations. Consider changing this to
log.Debugto maintain consistency and reduce log verbosity.- log.Info("Pooled new executable transaction", "hash", hash, "from", from, "to", tx.To()) + log.Debug("Pooled new executable transaction", "hash", hash, "from", from, "to", tx.To())
891-892: Consider using Debug log level for new future transactionsConsistent with the previous comments, logging new future transactions at the Info level might lead to excessive logging during normal operations. Consider changing this to
log.Debugto maintain consistency across the codebase and reduce log verbosity.- log.Info("Pooled new future transaction", "hash", hash, "from", from, "to", tx.To()) + log.Debug("Pooled new future transaction", "hash", hash, "from", from, "to", tx.To())
Line range hint
926-932: Consider using Debug log level for enqueued transactions and good addition of "new tx" infoConsistent with the previous comments, logging enqueued transactions at the Info level might lead to excessive logging during normal operations. Consider changing this to
log.Debugto maintain consistency across the codebase and reduce log verbosity.The addition of the "new tx" information in the log is a good improvement, as it helps distinguish between new and existing transactions.
- log.Info("Enqueued transaction", "hash", hash.String(), "from", from, "to", tx.To(), "new tx", !addAll) + log.Debug("Enqueued transaction", "hash", hash.String(), "from", from, "to", tx.To(), "new tx", !addAll)
980-981: Consider using Debug log level for promoted transactionsIn line with the previous comments, logging promoted transactions at the Info level might lead to excessive logging during normal operations. Consider changing this to
log.Debugto maintain consistency across the codebase and reduce log verbosity.- log.Info("Promoted transaction from queue to pending", "hash", hash.String(), "from", addr, "to", tx.To()) + log.Debug("Promoted transaction from queue to pending", "hash", hash.String(), "from", addr, "to", tx.To())
1152-1154: Consider using Debug log level for removed transactionsConsistent with the previous comments, logging removed transactions at the Info level might lead to excessive logging during normal operations. Consider changing this to
log.Debugto maintain consistency across the codebase and reduce log verbosity.- log.Info("remove tx", "hash", hash, "outofbound", outofbound) + log.Debug("remove tx", "hash", hash, "outofbound", outofbound)
1381-1383: Consider using Debug log level for consistency, but Info might be acceptableWhile we've been recommending changing log levels to Debug for consistency and to reduce log verbosity, this particular log for dumping transaction hashes during reorg might be acceptable at the Info level. It's a less frequent operation and is guarded by a threshold check.
However, for the sake of consistency across the codebase, you might still want to consider changing it to Debug. The decision should be based on how critical this information is for regular monitoring vs. debugging.
If you decide to change it for consistency:
- log.Info("dumping runReorg tx hashes", "txHash", txs.Hash().Hex()) + log.Debug("dumping runReorg tx hashes", "txHash", txs.Hash().Hex())
1535-1537: Consider using Debug log level for removed cap-exceeding transactionsConsistent with the previous comments, logging removed cap-exceeding transactions at the Info level might lead to excessive logging, especially if the cap is frequently reached. Consider changing this to
log.Debugto maintain consistency across the codebase and reduce log verbosity.- log.Info("Removed cap-exceeding queued transaction", "hash", hash) + log.Debug("Removed cap-exceeding queued transaction", "hash", hash)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- core/tx_pool.go (9 hunks)
- params/version.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
core/tx_pool.go (1)
1776-1778: Appropriate use of Warn log level for long-lived transactionsThis is a good addition to the logging system. Using the Warn log level for transactions that have been in the pool for over 30 minutes is appropriate. It helps highlight potential issues with long-lived transactions without being as verbose as Info-level logging.
The condition
txLifecycle >= time.Minute*30ensures that this log only appears for truly long-lived transactions, which is a good practice to avoid unnecessary warnings.
|
rollback some log level. use |
georgehao
dismissed stale reviews from 0xmountaintop and colinlyguo
The base branch was changed.
|
Close this pr to update the changes to syncUpstream/active branch. #1079 |
1. Purpose or design rationale of this PR
add log for tx that be consumed in txpool
2. PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
3. Deployment tag versioning
Has the version in
params/version.gobeen updated?4. Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
New Features
Chores