refactor: improve logging #51
Conversation
📝 WalkthroughWalkthroughThis pull request consolidates logging across various modules including blame management, TSS operations, key generation, key signing, and P2P communications. Logger parameters are now consistently injected into functions that previously relied on global or static log instances. Import statements have been updated to use a unified logging package, and logging statements have been refactored to adopt structured logging practices. In addition, network configuration for TSS initialization has been streamlined via a consolidated structure, further standardizing error handling and context propagation. Changes
Assessment against linked issues
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 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 (
|
swift1337
requested review from
a team and
gartnera
and removed request for
a team
![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
🧹 Nitpick comments (9)
p2p/metric_reporter.go (5)
44-45: Fix typo in comment.There's a minor typo in this comment: "ac connection" should be "a connection".
-// AllowPeer is invoked when attaching ac connection to a peer is allowed +// AllowPeer is invoked when attaching a connection to a peer is allowed
61-63: Consider using structured fields for protocol blocking.While other methods have been updated to use fully structured logging, this method still uses
Msgf. Consider converting it to use structured fields for consistency.-func (rmr *ResourceMetricReporter) BlockProtocol(proto protocol.ID) { - rmr.logger.Error().Msgf("Protocol is blocked: %s", proto) +func (rmr *ResourceMetricReporter) BlockProtocol(proto protocol.ID) { + rmr.logger.Error().Stringer("protocol", proto).Msg("Protocol is blocked") }
69-71: Consider using structured fields for service blocking.This method still uses
Msgfwhile others use structured fields. For consistency, consider converting it to use structured logging.-func (rmr *ResourceMetricReporter) BlockService(svc string) { - rmr.logger.Error().Msgf("Service %q is blocked", svc) +func (rmr *ResourceMetricReporter) BlockService(svc string) { + rmr.logger.Error().Str("service", svc).Msg("Service is blocked") }
74-76: Consider using structured fields for service peer blocking.This method still uses
Msgfwith the peer ID as a structured field. For consistency, consider converting the service name to a structured field as well.-func (rmr *ResourceMetricReporter) BlockServicePeer(svc string, p peer.ID) { - rmr.logger.Error().Stringer(logs.Peer, p).Msgf("Service: %q from peer is blocked", svc) +func (rmr *ResourceMetricReporter) BlockServicePeer(svc string, p peer.ID) { + rmr.logger.Error().Stringer(logs.Peer, p).Str("service", svc).Msg("Service from peer is blocked") }
82-84: Consider using structured fields for memory blocking.For consistency with other methods, consider using structured fields for the memory size instead of
Msgf.-func (rmr *ResourceMetricReporter) BlockMemory(size int) { - rmr.logger.Error().Msgf("Memory blocked, size: %d", size) +func (rmr *ResourceMetricReporter) BlockMemory(size int) { + rmr.logger.Error().Int("size", size).Msg("Memory blocked") }tss/tss.go (2)
64-70: Consider expanding documentation forNetworkConfig.While this new struct is quite straightforward, adding a docstring to clarify each field’s purpose (e.g., clarifying port usage, categories of whitelisted peers, etc.) can help future maintainers and users.
74-81: Validate network configuration before usage.This updated
Newfunction takes a singleNetworkConfigstruct. Consider validating thatExternalIPis non-empty andPortis a valid positive integer before continuing, to avoid misconfiguration issues.keysign/eddsa/tss_keysign.go (2)
114-114: Refine the log phrasing for clarity.The message
"we are not in this rounds key sign"could be made more formal, e.g."Local party is not participating in this key sign round."This improves clarity and tone.- tKeySign.logger.Info().Msg("we are not in this rounds key sign") + tKeySign.logger.Info().Msg("Local party is not participating in this key sign round")
208-208: Confirm usage ofCmp(b) != -1.
Cmp(b) != -1is effectivelya >= b. For better readability, consider usingCmp(b) >= 0. This is a minor enhancement, yet it often reads more naturally.- return a.Cmp(b) != -1 + return a.Cmp(b) >= 0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
blame/manager.go(2 hunks)blame/policy_test.go(2 hunks)cmd/tss/main.go(3 hunks)cmd/tss/tss_http.go(1 hunks)common/tss.go(5 hunks)common/tss_helper_test.go(3 hunks)common/tss_test.go(4 hunks)keygen/ecdsa/keygen_test.go(5 hunks)keygen/ecdsa/tss_keygen.go(3 hunks)keygen/eddsa/keygen_test.go(4 hunks)keygen/eddsa/tss_keygen.go(2 hunks)keysign/ecdsa/keysign_old_test.go(5 hunks)keysign/ecdsa/keysign_test.go(5 hunks)keysign/ecdsa/tss_keysign.go(8 hunks)keysign/eddsa/keysign_test.go(3 hunks)keysign/eddsa/tss_keysign.go(6 hunks)keysign/signature_notifier.go(5 hunks)keysign/signature_notifier_test.go(5 hunks)logs/fields.go(1 hunks)p2p/communication.go(11 hunks)p2p/communication_test.go(5 hunks)p2p/metric_reporter.go(1 hunks)p2p/party_coordinator.go(10 hunks)p2p/party_coordinator_test.go(3 hunks)p2p/party_coordinator_test_with_leader_test.go(4 hunks)tss/keygen.go(9 hunks)tss/keysign.go(10 hunks)tss/tss.go(5 hunks)tss/tss_4nodes_test.go(1 hunks)tss/tss_4nodes_zeta_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test go 1.22
🔇 Additional comments (154)
blame/manager.go (2)
9-10: Import update to use structured logging package.Good change to remove the direct import of
github.com/rs/zerolog/login favor of the customgithub.com/zeta-chain/go-tss/logspackage. This ensures consistent logging patterns across the codebase.
28-32: Function signature update to accept logger parameter.Good refactoring to explicitly receive a logger parameter instead of relying on a global instance. This follows the dependency injection pattern, making the code more testable and flexible.
logs/fields.go (2)
5-12: Well-structured constant definitions for log fields.The constant definitions provide a standardized way to refer to log fields across the codebase, which helps maintain consistency and reduces the likelihood of typos in field names.
14-21: Utility function to extract party information for structured logging.This is a useful helper function that encapsulates the extraction of party information for logging purposes. The function returns a map with appropriate fields, making it easy to include party details in structured logs.
keysign/ecdsa/keysign_old_test.go (5)
146-147: Updated test to use dependency injection for logger.Correctly updated the test to pass a logger instance to the
p2p.NewCommunicationfunction, consistent with the updated function signature.
152-155: Updated bootstrapped communication initialization with logger.This change correctly propagates the logger to the communication instances, ensuring consistent logging throughout the test.
287-292: Updated TssKeySign instantiation with logger parameter.The test now correctly passes the logger to the
NewTssKeySignfunction, consistent with the updated function signature.
364-370: Updated TssKeySign instantiation in TestSignMessageRejectOnePeer.Correctly updated all test instances to include the logger parameter in the
NewTssKeySignfunction call.
421-432: Updated TestCloseKeySignnotifyChannel to include logger parameter.The test now correctly passes the logger to the
NewTssKeySignfunction, ensuring consistency with the updated function signature.keysign/ecdsa/tss_keysign.go (9)
25-26: Updated imports to use structured logging package.Good replacement of direct zerolog import with the custom logs package, ensuring consistent logging patterns throughout the codebase.
51-54: Enhanced function signature to accept logger parameter.The
NewTssKeySignfunction now accepts a logger parameter and properly enhances it with component and message ID context. This follows the pattern of explicit logger propagation throughout the application.
56-58: Updated constructor to use injected logger.Good implementation of logger initialization and propagation to the TssCommon instance. This ensures consistent logging patterns throughout the keysign process.
87-88: Enhanced logging with structured fields.Good use of structured logging with the
Fieldsmethod and thelogs.Partyhelper function. This improves log clarity and makes it easier to filter logs by party information.
107-108: Simplified log message format.Improved log message format by removing unnecessary string formatting and using direct message logging instead.
131-132: Enhanced log with structured fields for keysign parties.Good use of structured logging with string array fields to clearly log the parties involved in the keysign operation.
195-196: Enhanced success log with structured host information.Good use of structured logging with the
Stringermethod to include the host ID in the success message.
201-202: Simplified comparison logic.Good simplification of the comparison logic in the sorting function. The new approach is more concise and easier to understand.
229-232: Enhanced timeout log with structured duration information.Excellent enhancement to include the timeout duration in the log message. This provides important context for troubleshooting timeout issues.
cmd/tss/tss_http.go (1)
146-146: Enhanced logging with structured fields.The change replaces simple message logging with structured logging that includes the entire request object, aligning with the PR's objective to enhance logs with structured fields. This approach improves observability and makes debugging easier by providing complete request context in a queryable format.
blame/policy_test.go (2)
12-13: Added zerolog import for logger propagation.The import of the zerolog package is necessary to support the pattern of logger propagation throughout the project components, consistent with the PR's objectives.
44-45: Updated BlameManager to use zerolog.The BlameManager initialization now includes a logger parameter (using zerolog.Nop() for tests), which ensures consistent logging behavior throughout the blame management component. This aligns with the PR's goal of propagating nested loggers with consistent identifiers.
keysign/signature_notifier_test.go (5)
15-16: Added zerolog import for signature notifier.The import of the zerolog package supports the logger propagation pattern being implemented throughout the project. This is consistent with the PR objectives.
25-26: Initialized no-op logger for tests.Creating a no-op logger for tests is a good practice as it ensures the test behavior is consistent while maintaining the same interface as production code. This implementation maintains test simplicity while supporting the structured logging pattern.
66-68: Updated SignatureNotifier constructor calls.The SignatureNotifier initialization now includes a logger parameter, consistent with the pattern implemented throughout the codebase. This ensures standardized logging behavior in the signature notification component.
99-100: Consistent logger usage in multiple test functions.The consistent use of no-op loggers across test functions demonstrates a systematic approach to logger propagation. This implementation ensures all components in the TSS service adhere to the same logging pattern.
139-141: Updated SignatureNotifier constructor in second test.The SignatureNotifier initialization in the second test function follows the same pattern, ensuring consistency across all test cases. This systematic implementation reinforces the PR's goal of standardized logging throughout the codebase.
common/tss_test.go (4)
21-22: Added zerolog log import for TssCommon tests.The import of the zerolog log package is necessary to support the updated constructor signature for TssCommon, consistent with the project-wide logging enhancements.
140-141: Updated TssCommon constructor with logger parameter.The TssCommon initialization now includes a logger parameter, ensuring that structured logging is propagated throughout the TSS processing chain. This change aligns with the PR objective of standardizing logging across the project.
234-235: Consistent logger usage in test setup.The setupProcessVerMsgEnv function now includes a logger parameter when initializing TssCommon, maintaining consistency with the updated constructor pattern throughout the project. This systematic implementation ensures all test environments follow the same logging architecture.
483-484: Updated TssCommon initialization in TestTssCommon.The TssCommon initialization in the TestTssCommon test case follows the new constructor pattern, ensuring consistency throughout the codebase. This implementation completes the systematic propagation of loggers to all TSS components.
tss/tss_4nodes_zeta_test.go (2)
243-249: Well-structured network configuration consolidationThe introduction of
NetworkConfigstructure effectively consolidates related configuration parameters, enhancing maintainability and readability. This approach aligns with the PR's objectives of streamlining the TSS service constructor.
251-258: Clean parameter organization in constructor callThe updated
Newfunction call uses the consolidatednetworkConfigstructure as its first parameter, followed by other necessary parameters in a logical order. This improves readability and makes the function signature more maintainable for future changes.cmd/tss/main.go (3)
16-16: Appropriate logger importThe addition of the zerolog import with a clear alias supports the PR objective of establishing consistent logging practices throughout the project.
59-65: Well-structured network configurationThe
networkConfigstructure consolidates related parameters, providing a cleaner and more maintainable approach to configuration management. This implementation is consistent with the changes in the test files.
68-75: Improved constructor call with structured parametersThe updated
Newfunction call now uses the consolidated network configuration and properly passes the logger instance instead of nil. This enhances error tracking capabilities and aligns with the PR's objective of propagating loggers throughout the system.p2p/party_coordinator_test.go (3)
13-13: Required logger package importThe addition of the zerolog import supports the injection of logger instances in the test functions, aligning with the PR's objective of standardizing logging throughout the codebase.
50-50: Logger integration in party coordinator testUsing
zerolog.Nop()as a parameter toNewPartyCoordinatoris a suitable choice for tests as it provides a no-operation logger that doesn't produce output, avoiding test pollution while maintaining the correct interface.
89-89: Consistent logger usage in timeouts testThe addition of
zerolog.Nop()as a parameter toNewPartyCoordinatormaintains consistency with other test implementations while incorporating the new logging requirements.p2p/party_coordinator_test_with_leader_test.go (2)
13-13: Required logger package importThe addition of the zerolog import supports the logger parameter requirements for the party coordinator tests, consistent with the project-wide logging standardization.
111-111: Consistent logger usage across all party coordinator testsThe addition of
zerolog.Nop()as the third parameter to allNewPartyCoordinatorfunction calls ensures consistent logger injection throughout the test suite, aligning with the PR's objectives of structured logging implementation.Also applies to: 149-149, 230-230
common/tss_helper_test.go (3)
14-14: Good addition of zerolog import.This import properly supports the added zerolog parameter in the constructor calls.
74-74: Properly updated constructor call with logger parameter.You've correctly added the no-operation logger instance as the last parameter to the
NewTssCommonconstructor, ensuring test compatibility with the updated function signature.
90-90: Consistent constructor update with logger parameter.Consistent with the previous constructor call update, this provides a no-operation logger for testing purposes.
tss/tss_4nodes_test.go (2)
385-391: Excellent refactoring to NetworkConfig structure.This refactoring improves code clarity by consolidating related configuration parameters into a single structure, making the code more maintainable and the intent clearer.
393-400: Clean constructor call with improved parameter organization.The updated
Newfunction call correctly uses the consolidatednetworkConfigstructure and includes the logger parameter, aligning with the PR objectives of streamlining the constructor and propagating loggers throughout the codebase.keysign/ecdsa/keysign_test.go (6)
153-153: Properly updated Communication constructor with logger parameter.The addition of the logger parameter to the constructor call ensures consistent logging throughout the communication layer.
159-159: Consistent constructor update with logger parameter.Similar to the previous constructor call, this ensures logger propagation is maintained consistently.
212-217: Well-formatted constructor call with logger parameter.The updated function call with improved formatting enhances readability, and the addition of the logger parameter ensures consistent logging.
318-323: Consistent constructor update with maintained formatting.The updated function call maintains consistent formatting and properly includes the logger parameter.
420-426: Proper logger integration in final constructor call.The consistent pattern of logger injection is maintained through all constructor calls in the file, ensuring uniform logging behavior.
477-488: Clean formatting with proper logger parameter.The constructor call's improved formatting with each parameter on a new line enhances readability and maintainability.
keysign/signature_notifier.go (6)
17-17: Added logs package import for structured logging.The addition of this import enables the use of standardized log field names, improving consistency across the codebase.
44-49: Excellent logger enhancement with contextual information.The updated constructor now properly enriches the logger with component and host information, providing better context for log entries. This enhances log filtering and analysis capabilities.
116-116: Improved error logging with standard method.Changed from custom error message format to using zerolog's structured error method, improving consistency and log parsing.
123-123: Enhanced error logging with peer context.The updated error logging now includes peer information using structured fields, improving the diagnostic value of the log entry.
157-157: Structured log entry with peer information.The debug log now uses structured logging with standardized field names, improving consistency and searchability.
225-228: Comprehensive structured error logging.The error logging has been significantly improved with structured fields for message ID and peer information, enhancing troubleshooting capabilities by providing full context in a machine-parseable format.
p2p/metric_reporter.go (5)
8-9: Import added for structured logging fields.The addition of the logs package is appropriate for accessing standardized field names used across the codebase.
16-19: Good constructor enhancement with logger propagation.The constructor now receives a logger parameter and properly initializes it with component context, aligning with the PR objective of propagating nested loggers throughout the project.
26-31: Improved logging with structured fields.The logging has been enhanced from simple formatted messages to structured logging with specific fields for connection direction and file descriptor usage. This change improves log analysis capabilities.
37-42: Enhanced stream blocking log with structured fields.The change to use structured logging with standardized fields for peer ID and connection direction is consistent with the PR's objective to enhance logs with structured fields.
48-50: Simplified peer blocking log with structured fields.The log statement now uses the standardized
logs.Peerfield, making it consistent with other structured logs in the codebase.tss/keysign.go (11)
24-25: Import added for structured logging fields.The addition of the logs package is appropriate for accessing standardized field names used across the codebase.
92-97: Enhanced logging for non-active signer.The logging has been improved to use structured fields, providing better context with the host ID when reporting that the node is not an active signer.
133-138: Improved error logging with structured fields.The error log now includes the online peers as a structured field, providing better context for debugging when a keysign party fails to form.
159-166: Enhanced error logging with message ID and peer information.The addition of the message ID as a structured field improves traceability. Including peers in the error context is valuable for diagnosing issues with keysign party formation.
180-186: Improved logging for non-active signer with message ID.The log now includes both the message ID and host ID as structured fields, providing better context and traceability.
240-241: Enhanced request logging with structured fields.The initial logging of the keysign request now uses structured fields for the message ID and includes the entire request object, improving traceability and debugging context.
261-263: Logger propagation to keysign instance.The logger is now properly propagated to the ECDSA keysign instance, ensuring consistent logging context throughout the keysign process.
275-277: Logger propagation to EdDSA instance.The logger is now properly propagated to the EdDSA keysign instance, ensuring consistent logging context throughout the keysign process.
322-323: Simplified error logging.The error logging for hash conversion failures has been simplified and standardized.
325-327: Simplified error logging for consistency.Similar to the previous hash conversion error, this log has been simplified and standardized.
352-357: Enhanced logging for insufficient signers.The error log for insufficient signers now includes structured fields for the threshold and actual signer count, providing better context for debugging.
keysign/eddsa/keysign_test.go (4)
151-152: Updated communication constructor with logger parameter.The constructor for the first communication instance now includes the logger parameter, aligning with the signature change in the p2p package.
157-158: Updated communication constructor with logger parameter for other nodes.Similar to the first instance, the constructor for subsequent communication instances now includes the logger parameter.
207-216: Updated keysign instance creation with logger parameter.The constructor for the keysign instance now includes the logger parameter, ensuring proper logging context propagation during testing.
269-270: Updated keysign instance creation in test with logger parameter.The constructor for the keysign instance in the
TestCloseKeySignnotifyChanneltest now includes the logger parameter to match the updated function signature.keygen/ecdsa/tss_keygen.go (4)
21-22: Import added for structured logging fields.The addition of the logs package is appropriate for accessing standardized field names used across the codebase.
39-51: Enhanced constructor with logger parameter.The constructor now receives a logger parameter, aligning with the PR objective of propagating nested loggers throughout the project.
52-56: Excellent logger initialization with context.The logger is initialized with component and message ID fields, providing valuable context for all subsequent log entries from this module.
57-62: Proper logger propagation to struct and common component.The logger is correctly assigned to the struct field and passed to the common component, ensuring consistent logging context throughout the key generation process.
p2p/communication.go (10)
24-24: Good update to use the centralized logging package.This change ensures consistent logging practices throughout the codebase by importing a centralized logging package.
70-70: Good enhancement to the function signature.Adding a logger parameter to the constructor follows the dependency injection pattern, making the code more testable and preventing reliance on global state. This aligns with the PR's goal of propagating loggers throughout the project.
85-85: Excellent structured logging implementation.The logger is properly configured with the component name, which aligns with the PR objectives of using structured logging with consistent identifiers (
component=$component). This will significantly improve log filtering and analysis.
130-134: Great improvement to error logging with structured fields.This change enhances the error logging by including structured fields for peer ID and message ID, making logs more informative and easier to filter.
148-149: Improved error message clarity.The error message now avoids embedding the peer ID directly in the error string, instead relying on the structured logging to capture this information. This is a better practice as it keeps error messages concise while ensuring all relevant information is still logged.
175-175: Better structured error logging.This change improves the logging by adding the peer ID as a structured field, enhancing log searchability and context.
196-202: Comprehensive structured logging for timeout events.This is an excellent enhancement to the timeout logging, with structured fields for message ID, peer ID, protocol, message type, and timeout duration. This will make debugging timeout issues much more effective.
247-247: Enhanced peer connection failure logging.Using structured logging with the peer information improves the error context.
406-406: Better peer connection error logging.Structured logging with peer information enhances the error context for connection failures.
411-411: Informative connection success logging.This structured log provides clear information about successful bootstrap node connections.
keygen/eddsa/keygen_test.go (5)
113-114: Good logger propagation in test setup.Updating the
NewCommunicationcall to include the logger parameter ensures consistent logging throughout the test environment.
119-120: Consistent logger propagation for bootstrap peers.The logger is properly passed to the bootstrapped communications instance, ensuring log consistency across all test nodes.
166-177: Enhanced constructor with logger parameter.The
Newfunction now receives the logger as a parameter, which ensures proper log context propagation throughout the key generation process.
208-208: Consistent logger usage in test scenario.The test correctly passes the logger to the key generation instance, maintaining consistency with production code.
217-217: Proper logger injection in notification test.The test correctly passes the logger to the key generation instance for the notification channel test.
tss/keygen.go (7)
32-44: Good enhancement to ECDSA key generation initialization.Adding the p2p communication instance and logger to the
NewTssKeyGenfunction ensures proper context propagation and structured logging throughout the key generation process.
46-57: Consistent parameter enhancement for EdDSA key generation.The
eddsa.Newfunction now receives the same p2p communication and logger parameters as its ECDSA counterpart, ensuring consistency in the codebase.
90-95: Enhanced error logging with structured fields.The error log now includes structured fields for peers and timeout duration, providing better context for troubleshooting join party failures.
113-116: Improved blame logging with structured fields.The error log now includes peer information as a structured field, making it easier to identify problematic peers during party formation.
147-150: Better structured error logging for party formation failures.This error log now provides clearer context with structured peer information when party formation fails.
295-298: Enhanced leader blame logging.The error log for the leaderless join party scenario now includes structured fields for better analysis.
328-331: Improved error logging for party formation failures.This error log now includes structured peer information, enhancing troubleshooting capabilities.
p2p/party_coordinator.go (17)
18-18: Good update to use the centralized logging package.This change ensures consistent logging practices by importing the project's centralized logging package.
41-43: Excellent constructor enhancement with logger parameter.The
NewPartyCoordinatorfunction now accepts a logger parameter and properly configures it with the component context. This follows the dependency injection pattern and ensures consistent logging throughout the party coordination process.
49-50: Proper logger initialization in struct.The logger is correctly assigned to the struct field, ensuring it's available throughout the lifetime of the PartyCoordinator.
78-81: Enhanced missing message ID logging.The log now includes structured peer information, providing better context for debugging.
87-97: Improved leader validation logging.The code has been refactored to improve readability and the logging now includes detailed structured fields for the peer, leader, and message ID, enhancing troubleshooting capabilities.
104-109: Better error logging for stream write failures.The log now includes structured fields for peer, leader, and message ID, providing comprehensive context for debugging.
121-121: Enhanced party readiness logging.The log now includes the message ID as a structured field, improving context for debugging party readiness issues.
138-139: Good scoped logger implementation.Creating a scoped logger with peer context enhances log readability and correlation.
142-143: Improved error logging with method chaining.Using the
Err()method provides cleaner structured logging for stream reading failures.
158-159: Cleaner info logging.The log message is now more concise and follows best practices.
171-172: Good scoped logger implementation for leader handling.Creating a scoped logger with peer context enhances log readability and correlation in the leader handler.
175-176: Improved error logging using method chaining.Using the
Err()method provides cleaner structured logging for stream reading failures.
188-188: Better debug logging with formatted message.Using
Msgfappropriately for dynamic content in debug logs.
198-199: Enhanced error logging for leader response failures.Using structured logging improves error context.
315-318: Improved stream closure error logging.The error log now includes the message ID as a structured field, enhancing troubleshooting capabilities for stream closure issues.
330-333: Better peer response failure logging.The log now includes the peer as a structured field, providing better context for debugging response issues.
398-399: Enhanced leader error logging.Using structured logging with the leader public key improves error context.
keygen/ecdsa/keygen_test.go (6)
127-127: Logger parameter properly added to NewCommunication callThe logger parameter has been consistently added to the p2p.NewCommunication calls, implementing structured logging. This aligns with the PR objective of propagating loggers throughout the project.
133-133: Logger parameter properly added to bootstrapped NewCommunication callThe logger parameter has been correctly added to the bootstrapped version of the p2p.NewCommunication call as well, maintaining consistency across all communication initialization points.
204-205: Logger parameter correctly added to NewTssKeyGen callThe logger parameter has been appropriately added to NewTssKeyGen, enabling structured logging throughout the key generation process. This enhances observability of the TSS operations.
262-263: Logger parameter correctly added to NewTssKeyGen test with stopThe logger parameter has been properly added to the NewTssKeyGen call in the test case that verifies stopping behavior. Consistent with other changes in the file.
310-311: Logger parameter correctly added to NewTssKeyGen error testThe logger parameter has been properly added to the NewTssKeyGen call in the error test case. This maintains consistency throughout the test suite.
331-332: Logger parameter correctly added to final NewTssKeyGen test caseThe logger parameter has been properly added to the NewTssKeyGen call in the test case for closing the key generation notification channel.
keygen/eddsa/tss_keygen.go (5)
20-20: Appropriate logger package importThe import of the logs package has been correctly added, replacing the direct log package import. This provides access to structured logging constants and utilities.
47-47: Logger parameter added to New constructorThe logger parameter has been properly added to the New function signature, enabling the propagation of structured logging through the key generation component.
49-49: Enhanced logger with component and message ID contextThe logger is now enhanced with component=keygen and message ID fields, implementing structured logging for improved traceability as specified in the PR objectives.
52-52: Using injected logger instead of creating a new oneThe KeyGen struct now correctly uses the injected logger with context instead of creating a new one. This is a good practice for maintaining logging context.
54-54: Logger properly propagated to TssCommonThe logger is correctly passed to the NewTssCommon constructor, ensuring that structured logging is consistently applied throughout the component hierarchy.
common/tss.go (6)
62-62: Logger parameter added to NewTssCommon constructorThe logger parameter has been appropriately added to the NewTssCommon function signature, enabling propagation of structured logging through this core component.
66-66: Using injected logger directlyThe TssCommon struct now correctly uses the injected logger directly instead of creating a new one. This maintains the logging context provided by the caller.
80-80: Logger propagated to blame managerThe logger is correctly passed to the blame.NewBlameManager constructor, ensuring that structured logging is consistently applied to the blame management component.
142-142: Improved error logging formatThe error logging has been simplified to use Msg() instead of Msgf() where no formatting is needed. This improves code clarity and maintains consistent log structure.
207-207: Error logging format improvedSimilar to the previous change, the error logging has been simplified to use Msg() for better readability and consistency in the log structure.
606-606: Error logging format improved for blame managementThe error logging in the blame management code has been updated to use the simpler Msg() format for better readability and consistency.
p2p/communication_test.go (7)
10-10: Add zerolog logger importThe zerolog logger has been correctly imported for use in the test file, enabling structured logging in tests.
22-22: Logger parameter added to NewCommunication in basic testThe logger parameter has been appropriately added to the NewCommunication call in the basic communication test.
65-65: Logger parameter added to NewCommunication in P2P testThe logger parameter has been properly added to the NewCommunication call in the P2P communication test.
71-71: Logger parameter added to second NewCommunication instanceThe logger parameter has been correctly added to the second NewCommunication call (comm2) in the test.
81-81: Logger parameter added to third NewCommunication instanceThe logger parameter has been correctly added to the third NewCommunication call (comm3) in the test, maintaining consistency.
88-94: Logger parameter added to fourth NewCommunication instanceThe logger parameter has been correctly added to the fourth NewCommunication call (comm4) in the test. The formatting is clean and consistent.
108-114: Logger parameter added to fifth NewCommunication instanceThe logger parameter has been correctly added to the fifth NewCommunication call (comm5) in the test. This completes the consistent application of the logger parameter across all test instances.
tss/tss.go (6)
8-24: Adopt consistent imports organization.The newly introduced imports (
time,github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1,github.com/pkg/errors, andgithub.com/zeta-chain/go-tss/logs) align well with their usage in the code, especiallygithub.com/pkg/errorsfor contextual error wrapping. Keeping them in a coherent order or grouping (standard library vs. external) can improve readability if it becomes a team-wide convention, but there's no intrinsic issue here.
83-96: Good use of error wrapping and bech32 conversion.Retrieving the private key bytes and converting them into a bech32 public key are nicely encapsulated. The usage of
errors.Wrapclarifies the source of any failures. This is clean and follows best practices.
98-114: Efficient reorganization for state manager, P2P startup, and logging.These refactors provide clarity:
• Merging local state with provided bootstrap peers inresolveBootstrapPeersis logically sound.
• Instantiating outcomes (metrics, signature notifier) in well-labeled steps.
• Applying a structured logger consistently improves trackability.All these changes enhance maintainability while retaining functionality.
Also applies to: 116-116, 119-120, 123-125, 131-133, 139-139, 142-142
255-273: Peer gathering logic is clearly outlined.Iterating over
host.Network().Conns()to collect peer info is straightforward and maintainable. No immediate issues found, and the approach is suitable for enumerating known peers.
293-340: Thorough approach to merge and filter bootstrap peers.The logic in
resolveBootstrapPeershandles whitelisted peers, merges local addresses, and deduplicates them. This ensures only authorized peers are retained. The code appears secure and well-structured.
342-361: Pre-parameter logic is well-handled.Using
ensurePreParamsto generate and validate pre-params reduces clutter in the parent function. The concurrency-limited generation frombkeygen.GeneratePreParamslooks appropriate for typical keygen performance constraints.keysign/eddsa/tss_keysign.go (4)
24-24: Import of structured logging package looks good.Replacing the old approach with
"github.com/zeta-chain/go-tss/logs"fosters consistency across the codebase.
50-56: Injecting a logger parameter improves testability.Allowing
Newto accept azerolog.Loggerpromotes more readable, structured logs. The additional context (component,msgID) is particularly helpful for debugging multi-party operations.
85-87: Structured logging for key sign party operations is effective.Using
Fields(logs.Party(eachParty))ensures relevant party details are captured in logs. This approach helps identify which local party faced issues.Also applies to: 90-90, 93-93
200-203: Consistent structured logging for success messages.Attaching
"host", tKeySign.p2pComm.GetHost().ID()clarifies the origin of the success event. This is aligned with the new logging pattern.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
keysign/request.go (1)
24-27: Consider including additional relevant fields in the zerolog object marshaling.The implementation correctly adds the
MessagesandBlockHeightfields to the structured log, however, theRequeststruct contains other potentially relevant fields likePoolPubKey,SignerPubKeys, andVersionwhich might be valuable for debugging and tracing purposes.func (r *Request) MarshalZerologObject(e *zerolog.Event) { e.Strs("request.messages", r.Messages) e.Int64("request.block_height", r.BlockHeight) + e.Str("request.pool_pub_key", r.PoolPubKey) + e.Strs("request.signer_pub_keys", r.SignerPubKeys) + e.Str("request.version", r.Version) }keysign/ecdsa/tss_keysign.go (2)
132-132: Consider improving log message clarityThe log message "Keysign parties" could be enhanced for better clarity.
- tKeySign.logger.Info().Strs("parties", parties).Msg("Keysign parties") + tKeySign.logger.Info().Strs("parties", parties).Msg("Identified keysign participants")
152-160: Enhance error message differentiationThe error messages for setting up ID maps are very similar and only differ by "#1" and "#2". Consider providing more specific context in these error messages.
- err = conversion.SetupIDMaps(partyIDMap, tKeySign.tssCommonStruct.PartyIDtoP2PID) - if err != nil { - return nil, fmt.Errorf("fail to setup id maps #1: %w", err) - } - - err = conversion.SetupIDMaps(partyIDMap, blameMgr.PartyIDtoP2PID) - if err != nil { - return nil, fmt.Errorf("fail to setup id maps #2: %w", err) + if err = conversion.SetupIDMaps(partyIDMap, tKeySign.tssCommonStruct.PartyIDtoP2PID); err != nil { + return nil, fmt.Errorf("fail to setup common structure ID maps: %w", err) + } + + if err = conversion.SetupIDMaps(partyIDMap, blameMgr.PartyIDtoP2PID); err != nil { + return nil, fmt.Errorf("fail to setup blame manager ID maps: %w", err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cmd/tss/tss_http.go(1 hunks)keysign/ecdsa/tss_keysign.go(8 hunks)keysign/eddsa/tss_keysign.go(6 hunks)keysign/request.go(2 hunks)p2p/communication.go(10 hunks)tss/keysign.go(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/tss/tss_http.go
- tss/keysign.go
- keysign/eddsa/tss_keysign.go
🔇 Additional comments (18)
keysign/request.go (1)
3-4: Appropriate import addition for structured logging.The addition of the zerolog import aligns with the PR objective to enhance logging functionality across the codebase.
keysign/ecdsa/tss_keysign.go (5)
87-87: LGTM: Good use of structured loggingThe log message has been improved with structured fields for party information, enhancing the ability to filter and analyze logs.
107-108: Improved log message concisenessThe log message has been simplified to use a non-formatted message, which improves readability.
200-200: Good use of structured logging for host informationUsing Stringer for host ID provides a consistent and standardized way to log host information.
206-206: Simplified comparison logicGood simplification of the comparison logic in the sort function, making the code more concise.
234-237: Enhanced timeout logging with structured fieldsThe timeout logging has been improved to include the timeout duration as a structured field, which provides valuable context for troubleshooting timeout issues.
p2p/communication.go (12)
24-24: Approve the import package change for unified logging.Switching to a dedicated logging package improves the consistency of log messages across the codebase and centralizes logging configuration.
70-70: Approve constructor signature update for dependency injection.Adding the logger parameter to the constructor follows dependency injection best practices, making the code more maintainable and testable.
85-85: Approve structured logger configuration with component context.The logger initialization now provides component context which enhances log filtering and analysis capabilities.
130-134: Approve enhanced error logging with structured fields.The error logging now includes peer ID and message ID as structured fields, which significantly improves debugging and traceability.
148-149: Approve error message refinement.The error message has been appropriately refined to be more generic while still including the necessary context.
165-212: Approve significant readFromStream method improvements.The method has been thoroughly restructured with:
- More efficient error handling
- Enhanced structured logging
- Better timeout logic with detailed context
- Clearer flow control
This will make debugging peer communication issues substantially easier.
254-254: Approve structured error logging for peer connectivity.Using structured logging with the Stringer method for the peer ID enhances the log quality for debugging peer connectivity issues.
271-272: Approve improved error formatting.The error message now properly wraps the original error, which improves error tracing.
313-313: Approve dependency injection in resource metrics.Passing the logger to the metrics reporter ensures consistent logging across components.
413-413: Approve structured error logging for bootstrap peer connections.The structured logging pattern with the peer info as a Stringer enhances the context of bootstrap connection errors.
418-418: Approve structured info logging for successful connections.Adding the peer information as a structured field makes successful connection logs more informative and queryable.
216-222:✅ Verification successful
Verify if select statement modifications are intended.
The restructured select statement now only checks for stop signals before proceeding to read from the stream, whereas the old pattern may have been doing more.
🏁 Script executed:
#!/bin/bash # Find the previous implementation of handleStream to verify changes rg -A 5 "func \(c \*Communication\) handleStream" --glob "*.go" -B 10Length of output: 881
Clarify Select Statement Intent
After reviewing the previous implementation via the grep command, it’s clear that the
selectblock inhandleStreamis intentionally structured to simply check for a stop signal before callingc.readFromStream(stream). There is no evidence of additional logic (or removed logic) that would have changed its behavior. The refactoring appears deliberate and aligned with the current design.
- The block checks for
c.stopChanand returns immediately if a stop signal is received.- Otherwise, it proceeds to read from the stream without any extra conditions.
The implementation is consistent with the intended functionality. No modifications seem necessary.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
keysign/request.go (2)
35-37: Add validation for map input in SetLogFields.The method currently directly assigns the input map without validation. Consider adding a nil check to prevent potential issues.
func (r *Request) SetLogFields(kv map[string]any) { + if kv == nil { + r.logFields = make(map[string]any) + return + } r.logFields = kv }
25-37: Add documentation for new methods.Consider adding Go-style documentation comments for the newly added methods to explain their purpose and usage.
+// MarshalZerologObject implements zerolog.LogObjectMarshaler interface for structured logging func (r *Request) MarshalZerologObject(e *zerolog.Event) { e.Strs("request.messages", r.Messages) e.Int64("request.block_height", r.BlockHeight) if len(r.logFields) != 0 { e.Fields(r.logFields) } } +// SetLogFields sets additional logging fields for the request func (r *Request) SetLogFields(kv map[string]any) { r.logFields = kv }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
keysign/ecdsa/tss_keysign.go(8 hunks)keysign/request.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- keysign/ecdsa/tss_keysign.go
🔇 Additional comments (2)
keysign/request.go (2)
3-4: Added appropriate import for structured logging.The addition of the zerolog import aligns with the PR objective of enhancing logging functionality across the codebase.
26-33: Structured logging implementation follows best practices.The
MarshalZerologObjectmethod correctly implements the zerolog interface, properly logging request details and conditionally adding custom fields. This approach ensures consistent structured logging format.
|
Also tested E2E with node's repo @ |
| stream, err := c.connectToOnePeer(pID) | ||
| if err != nil { | ||
| return fmt.Errorf("fail to open stream to peer(%s): %w", pID, err) | ||
| return fmt.Errorf("fail to open stream to peer: %w", err) |
There was a problem hiding this comment.
I think we should keep the peer id in this message
There was a problem hiding this comment.
Line 131 in 31a8b77
module=tssandcomponent=$componentCloses #48
Summary by CodeRabbit
New Features
Refactor