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

refactor: improve RPC subscription logging and code clarity #2027

Closed
wants to merge 2 commits into from

Conversation

pedro-pelicioni-cw
Copy link
Contributor

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

User description

  • Enhance logging for subscription events with more concise and informative messages
  • Add short_info() methods to provide cleaner subscriber information for tracing
  • Simplify code structure and remove unnecessary comments
  • Improve variable naming for better readability
  • Change log level for subscription events from info to debug

PR Type

Enhancement, Bug fix


Description

  • Improve logging clarity and conciseness

  • Add short_info() methods for better tracing

  • Refactor code structure and naming

  • Change log level for subscriptions to debug


Changes walkthrough 📝

Relevant files
Enhancement
rpc_subscriptions.rs
Refactor RPC subscriptions for better logging and clarity

src/eth/rpc/rpc_subscriptions.rs

  • Improved logging messages and levels
  • Added short_info() methods for subscriptions
  • Refactored code structure and variable naming
  • Updated subscription handling and notification logic
  • +101/-104

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • - Enhance logging for subscription events with more concise and informative messages
    - Add `short_info()` methods to provide cleaner subscriber information for tracing
    - Simplify code structure and remove unnecessary comments
    - Improve variable naming for better readability
    - Change log level for subscription events from `info` to `debug`
    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Logging Improvement

    The PR changes log levels from info to debug for subscription events. Ensure this change doesn't impact the ability to monitor and debug the system effectively.

    tracing::debug!(
        id = sink.subscription_id().to_string_ext(),
        %rpc_client,
        "subscribing to newPendingTransactions event"
    );
    Error Handling

    The error handling for the broadcast receiver in the notifier functions has been simplified. Verify that this doesn't lead to any loss of important error information or unexpected behavior.

    Ok(Ok(log)) => log,
    Ok(Err(_)) => break,
    Err(_) => continue,

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Improve notification sending reliability

    Implement a more robust error handling mechanism for the notification sending
    process, including retries and backoff strategies, to improve the reliability of
    subscription notifications in case of temporary network issues.

    src/eth/rpc/rpc_subscriptions.rs [287-289]

    -if let Err(e) = sink.send_timeout(msg_clone, NOTIFICATION_TIMEOUT).await {
    -    tracing::warn!(error = ?e, "failed to send notification");
    +const MAX_RETRIES: usize = 3;
    +const BACKOFF_BASE: Duration = Duration::from_millis(100);
    +
    +for retry in 0..MAX_RETRIES {
    +    match sink.send_timeout(msg_clone.clone(), NOTIFICATION_TIMEOUT).await {
    +        Ok(_) => break,
    +        Err(e) if retry < MAX_RETRIES - 1 => {
    +            let backoff = BACKOFF_BASE * 2u32.pow(retry as u32);
    +            tracing::warn!(error = ?e, retry, backoff = ?backoff, "failed to send notification, retrying");
    +            tokio::time::sleep(backoff).await;
    +        }
    +        Err(e) => {
    +            tracing::error!(error = ?e, "failed to send notification after max retries");
    +        }
    +    }
     }
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion introduces a robust error handling mechanism with retries and backoff strategies for sending notifications. This significantly improves the reliability of the subscription system, especially in cases of temporary network issues, which is crucial for maintaining consistent communication with subscribers.

    Medium
    Optimize subscription storage for concurrency

    Consider using a more efficient data structure for storing subscriptions, such as a
    concurrent hash map or a read-write lock per subscription type, to reduce contention
    and improve performance in high-concurrency scenarios.

    src/eth/rpc/rpc_subscriptions.rs [370-373]

    +use dashmap::DashMap;
    +
     pub struct RpcSubscriptionsConnected {
    -    pub pending_txs: RwLock<HashMap<ConnectionId, Subscription>>,
    -    pub new_heads: RwLock<HashMap<ConnectionId, Subscription>>,
    -    pub logs: RwLock<HashMap<ConnectionId, HashMap<LogFilter, SubscriptionWithFilter>>>,
    +    pub pending_txs: DashMap<ConnectionId, Subscription>,
    +    pub new_heads: DashMap<ConnectionId, Subscription>,
    +    pub logs: DashMap<ConnectionId, HashMap<LogFilter, SubscriptionWithFilter>>,
     }
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion proposes using a more efficient concurrent data structure (DashMap) for storing subscriptions, which could significantly improve performance in high-concurrency scenarios. This is a valuable optimization for a system dealing with multiple subscriptions.

    Medium

    @pedro-pelicioni-cw pedro-pelicioni-cw deleted the fixing-subscribers-notifications branch February 25, 2025 02:08
    # 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.

    1 participant