Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat(rpc): reduce verbosity in subscriber notification logs #2028

Merged
merged 7 commits into from
Feb 26, 2025

Conversation

pedro-pelicioni-cw
Copy link
Contributor

@pedro-pelicioni-cw pedro-pelicioni-cw commented Feb 26, 2025

User description

Replace detailed subscription object display with a simple list of client names in notification logs. This significantly reduces log verbosity while maintaining essential information such as block hash, block number, and client identifiers.

Before this change, logs were showing the entire internal structure of subscription objects, resulting in extremely verbose logs that were difficult to read and analyze. Now, logs only show the client names without any internal implementation details.

This change improves log readability and reduces resource consumption in production environments, addressing feedback that logs were excessively verbose.

Tested with multiple simultaneous clients to ensure essential information is preserved even with a large number of subscribers.


PR Type

Enhancement


Description

  • Reduce log verbosity in subscriber notifications

  • Replace subscription object display with client names

  • Improve readability of logs for pending transactions

  • Enhance logging for new blocks and logs


Changes walkthrough 📝

Relevant files
Enhancement
rpc_subscriptions.rs
Refactor RPC subscription logging for improved clarity and reduced
verbosity

src/eth/rpc/rpc_subscriptions.rs

  • Replace detailed subscription object with client names in logs
  • Add conditional logging based on subscriber count
  • Improve logging for pending transactions, new blocks, and logs
  • Refactor variable names for clarity (e.g., interested_subs to
    subscribers)
  • +47/-26 

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • Replace detailed subscription object display with a simple list of client names
    in notification logs. This significantly reduces log verbosity while maintaining
    essential information such as block hash, block number, and client identifiers.
    
    Before this change, logs were showing the entire internal structure of subscription
    objects, resulting in extremely verbose logs that were difficult to read and
    analyze. Now, logs only show the client names without any internal implementation
    details.
    
    This change improves log readability and reduces resource consumption in production
    environments, addressing feedback that logs were excessively verbose.
    
    Tested with multiple simultaneous clients to ensure essential information is
    preserved even with a large number of subscribers.
    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Performance Concern

    The code creates new Vec for client names in each iteration of the loop. Consider reusing a pre-allocated vector to improve performance, especially for high-frequency events.

    let client_names: Vec<String> = subscribers.iter()
        .map(|s| s.client.to_string())
        .collect();
    Logging Improvement

    The logging statements use dynamic fields for clients, which goes against the guideline to add dynamic fields as tracing fields. Consider restructuring the log to use static messages with dynamic fields.

    tracing::info!(
        tx_hash = ?tx_hash,
        clients = ?client_names,
        "notifying subscribers about new pending transaction"
    );

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Add debug log for no subscribers

    Consider adding a debug log for cases when there are no subscribers to help with
    troubleshooting and monitoring system behavior.

    src/eth/rpc/rpc_subscriptions.rs [176-187]

     if subscribers.len() > 0 {
    -    // Extrai apenas os nomes dos clientes
    +    // Extract only client names
         let client_names: Vec<String> = subscribers.iter()
             .map(|s| s.client.to_string())
             .collect();
         
         tracing::info!(
             tx_hash = ?tx_hash,
             clients = ?client_names,
             "notifying subscribers about new pending transaction"
         );
    +} else {
    +    tracing::debug!(
    +        tx_hash = ?tx_hash,
    +        "no subscribers for new pending transaction"
    +    );
     }
    Suggestion importance[1-10]: 7

    __

    Why: Adding a debug log for cases with no subscribers enhances the system's observability and can be valuable for troubleshooting. This improvement aids in monitoring and debugging, making it a worthwhile addition to the code.

    Medium
    Use HashSet for unique client names

    Consider using a more efficient data structure like HashSet instead of Vec for
    client_names to avoid potential duplicates and improve performance.

    src/eth/rpc/rpc_subscriptions.rs [178-180]

    -let client_names: Vec<String> = subscribers.iter()
    +use std::collections::HashSet;
    +
    +let client_names: HashSet<String> = subscribers.iter()
         .map(|s| s.client.to_string())
         .collect();
    Suggestion importance[1-10]: 6

    __

    Why: Using a HashSet instead of a Vec for client names is a good optimization that can improve performance and prevent duplicates. However, the impact depends on the typical number of subscribers and the frequency of duplicate client names.

    Low
    Use constant for subscriber threshold

    Consider using a constant or configuration value for the subscriber threshold
    instead of hardcoding it to 0. This allows for easier adjustment of logging behavior
    in the future.

    src/eth/rpc/rpc_subscriptions.rs [176-187]

    -if subscribers.len() > 0 {
    -    // Extrai apenas os nomes dos clientes
    +const SUBSCRIBER_THRESHOLD: usize = 0;
    +if subscribers.len() > SUBSCRIBER_THRESHOLD {
    +    // Extract only client names
         let client_names: Vec<String> = subscribers.iter()
             .map(|s| s.client.to_string())
             .collect();
         
         tracing::info!(
             tx_hash = ?tx_hash,
             clients = ?client_names,
             "notifying subscribers about new pending transaction"
         );
     }
    Suggestion importance[1-10]: 5

    __

    Why: The suggestion improves code maintainability by introducing a constant for the subscriber threshold. While it's a good practice, the impact is moderate as the current threshold of 0 is unlikely to change frequently.

    Low

    Streamline the code for extracting client names in RPC subscription logging by removing unnecessary line breaks and using a more concise iterator chain. This change maintains the previous logging functionality while improving code readability and reducing visual complexity.
    
    The modifications affect three different subscription notification scenarios:
    - Pending transaction notifications
    - New block header notifications
    - Log event notifications
    
    No functional changes are introduced, only code formatting and simplification.
    Replace detailed subscription object display with a concise client summary
    in notification logs. This change:
    
    1. Significantly reduces log verbosity by removing internal implementation details
    2. Groups clients by type (banking, issuing, stratus, etc.) and shows only the count
    3. Formats output as "type: count" for easy reading
    4. Maintains essential information like block hash and number
    
    Before this change, logs were showing the entire internal structure of subscription
    objects, resulting in extremely verbose logs. Now, logs show a concise summary
    of clients that is both informative and readable.
    
    This change improves log readability and reduces resource consumption in production
    environments, addressing feedback that logs were excessively verbose.
    
    Tested with multiple simultaneous clients to ensure essential information is
    preserved even with a large number of subscribers.
    Clean up unnecessary whitespace in RPC subscription logging code across multiple notification scenarios. This change:
    
    - Removes extra blank lines in client type counting logic
    - Maintains existing functionality without altering the core implementation
    - Improves code readability by reducing visual noise
    …thod
    
    Introduce a new `get_client_type` method to centralize and simplify client type extraction logic in RPC subscription notifications. This change:
    
    - Removes repeated client type identification code across different notification scenarios
    - Uses a more concise method to extract client type from the client string
    - Improves code maintainability by centralizing type extraction logic
    - Reduces code duplication in subscription notification handling
    @pedro-pelicioni-cw pedro-pelicioni-cw merged commit bc5d420 into main Feb 26, 2025
    40 checks passed
    @pedro-pelicioni-cw pedro-pelicioni-cw deleted the feature/enhance-subscription-logs branch February 26, 2025 19:15
    @pedro-pelicioni-cw pedro-pelicioni-cw linked an issue Feb 26, 2025 that may be closed by this pull request
    @gabriel-aranha-cw
    Copy link
    Contributor

    Final benchmark:
    Run ID: bench-29191575

    Git Info:

    Configuration:

    • Target Account Strategy: Default

    Leader Stats:
    RPS Stats: Max: 1806.00, Min: 746.00, Avg: 1541.66, StdDev: 94.24
    TPS Stats: Max: 1803.00, Min: 1333.00, Avg: 1497.12, StdDev: 92.88

    Follower Stats:
    Imported Blocks/s: Max: 4.00, Min: 1.00, Avg: 1.77, StdDev: 0.46
    Imported Transactions/s: Max: 3539.00, Min: 588.00, Avg: 2694.21, StdDev: 679.77

    Plots:

    # for free to join this conversation on GitHub. Already have an account? # to comment
    Labels
    None yet
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    Log when subscribers are notified
    2 participants