chore: improve tracing handling #144
Conversation
WalkthroughThis pull request removes redundant span propagation parameters and refines method signatures across multiple modules. In the consensus code, the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Stage
participant State
participant LedgerStore
Client->>Stage: roll_forward(point, raw_block)
Stage->>Stage: forward(point, block, block_header_hash)
Note right of Stage: Process block & validate state
Stage->>State: forward(slot, point, block)
State->>State: handle_volatile_state(now_stable, slot, current_epoch)
State-->>Stage: BlockValidationResult
Stage-->>Client: BlockValidationResult
sequenceDiagram
participant Peer
participant Consensus
Peer->>Consensus: forward_block(peer, header)
Consensus->>Consensus: Capture current span (Span::current())
Consensus-->>Peer: ValidateBlockEvent::Validated
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (11)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (8)
Note 🎁 Summarized by CodeRabbit FreeYour organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above. 🪧 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
🧹 Nitpick comments (1)
crates/amaru-stores/src/rocksdb/mod.rs (1)
167-177: Well-structured pot adjustment methodThe
adjust_potsmethod provides a clean interface for adjusting pots based on the rewards summary. The instrumentation includes helpful fields that will make tracing much more informative.I notice that this method currently only resets fees to zero. You might want to use the
rewards_summaryparameter more extensively in the future.Consider updating this method to actually use the
rewards_summaryparameter for adjusting other aspects of the pots, as the name suggests:fn adjust_pots(&self, rewards_summary: &RewardsSummary) -> Result<(), StoreError> { self.with_pots(|mut row| { + let pots = row.borrow_mut(); + pots.treasury += rewards_summary.delta_treasury(); + pots.reserves += rewards_summary.delta_reserves(); - row.borrow_mut().fees = 0; + pots.fees = 0; }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
crates/amaru-consensus/src/consensus/header_validation.rs(4 hunks)crates/amaru-consensus/src/consensus/wiring.rs(2 hunks)crates/amaru-ledger/src/rewards.rs(1 hunks)crates/amaru-ledger/src/state.rs(4 hunks)crates/amaru-ledger/src/state/transaction.rs(1 hunks)crates/amaru-ledger/src/store.rs(2 hunks)crates/amaru-stores/src/rocksdb/mod.rs(4 hunks)crates/amaru/src/pipeline.rs(3 hunks)crates/amaru/src/sync/pull.rs(1 hunks)simulation/amaru-sim/src/bin/amaru-sim/sync.rs(0 hunks)
💤 Files with no reviewable changes (1)
- simulation/amaru-sim/src/bin/amaru-sim/sync.rs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build on windows-latest with target x86_64-pc-windows-msvc
- GitHub Check: Snapshots (preprod, 10.1.4)
🔇 Additional comments (29)
crates/amaru-ledger/src/store.rs (2)
25-25: Good addition of tracing import.Adding the
instrumentandLevelimports from tracing is proper preparation for the new instrumented method.
165-172: Brilliant implementation oftick_pools!This new method is a cracking addition that follows the established patterns in the Store trait. The instrumentation with tracing will provide excellent visibility into pool ticking operations without drowning logs in details (thanks to
skip_all).The implementation efficiently leverages the existing
with_poolsmethod and applies the tick operation for a specified epoch to each pool - keeping things DRY as a bone.crates/amaru/src/sync/pull.rs (2)
118-127: Smashing improvement to tracing instrumentation!This change is spot on! Replacing the manual span creation with the declarative
#[instrument]attribute makes the code much cleaner. The fields are well chosen to provide relevant context for debugging - including point, tip and peer information.Using
skip_allto avoid logging the full method arguments while explicitly defining the fields you want is a neat trick that keeps the logs tidy.
133-133: Correctly simplified Rollback event creation.Good job removing the span parameter from the Rollback event creation, which aligns with the changes to the PullEvent enum elsewhere in the codebase. This makes the code more consistent and easier to follow.
crates/amaru-ledger/src/rewards.rs (1)
793-796: Sensible ownership model improvement.Crikey, this is much better! Changing the method signature from
selfto&selfmeans theRewardsSummaryinstance isn't consumed when callingunclaimed_rewards(). This allows for multiple calls to the method or further use of the instance afterwards.The corresponding change from
into_iter()toiter()properly reflects this by borrowing rather than consuming the accounts collection. This is a textbook example of good Rust ownership practices.crates/amaru-consensus/src/consensus/wiring.rs (4)
2-3: Good imports cleanup and constants addition.The imports have been properly organized, and adding the EVENT_TARGET constant is a bonzer improvement for consistency throughout the module.
Also applies to: 5-5, 9-10
14-14: Proper simplification of the Rollback event variant.Removing the Span parameter from the Rollback variant is a sensible change. No need to pass spans around like hot potatoes when they're not needed! This streamlines the event payload and aligns with your other tracing improvements.
39-52: Excellent tracing enhancement for RollForward handling.This is a proper job! Adding the trace span with detailed fields provides much better observability for roll-forward events. The commentary explains clearly what's happening with the span, and setting the parent span ensures the tracing context is maintained.
Including the slot and hash in the span fields is brilliant for troubleshooting, as these are key identifiers when tracking block progress through the system.
53-56: Correctly updated Rollback handler call.You've properly updated the handle_roll_back call to match the new method signature without the span parameter. This maintains consistency with the changes to the PullEvent enum and overall tracing approach.
crates/amaru-ledger/src/state.rs (3)
182-271: Nice extraction of volatile state handling logic!This is a brilliant refactoring that moves the volatile state handling logic into its own method. It makes the
forwardmethod much cleaner while properly encapsulating this complex logic. Good use of span instrumentation as well.A proper job of modularization, this is! The code is now as tidy as a well-organized pub after closing time.
277-278: Good simplification of method signatureRemoving the
spanparameter streamlines the API and is consistent with the pattern of usingSpan::current()elsewhere. This is a nice improvement to reduce parameter passing.
295-295: Clean delegation to the new methodNice replacement of the inline logic with a call to the new
handle_volatile_statemethod. This is a textbook example of the "Extract Method" refactoring pattern.crates/amaru-ledger/src/state/transaction.rs (3)
36-43: Simplified function signature by removing parent span parameterRemoving the
parentparameter fromapplyfunction is a good call. No need to pass spans around when you can access the current span through the tracing system. This makes the API cleaner and less prone to misuse.Like removing the unnecessary middleman at a farmers' market - direct from producer to consumer!
80-90: Streamlined apply_certificate callsThe
parentparameter has been removed from theapply_certificatecalls, consistent with the function signature change. This kind of consistent refactoring across the codebase is exactly what we want to see.
236-237: Removed parent parameter from trace loggingConsistently removed the
parentparameter from all trace logging statements. This is a thorough cleanup that ensures consistency throughout the file. Good attention to detail!Also applies to: 240-241, 250-251, 255-256, 262-263, 267-268, 278-279, 288-289, 302-303
crates/amaru/src/pipeline.rs (3)
57-69: Great extraction of forward logic into dedicated method!This new
forwardmethod is a cracking piece of work! It encapsulates the block processing logic with proper instrumentation and clean error handling. The use of#[instrument]with detailed fields will make debugging much easier.The pattern of returning the current span with the result is elegant - you're killing two birds with one stone here.
81-81: Clean delegation to forward methodThe
roll_forwardmethod now simply delegates to the newforwardmethod, making it much cleaner and focused. This is as tidy as a freshly made bed in a 5-star hotel!
84-84: Added instrumentation to rollback_to methodNice addition of the
#[instrument]attribute to therollback_tomethod. This brings it in line with the newforwardmethod and will make tracing consistent across both operations. Good attention to symmetry in the API.crates/amaru-stores/src/rocksdb/mod.rs (4)
154-165: Good extraction of reward application logicThis new
apply_rewardsmethod nicely encapsulates the logic for applying rewards by modifying the treasury and fees. The instrumentation will make it easier to trace this operation during debugging.Clean, focused, and well-instrumented - just the ticket!
179-187: Simple and focused fee reset methodNice addition of this focused method. It's small but important to have these operations as separate, well-named methods rather than scattered inline.
189-203: Good encapsulation of block count reset logicThis
reset_blocks_countmethod provides a nice abstraction for resetting the block count. The comment about potentially needing a more efficient way to drop a "table" is valuable and flags a possible future optimization.Good job including this note - it's like leaving a tip for the next developer who has to work with this code. Very considerate!
415-437: Clean use of extracted methods in next_snapshotThe refactoring of
next_snapshotto use the newly created methods makes the code much more readable and maintainable. This is a textbook example of good refactoring - breaking down a complex method into well-named, single-responsibility methods.This change has made the method as readable as a well-written novel - you know exactly what's happening at each step!
crates/amaru-consensus/src/consensus/header_validation.rs (7)
31-31: Nice addition to the tracing importsAdding
Spanto the imports is a good move here - it'll come in handy with theSpan::current()usage later on. Clean and tidy!
120-120: Good use ofSpan::current()instead of parameter passingLove this change! Using
Span::current()rather than manually passing a span around is much cleaner. It's like letting the postman deliver mail without having to tell him the address for every house - the context is already there.
132-133: Parameter reduction looks spot onRemoving the parent_span parameter from this
forward_blockcall keeps things consistent with your new approach. Less parameters means less faff for everyone!
183-183: Clean parameter removalAnother spot where you've properly removed the parent_span parameter. Top job on being thorough!
189-190: Parameter cleanup continuesGood stuff! The parent_span has been properly removed from the
switch_to_forkcall as well, maintaining consistency across the codebase.
199-199: Brilliant addition of instrumentationAdding the
#[instrument]attribute tohandle_roll_backis a cracking improvement! Theskip(self)is a nice touch too - no need to faff about trying to format that self reference.
221-222: Final parameter cleanup looks goodYou've done a proper job of removing the parent_span parameter from this last
switch_to_forkcall. The changes look consistent throughout the file - you've dotted all the i's and crossed all the t's!
jeluard
force-pushed
the
jeluard/better-tracing
branch
3 times, most recently
from
f50e0eb to
0d4a19e
Compare
jeluard
force-pushed
the
jeluard/better-tracing
branch
from
0d4a19e to
851ed74
Compare
Signed-off-by: jeluard <[email protected]>
jeluard
force-pushed
the
jeluard/better-tracing
branch
from
851ed74 to
2b4cad4
Compare
WIP
Clarifies
tracingusage by advocating to leverage smaller functions andinstrument.Remove
parentfrom spans as they should be inherited; new threads loose it so it has to be restored at thegasketstage boundaries.Summary by CodeRabbit
roll_backmethod for better traceability.