chore: cleanups to improve feedback #101
Conversation
As we are automatically generating spans from method names, it's better to have something more expressive than get/put :) Signed-off-by: Arnaud Bailly <[email protected]>
Signed-off-by: Arnaud Bailly <[email protected]>
WalkthroughThis pull request standardises header storage and retrieval across several crates. In Amaru’s CLI tool and ChainStore trait implementations, the methods have been renamed from Changes
Sequence Diagram(s)sequenceDiagram
participant CMD as ImportChainDB Command
participant DB as Database Store
CMD->>DB: Call store_header(hash, header)
alt Storage Failure
DB-->>CMD: Return error
CMD->>CMD: Trigger WorkerError::Panic
else Successful Storage
DB-->>CMD: Acknowledge success
end
sequenceDiagram
participant HS as HeaderStage (Consensus)
participant CS as ChainStore
participant CSel as ChainSelection
participant Log as Logging System
HS->>CS: Call load_header(hash)
CS-->>HS: Return header
HS->>CSel: Process header (roll_forward/rollback)
CSel-->>HS: Return SwitchToFork struct (with peer, rollback_point, tip, fork)
HS->>Log: Emit debug log with detailed instrumentation
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 (
|
| @@ -111,7 +113,10 @@ impl HeaderStage { | |||
| .or_panic() | |||
| } | |||
|
|
|||
| #[instrument(level = Level::INFO, skip(self))] | |||
| #[instrument(level = Level::DEBUG, skip_all, | |||
There was a problem hiding this comment.
Why the switch back to DEBUG here? I thought that it's a significant enough (and occasional) event that it deserves being treated as log, no ?
There was a problem hiding this comment.
I thought this was a mistake as we/you changed all instrument to DEBUG.
There was a problem hiding this comment.
I really think we should not care about those levels in the code, but I will revert back to INFO
There was a problem hiding this comment.
Funny enough, I went for DEBUG in ADR-007 because i didn't see that tracing had a TRACE level. So I will be switching those DEBUG to TRACE in an upcoming commit.
So that anything DEBUG/INFO will be treated as log. It'll be less confusing.
Still for this one, I really think it shouldn't be a trace, but a log. A debug one perhaps but as you said, I don't think we should care about those levels for logging. Every log as INFO is good IMO.
There was a problem hiding this comment.
I am having a hard time making a difference between logs and traces, they are one and the same thing. the difference happens downstream with the use you make of that information: you can ignore timing and treat the entry or exit of a function as a log, being interested in the discrete event this represents, or you can care about the duration, or perhaps the fact it raised and error, and/or the call tree it produced, with or without timings.
IMNSHO, those differences we make up-front are probably short-sighted and only breeds confusion and complexity in the code base. But I value more alignment within the team than opinion so I will make sure to read and follow the policy we agreed on in the ADR 👍
There was a problem hiding this comment.
We can summarize the difference simply:
- logs = for humans
- traces = for programs
As a human, I cannot process outputs on stdout when my terminal is flooded with thousands of lines per second. So of course, I can redirect the output, or export it to a sub-system for analysis/observability (which I believe is what you are advocating for).
But, it's also nice to have some readily available output meant for immediate human-consumption. Something which I need not to spin up an telemetry backend with a collector and a trace visualizer to make sense of (which I believe, you have also been advocating for 😅 ...).
Signed-off-by: Arnaud Bailly <[email protected]>
![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
🧹 Nitpick comments (2)
crates/consensus/src/consensus/chain_selection.rs (2)
24-24: Fix typo in documentation.There's a small typo in the doc comment: "sqeuence" should be "sequence". Just a wee fix needed there!
169-169: Consider adding block height to instrumentation.The logging is good, but adding
header.block_height()would give us the full picture. It's like having a pint without knowing how full it is!- #[instrument(level = Level::DEBUG, skip(self, header), fields(header.slot = header.slot(), header.hash = header.hash().to_string()))] + #[instrument(level = Level::DEBUG, skip(self, header), fields(header.slot = header.slot(), header.hash = header.hash().to_string(), header.height = header.block_height()))]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/consensus/src/consensus/chain_selection.rs(8 hunks)crates/consensus/src/consensus/mod.rs(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/consensus/src/consensus/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build on windows-latest with target x86_64-pc-windows-msvc
🔇 Additional comments (6)
crates/consensus/src/consensus/chain_selection.rs (6)
18-18: LGTM! Clean import addition.The tracing import is spot on for the new instrumentation changes.
100-105: Brilliant refactor to named fields!This is a cracking improvement! Moving from tuple to struct format makes the code much more self-documenting. It's like going from "fish and chips in newspaper" to "fish and chips in a proper box with labels" - you know exactly what you're getting!
212-212: Spot on with the instrumentation!The instrumentation matches the suggested format perfectly.
222-223: Consider implementing the intersection optimization.The TODO comment flags a good optimization opportunity. Instead of always switching to the anchor point, finding the best intersection point could reduce unnecessary rollbacks. It's like taking the M25 when there's a shortcut through the city!
Would you like me to help create an issue to track this optimization task?
249-249: Nice consistent instrumentation!Keeping the same instrumentation pattern across related methods. Good stuff!
346-354: Tests updated properly for the new struct format.All test assertions have been properly updated to match the new
SwitchToForkstruct format. Good job keeping the tests in sync!Also applies to: 458-466
Summary by CodeRabbit
Refactor
Chore