(Node) Add more logs #2291
(Node) Add more logs #2291
Conversation
📝 WalkthroughWalkthroughThe changes in this pull request focus on enhancing the logging and restructuring the conditional checks within the Changes
Possibly related PRs
Suggested reviewers
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
node/pkg/aggregator/aggregator.go (3)
111-122: LGTM! Enhanced logging and improved code structure.The changes in the
HandleTriggerMessagefunction improve both the code structure and logging capabilities:
- Introducing
currentLeaderenhances readability and reduces repeated function calls.- The new order of checks is more logical, first verifying if the sender is the leader, then checking if it's from the current node.
- Enhanced logging provides better context for debugging, aligning with the PR objective.
Consider adding the round ID to the error log message when a trigger message is sent from a non-leader:
- log.Warn().Str("Player", "Aggregator").Str("Sender", msg.SentFrom).Str("CurrentLeader", currentLeader).Str("Me", n.Raft.GetHostId()).Msg("trigger message sent from non-leader") + log.Warn().Str("Player", "Aggregator").Str("Sender", msg.SentFrom).Str("CurrentLeader", currentLeader).Str("Me", n.Raft.GetHostId()).Int32("RoundID", triggerMessage.RoundID).Msg("trigger message sent from non-leader")This addition would provide even more context for debugging purposes.
Also applies to: 127-127
209-211: LGTM! Consistent improvements in logging and code structure.The changes in the
HandlePriceFixMessagefunction are consistent with those made inHandleTriggerMessage:
- Introduction of
currentLeaderimproves code readability.- Enhanced logging provides better context for debugging, aligning with the PR objective.
Consider adding the round ID to the error log message when a price fix message is sent from a non-leader:
- log.Warn().Str("Player", "Aggregator").Str("Sender", msg.SentFrom).Str("CurrentLeader", currentLeader).Str("Me", n.Raft.GetHostId()).Msg("price fix message sent from non-leader") + log.Warn().Str("Player", "Aggregator").Str("Sender", msg.SentFrom).Str("CurrentLeader", currentLeader).Str("Me", n.Raft.GetHostId()).Int32("RoundID", priceFixMessage.RoundID).Msg("price fix message sent from non-leader")This addition would provide even more context for debugging purposes and maintain consistency with the
HandleTriggerMessagefunction.
Rollback of Round Update Logic Execution Order Not Found
The changes in this file successfully enhance the logging capabilities, aligning with one of the PR objectives. However, no implementation for rolling back the round update logic execution order was found, which was also mentioned in the PR summary.
🔗 Analysis chain
Line range hint
1-391: Verify the rollback of round update logic execution order.The changes in this file successfully enhance the logging capabilities, which aligns with one of the PR objectives. However, I couldn't find any changes related to rolling back the round update logic execution order, which was mentioned in the PR summary.
Could you please clarify if the rollback of the round update logic execution order has been implemented in another file? If it should be in this file, please indicate where this change should be made.
To help verify this, you can run the following script to search for any recent changes related to round update logic:
This script will help identify if the rollback was implemented in another file or if it's missing entirely.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for recent changes related to round update logic # Search for files with recent changes mentioning 'round' and 'update' git log -G"round.*update" --pretty=format:'%h %s' --name-only | grep -v '^$' | head -n 20 # Search for recent changes in files containing 'aggregator' git log --name-only --pretty=format:'%h %s' -- '*aggregator*' | grep -v '^$' | head -n 20Length of output: 2616
Description
Type of change
Please delete options that are not relevant.
Checklist before requesting a review
Deployment