-
Notifications
You must be signed in to change notification settings - Fork 142
duties: correct logger usage (part 2) #2453
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Summary
This PR continues the effort from PR #2400 to standardize logger usage across the SSV validator codebase. The changes refactor message handling throughout the validator and committee components by:
Core Interface Changes:
- Replacing
HandleMessagemethod calls withEnqueueMessageacross validators and committees - Removing logger parameters from message handling interfaces, allowing each component to establish its own properly contextualized logger
Logger Standardization:
- Implementing consistent logger setup patterns where all relevant context fields (messageType, messageID, role, slot, dutyID, committeeID) are established upfront at the beginning of message processing functions
- Moving away from passing loggers between components to having each validator/committee create structured loggers with comprehensive field context
- Eliminating helper methods like
fetchTraceContext,withDutyID, andbuildTraceContextin favor of direct field additions for clearer code flow
Architectural Improvements:
- Moving duty ID calculation and storage earlier in the
StartDutyprocess to ensure availability for logging - Centralizing all logger context setup in
ProcessMessagemethods rather than scattered throughout different code paths - Updating test files to use standardized
log.TestLogger(t)instead of manual zap logger creation
The changes maintain the same core functionality while establishing a clear, maintainable pattern for logging that ensures all duty execution operations have consistent observability context. This refactoring supports better debugging and monitoring of distributed validator operations by guaranteeing that related log entries contain the same contextual information regardless of the processing path taken.
Confidence score: 4/5
- This PR is safe to merge with minimal risk as it primarily focuses on logging improvements without changing core business logic
- Score reflects thorough refactoring of logging patterns with consistent implementation across multiple components, though some minor inconsistencies remain
- Pay attention to error handling changes in slot extraction where errors are now silently ignored for event messages
Context used:
Rule - Prefer shorter, clearer names for methods and constants. If one condition implies another (e.g., attesting implies participating), use the more specific name rather than combining both concepts. (link)
6 files reviewed, 1 comment
090f71e to
aa9d5c0
Compare
Codecov Report❌ Patch coverage is ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
f3b4192 to
443776f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Summary
This PR is part 2 of a comprehensive logging refactoring effort focused on standardizing logger usage across the SSV protocol codebase. The changes address three main objectives: adding missing log fields and removing duplicates, standardizing logger setup with consistent context fields (slot, dutyID, etc.), and resolving issue #2452 to ensure Event-type messages have proper slot numbers for tracing.
The refactoring includes several key architectural changes:
Message Type Cleanup: Removed unused SSVSyncMsgType constant and related code, simplifying the message type system to focus on actively used types (SSVEventMsgType, SSVConsensusMsgType, SSVPartialSignatureMsgType).
Field Naming Standardization: Systematically replaced generic field names with more specific QBFT-prefixed variants (fields.Round() → fields.QBFTRound(), fields.Height() → fields.QBFTHeight()) to avoid ambiguity between different types of rounds/heights in the system.
Logger Setup Centralization: Refactored message handling methods (HandleMessage → EnqueueMessage) to centralize logger context setup at the highest entry points (Validator.ProcessMessage, Committee.ProcessMessage). This ensures all log entries have consistent contextual fields from the earliest possible moment.
Duty ID Management: Moved from a hashmap-based duty ID caching approach to on-demand computation using new centralized functions (BuildDutyID, BuildCommitteeDutyID). This eliminates complex state management and makes duty IDs available for all message types including Events.
Code Organization: Created new utility files (observability/log/fields/duty_id.go, observability/log/fields/format.go) to consolidate formatting logic and removed unused constants/functions from the main fields package.
Observability Improvements: Removed duplicate trace attributes that were being logged elsewhere and standardized time field handling (StartTimeUnixMilli → SlotStartTime with proper time.Time type).
These changes prepare the codebase for implementing duty trace IDs (issue #1828) by establishing consistent logging patterns and ensuring all duty-related operations have proper contextual information for debugging and monitoring in the distributed consensus system.
Confidence score: 4/5
- This PR involves extensive refactoring of logging infrastructure with potential impact on debugging capabilities if not thoroughly tested
- Score reflects comprehensive changes to logging patterns that could affect troubleshooting if field names or context are incorrect
- Pay close attention to test files and ensure all logger usage patterns are consistently applied across the codebase
Context used:
Rule - Prefer shorter, clearer names for methods and constants. If one condition implies another (e.g., attesting implies participating), use the more specific name rather than combining both concepts. (link)
28 files reviewed, no comments
…l the affected code & would require additional testing)
oleg-ssvlabs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left few questions. Great changes!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors logger usage throughout the SSV codebase to standardize and improve how logging context (slot, dutyID, runner roles, etc.) is set up and maintained. The refactoring consolidates logger field management, introduces new utility functions for building duty IDs and formatting fields, and removes redundant logging code patterns.
- Standardizes logger field setup across validator and committee message handling
- Introduces helper functions for building duty IDs and formatting fields consistently
- Removes duplicate or redundant logger context setup code
Reviewed Changes
Copilot reviewed 33 out of 34 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/spec-alignment/differ.config.yaml | Adds new approved changes to the configuration |
| protocol/v2/types/messages.go | Updates comments for EventType constants |
| protocol/v2/ssv/validator/validator_queue.go | Refactors HandleMessage to EnqueueMessage with standardized logger setup |
| protocol/v2/ssv/validator/validator.go | Removes dutyIDs map and simplifies ProcessMessage logger setup |
| protocol/v2/ssv/validator/timer.go | Updates field name from Role to RunnerRole |
| protocol/v2/ssv/validator/non_committee_validator.go | Updates field names and error messages |
| protocol/v2/ssv/validator/duty_executor.go | Simplifies logger setup and removes redundant code |
| protocol/v2/ssv/validator/committee_queue_test.go | Updates test logger setup and method calls |
| protocol/v2/ssv/validator/committee_queue.go | Refactors HandleMessage to EnqueueMessage with improved context |
| protocol/v2/ssv/validator/committee.go | Major refactoring of logger setup and trace context building |
| protocol/v2/ssv/runner/*.go | Updates field names from Height/Round to QBFTHeight/QBFTRound |
| protocol/v2/ssv/queue/messages.go | Improves Slot() method error handling |
| protocol/v2/qbft/instance/*.go | Updates logging field names for consistency |
| protocol/v2/message/msg.go | Removes unused SSVSyncMsgType constant |
| operator/validator/controller*.go | Updates method calls and field names |
| operator/duties/scheduler.go | Updates field names for consistency |
| observability/log/fields/*.go | Major refactoring of field functions and formats |
| message/validation/*.go | Updates field names for consistency |
| exporter/api/query_handlers.go | Updates field name usage |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
This is a follow-up to #2400 to address the following:
loggerwith all the necessary context (slot, dutyID, etc.) - so it's very clear what fields are added in every case; This is to make sure there won't be a "(part 3)" necessary